From 0ca44829774627b2a5ea5e76bea6879ec9c221e2 Mon Sep 17 00:00:00 2001 From: Anderson Mesquita Date: Wed, 16 Dec 2015 14:29:37 -0500 Subject: [PATCH 1/3] Refactor upgrade and check upgrade cmdline options The main() function is growing too big (142 lines as of the date of this commit), so this attempts to extract some functionality out of there and into their own functions to make it easier to reason about them and keep functions short and concise. --- cmd/syncthing/main.go | 83 ++++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index cdd1d18e6..27bf9dae3 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -305,43 +305,14 @@ func main() { return } - if doUpgrade || doUpgradeCheck { - releasesURL := "https://api.github.com/repos/syncthing/syncthing/releases?per_page=30" - if cfg, _, err := loadConfig(locations[locConfigFile]); err == nil { - releasesURL = cfg.Options().ReleasesURL - } - rel, err := upgrade.LatestRelease(releasesURL, Version) - if err != nil { - l.Fatalln("Upgrade:", err) // exits 1 - } - - if upgrade.CompareVersions(rel.Tag, Version) <= 0 { - l.Infof("No upgrade available (current %q >= latest %q).", Version, rel.Tag) - os.Exit(exitNoUpgradeAvailable) - } - - l.Infof("Upgrade available (current %q < latest %q)", Version, rel.Tag) - - if doUpgrade { - // Use leveldb database locks to protect against concurrent upgrades - _, err = db.Open(locations[locDatabase]) - if err != nil { - l.Infoln("Attempting upgrade through running Syncthing...") - err = upgradeViaRest() - if err != nil { - l.Fatalln("Upgrade:", err) - } - l.Okln("Syncthing upgrading") - return - } - - err = upgrade.To(rel) - if err != nil { - l.Fatalln("Upgrade:", err) // exits 1 - } - l.Okf("Upgraded to %q", rel.Tag) - } + if doUpgradeCheck { + checkUpgrade() + return + } + if doUpgrade { + release := checkUpgrade() + performUpgrade(release) return } @@ -429,6 +400,46 @@ func debugFacilities() string { return b.String() } +func checkUpgrade() upgrade.Release { + releasesURL := "https://api.github.com/repos/syncthing/syncthing/releases?per_page=30" + if cfg, _, err := loadConfig(locations[locConfigFile]); err == nil { + releasesURL = cfg.Options().ReleasesURL + } + release, err := upgrade.LatestRelease(releasesURL, Version) + if err != nil { + l.Fatalln("Upgrade:", err) + } + + if upgrade.CompareVersions(release.Tag, Version) <= 0 { + noUpgradeMessage := "No upgrade available (current %q >= latest %q)." + l.Infof(noUpgradeMessage, Version, release.Tag) + os.Exit(exitNoUpgradeAvailable) + } + + l.Infof("Upgrade available (current %q < latest %q)", Version, release.Tag) + return release +} + +func performUpgrade(release upgrade.Release) { + // Use leveldb database locks to protect against concurrent upgrades + _, err := db.Open(locations[locDatabase]) + if err == nil { + err = upgrade.To(release) + if err != nil { + l.Fatalln("Upgrade:", err) + } + l.Okf("Upgraded to %q", release.Tag) + } else { + l.Infoln("Attempting upgrade through running Syncthing...") + err = upgradeViaRest() + if err != nil { + l.Fatalln("Upgrade:", err) + } + l.Okln("Syncthing upgrading") + os.Exit(exitUpgrading) + } +} + func upgradeViaRest() error { cfg, err := config.Load(locations[locConfigFile], protocol.LocalDeviceID) if err != nil { From a0b7ac402d948f994ad66d2ebcd45b121455fbf9 Mon Sep 17 00:00:00 2001 From: Anderson Mesquita Date: Wed, 16 Dec 2015 15:03:20 -0500 Subject: [PATCH 2/3] Refactor main.ensureDir() ensureDir() did not handle one last error case and there was some logic in the main() function that belonged to ensureDir() as well. It was also creating a directory with a hardcoded 0700 mode, regardless of what mode was passed to it. This refactors it a little to fix the broken behavior, avoid redundant checks by taking advantage of the behavior of MkdirAll, and move the extra logic from main() into ensureDir(). --- cmd/syncthing/main.go | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 27bf9dae3..599ea458a 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -289,10 +289,6 @@ func main() { return } - if info, err := os.Stat(baseDirs["config"]); err == nil && !info.IsDir() { - l.Fatalln("Config directory", baseDirs["config"], "is not a directory") - } - // Ensure that our home directory exists. ensureDir(baseDirs["config"], 0700) @@ -1007,15 +1003,16 @@ func shutdown() { stop <- exitSuccess } -func ensureDir(dir string, mode int) { - fi, err := os.Stat(dir) - if os.IsNotExist(err) { - err := osutil.MkdirAll(dir, 0700) - if err != nil { - l.Fatalln(err) - } - } else if mode >= 0 && err == nil && int(fi.Mode()&0777) != mode { - err := os.Chmod(dir, os.FileMode(mode)) +func ensureDir(dir string, mode os.FileMode) { + err := osutil.MkdirAll(dir, mode) + if err != nil { + l.Fatalln(err) + } + + fi, _ := os.Stat(dir) + currentMode := fi.Mode() & 0777 + if mode >= 0 && currentMode != mode { + err := os.Chmod(dir, mode) // This can fail on crappy filesystems, nothing we can do about it. if err != nil { l.Warnln(err) From 4098f97735752f787cdb678ebb390efd4d8616a6 Mon Sep 17 00:00:00 2001 From: Anderson Mesquita Date: Wed, 16 Dec 2015 15:33:14 -0500 Subject: [PATCH 3/3] Extract cmdline option parsing into a new function Another step towards reducing the general size of the main() function in favor of shorter, more focused functions. --- cmd/syncthing/main.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 599ea458a..487a1deff 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -203,6 +203,9 @@ var ( auditEnabled bool verbose bool paused bool + guiAddress string + guiAPIKey string + generateDir string noRestart = os.Getenv("STNORESTART") != "" noUpgrade = os.Getenv("STNOUPGRADE") != "" profiler = os.Getenv("STPROFILER") @@ -212,7 +215,7 @@ var ( innerProcess = os.Getenv("STNORESTART") != "" || os.Getenv("STMONITORED") != "" ) -func main() { +func parseCommandLineOptions() { if runtime.GOOS == "windows" { // On Windows, we use a log file by default. Setting the -logfile flag // to "-" disables this behavior. @@ -224,8 +227,6 @@ func main() { flag.StringVar(&logFile, "logfile", "-", "Log file name (use \"-\" for stdout)") } - var guiAddress, guiAPIKey string - var generateDir string flag.StringVar(&generateDir, "generate", "", "Generate key and config in specified dir, then exit") flag.StringVar(&guiAddress, "gui-address", guiAddress, "Override GUI address (e.g. \"http://192.0.2.42:8443\")") flag.StringVar(&guiAPIKey, "gui-apikey", guiAPIKey, "Override GUI API key") @@ -245,6 +246,10 @@ func main() { longUsage := fmt.Sprintf(extraUsage, baseDirs["config"], debugFacilities()) flag.Usage = usageFor(flag.CommandLine, usage, longUsage) flag.Parse() +} + +func main() { + parseCommandLineOptions() if guiAddress != "" { // The config picks this up from the environment.