From 9323f0faf83316963c50c180490d34284b28fc0c Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sun, 26 Jun 2016 10:07:27 +0000 Subject: [PATCH] lib/model: Refactor CheckFolderHealth into separate methods While attempting to fix #2782 I thought the problem was the CheckFolderHealth method, so I cleaned it up. That turned out not to be the case, but I think this is better anyhow. It also moves the "create folder and marker if the folder was empty in the index" code to StartFolder where I think it makes better sense. This is covered by a number of existing tests. GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3343 --- cmd/syncthing/main_test.go | 139 ------------------------------------- lib/model/model.go | 134 +++++++++++++++++++++++------------ 2 files changed, 90 insertions(+), 183 deletions(-) diff --git a/cmd/syncthing/main_test.go b/cmd/syncthing/main_test.go index b58905b4..4f7f7404 100644 --- a/cmd/syncthing/main_test.go +++ b/cmd/syncthing/main_test.go @@ -7,151 +7,12 @@ package main import ( - "os" "testing" "github.com/syncthing/syncthing/lib/config" - "github.com/syncthing/syncthing/lib/db" - "github.com/syncthing/syncthing/lib/model" "github.com/syncthing/syncthing/lib/protocol" ) -func TestFolderErrors(t *testing.T) { - // This test intentionally avoids starting the folders. If they are - // started, they will perform an initial scan, which will create missing - // folder markers and race with the stuff we do in the test. - - fcfg := config.FolderConfiguration{ - ID: "folder", - RawPath: "testdata/testfolder", - } - cfg := config.Wrap("/tmp/test", config.Configuration{ - Folders: []config.FolderConfiguration{fcfg}, - }) - - for _, file := range []string{".stfolder", "testfolder/.stfolder", "testfolder"} { - if err := os.Remove("testdata/" + file); err != nil && !os.IsNotExist(err) { - t.Fatal(err) - } - } - - ldb := db.OpenMemory() - - // Case 1 - new folder, directory and marker created - - m := model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil) - m.AddFolder(fcfg) - - if err := m.CheckFolderHealth("folder"); err != nil { - t.Error("Unexpected error", cfg.Folders()["folder"].Invalid) - } - - s, err := os.Stat("testdata/testfolder") - if err != nil || !s.IsDir() { - t.Error(err) - } - - _, err = os.Stat("testdata/testfolder/.stfolder") - if err != nil { - t.Error(err) - } - - if err := os.Remove("testdata/testfolder/.stfolder"); err != nil { - t.Fatal(err) - } - if err := os.Remove("testdata/testfolder/"); err != nil { - t.Fatal(err) - } - - // Case 2 - new folder, marker created - - fcfg.RawPath = "testdata/" - cfg = config.Wrap("/tmp/test", config.Configuration{ - Folders: []config.FolderConfiguration{fcfg}, - }) - - m = model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil) - m.AddFolder(fcfg) - - if err := m.CheckFolderHealth("folder"); err != nil { - t.Error("Unexpected error", cfg.Folders()["folder"].Invalid) - } - - _, err = os.Stat("testdata/.stfolder") - if err != nil { - t.Error(err) - } - - if err := os.Remove("testdata/.stfolder"); err != nil { - t.Fatal(err) - } - - // Case 3 - Folder marker missing - - set := db.NewFileSet("folder", ldb) - set.Update(protocol.LocalDeviceID, []protocol.FileInfo{ - {Name: "dummyfile"}, - }) - - m = model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil) - m.AddFolder(fcfg) - - if err := m.CheckFolderHealth("folder"); err == nil || err.Error() != "folder marker missing" { - t.Error("Incorrect error: Folder marker missing !=", m.CheckFolderHealth("folder")) - } - - // Case 3.1 - recover after folder marker missing - - if err = fcfg.CreateMarker(); err != nil { - t.Error(err) - } - - if err := m.CheckFolderHealth("folder"); err != nil { - t.Error("Unexpected error", cfg.Folders()["folder"].Invalid) - } - - // Case 4 - Folder path missing - - if err := os.Remove("testdata/testfolder/.stfolder"); err != nil && !os.IsNotExist(err) { - t.Fatal(err) - } - if err := os.Remove("testdata/testfolder"); err != nil && !os.IsNotExist(err) { - t.Fatal(err) - } - - fcfg.RawPath = "testdata/testfolder" - cfg = config.Wrap("testdata/subfolder", config.Configuration{ - Folders: []config.FolderConfiguration{fcfg}, - }) - - m = model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil) - m.AddFolder(fcfg) - - if err := m.CheckFolderHealth("folder"); err == nil || err.Error() != "folder path missing" { - t.Error("Incorrect error: Folder path missing !=", m.CheckFolderHealth("folder")) - } - - // Case 4.1 - recover after folder path missing - - if err := os.Mkdir("testdata/testfolder", 0700); err != nil { - t.Fatal(err) - } - - if err := m.CheckFolderHealth("folder"); err == nil || err.Error() != "folder marker missing" { - t.Error("Incorrect error: Folder marker missing !=", m.CheckFolderHealth("folder")) - } - - // Case 4.2 - recover after missing marker - - if err = fcfg.CreateMarker(); err != nil { - t.Error(err) - } - - if err := m.CheckFolderHealth("folder"); err != nil { - t.Error("Unexpected error", cfg.Folders()["folder"].Invalid) - } -} - func TestShortIDCheck(t *testing.T) { cfg := config.Wrap("/tmp/test", config.Configuration{ Devices: []config.DeviceConfiguration{ diff --git a/lib/model/model.go b/lib/model/model.go index efa69959..5bcec120 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -108,6 +108,14 @@ var ( folderFactories = make(map[config.FolderType]folderFactory, 0) ) +// errors returned by the CheckFolderHealth method +var ( + errFolderPathMissing = errors.New("folder path missing") + errFolderMarkerMissing = errors.New("folder marker missing") + errHomeDiskNoSpace = errors.New("home disk has insufficient free space") + errFolderNoSpace = errors.New("folder has insufficient free space") +) + // NewModel creates and starts a new model. The model starts in read-only mode, // where it sends index information to connected peers and responds to requests // for file data without altering the local folder in any way. @@ -162,7 +170,7 @@ func (m *Model) StartDeadlockDetector(timeout time.Duration) { deadlockDetect(m.pmut, timeout) } -// StartFolder constrcuts the folder service and starts it. +// StartFolder constructs the folder service and starts it. func (m *Model) StartFolder(folder string) { m.fmut.Lock() cfg, ok := m.folderCfgs[folder] @@ -180,6 +188,26 @@ func (m *Model) StartFolder(folder string) { panic(fmt.Sprintf("unknown folder type 0x%x", cfg.Type)) } + fs := m.folderFiles[folder] + v, ok := fs.LocalVersion(protocol.LocalDeviceID), true + indexHasFiles := ok && v > 0 + if !indexHasFiles { + // It's a blank folder, so this may the first time we're looking at + // it. Attempt to create and tag with our marker as appropriate. We + // don't really do anything with errors at this point except warn - + // if these things don't work, we still want to start the folder and + // it'll show up as errored later. + + if _, err := os.Stat(cfg.Path()); os.IsNotExist(err) { + if err := osutil.MkdirAll(cfg.Path(), 0700); err != nil { + l.Warnln("Creating folder:", err) + } + } + if err := cfg.CreateMarker(); err != nil { + l.Warnln("Creating folder marker:", err) + } + } + var ver versioner.Versioner if len(cfg.Versioning.Type) > 0 { versionerFactory, ok := versioner.Factories[cfg.Versioning.Type] @@ -1904,53 +1932,73 @@ func (m *Model) CheckFolderHealth(id string) error { return errors.New("folder does not exist") } - if minFree := m.cfg.Options().MinHomeDiskFreePct; minFree > 0 { - if free, err := osutil.DiskFreePercentage(m.cfg.ConfigPath()); err == nil && free < minFree { - return errors.New("home disk has insufficient free space") - } + // Check for folder errors, with the most serious and specific first and + // generic ones like out of space on the home disk later. Note the + // inverted error flow (err==nil checks) here. + + err := m.checkFolderPath(folder) + if err == nil { + err = m.checkFolderFreeSpace(folder) + } + if err == nil { + err = m.checkHomeDiskFree() } - fi, err := os.Stat(folder.Path()) + // Set or clear the error on the runner, which also does logging and + // generates events and stuff. + m.runnerExchangeError(folder, err) - v, ok := m.CurrentLocalVersion(id) - indexHasFiles := ok && v > 0 + return err +} - if indexHasFiles { - // There are files in the folder according to the index, so it must - // have existed and had a correct marker at some point. Verify that - // this is still the case. - - switch { - case err != nil || !fi.IsDir(): - err = errors.New("folder path missing") - - case !folder.HasMarker(): - err = errors.New("folder marker missing") - - case folder.Type != config.FolderTypeReadOnly: - // Check for free space, if it isn't a master folder. We aren't - // going to change the contents of master folders, so we don't - // care about the amount of free space there. - diskFreeP, errDfp := osutil.DiskFreePercentage(folder.Path()) - if errDfp == nil && diskFreeP < folder.MinDiskFreePct { - diskFreeBytes, _ := osutil.DiskFreeBytes(folder.Path()) - str := fmt.Sprintf("insufficient free space (%d MiB, %.2f%%)", diskFreeBytes/1024/1024, diskFreeP) - err = errors.New(str) - } - } - } else { - // It's a blank folder, so this may the first time we're looking at - // it. Attempt to create and tag with our marker as appropriate. - - if os.IsNotExist(err) { - err = osutil.MkdirAll(folder.Path(), 0700) - } - - if err == nil && !folder.HasMarker() { - err = folder.CreateMarker() - } +// checkFolderPath returns nil if the folder path exists and has the marker file. +func (m *Model) checkFolderPath(folder config.FolderConfiguration) error { + if fi, err := os.Stat(folder.Path()); err != nil || !fi.IsDir() { + return errFolderPathMissing } + if !folder.HasMarker() { + return errFolderMarkerMissing + } + + return nil +} + +// checkFolderFreeSpace returns nil if the folder has the required amount of +// free space, or if folder free space checking is disabled. +func (m *Model) checkFolderFreeSpace(folder config.FolderConfiguration) error { + if folder.MinDiskFreePct <= 0 { + return nil + } + + free, err := osutil.DiskFreePercentage(folder.Path()) + if err == nil && free < folder.MinDiskFreePct { + return errFolderNoSpace + } + + return nil +} + +// checkHomeDiskFree returns nil if the home disk has the required amount of +// free space, or if home disk free space checking is disabled. +func (m *Model) checkHomeDiskFree() error { + minFree := m.cfg.Options().MinHomeDiskFreePct + if minFree <= 0 { + return nil + } + + free, err := osutil.DiskFreePercentage(m.cfg.ConfigPath()) + if err == nil && free < minFree { + return errHomeDiskNoSpace + } + + return nil +} + +// runnerExchangeError sets the given error (which way be nil) on the folder +// runner. If the error differs from any previous error, logging and events +// happen. +func (m *Model) runnerExchangeError(folder config.FolderConfiguration, err error) { m.fmut.RLock() runner, runnerExists := m.folderRunners[folder.ID] m.fmut.RUnlock() @@ -1975,8 +2023,6 @@ func (m *Model) CheckFolderHealth(id string) error { runner.clearError() } } - - return err } func (m *Model) ResetFolder(folder string) {