diff --git a/backend/onedrive/onedrive.go b/backend/onedrive/onedrive.go index 06817fce5..2973ba8ce 100755 --- a/backend/onedrive/onedrive.go +++ b/backend/onedrive/onedrive.go @@ -302,7 +302,6 @@ func init() { m.Set(configDriveID, finalDriveID) m.Set(configDriveType, rootItem.ParentReference.DriveType) - config.SaveConfig() }, Options: append(oauthutil.SharedOptions, []fs.Option{{ Name: "region", diff --git a/backend/seafile/seafile.go b/backend/seafile/seafile.go index 794efd856..784beb4d8 100644 --- a/backend/seafile/seafile.go +++ b/backend/seafile/seafile.go @@ -372,7 +372,6 @@ func Config(ctx context.Context, name string, m configmap.Mapper) { m.Set(configAuthToken, token) // And delete any previous entry for password m.Set(configPassword, "") - config.SaveConfig() // And we're done here break } diff --git a/fs/config/config.go b/fs/config/config.go index b42be2e9d..df13e2195 100644 --- a/fs/config/config.go +++ b/fs/config/config.go @@ -230,7 +230,7 @@ func LoadConfig(ctx context.Context) { var ErrorConfigFileNotFound = errors.New("config file not found") // SaveConfig calling function which saves configuration file. -// if saveConfig returns error trying again after sleep. +// if SaveConfig returns error trying again after sleep. func SaveConfig() { ctx := context.Background() ci := fs.GetConfig(ctx) @@ -253,19 +253,6 @@ func SaveConfig() { func SetValueAndSave(name, key, value string) error { // Set the value in config in case we fail to reload it Data.SetValue(name, key, value) - - // Reload the config file - err := Data.Load() - if err == ErrorConfigFileNotFound { - // Config file not written yet so ignore reload - return nil - } else if err != nil { - return err - } - if !Data.HasSection(name) { - // Section doesn't exist yet so ignore reload - return nil - } // Save it again SaveConfig() return nil @@ -281,20 +268,6 @@ func getWithDefault(name, key, defaultValue string) string { return value } -// FileGetFresh reads the config key under section return the value or -// an error if the config file was not found or that value couldn't be -// read. -func FileGetFresh(section, key string) (value string, err error) { - if err := Data.Load(); err != nil { - return "", err - } - value, found := Data.GetValue(section, key) - if !found { - return "", errors.New("value not found") - } - return value, nil -} - // UpdateRemote adds the keyValues passed in to the remote of name. // keyValues should be key, value pairs. func UpdateRemote(ctx context.Context, name string, keyValues rc.Params, doObscure, noObscure bool) error { @@ -446,11 +419,6 @@ func FileDeleteKey(section, key string) bool { var matchEnv = regexp.MustCompile(`^RCLONE_CONFIG_(.*?)_TYPE=.*$`) -// FileRefresh ensures the latest configFile is loaded from disk -func FileRefresh() error { - return Data.Load() -} - // FileSections returns the sections in the config file // including any defined by environment variables. func FileSections() []string { diff --git a/fs/config/config_test.go b/fs/config/config_test.go index 486416451..2e597d9c3 100644 --- a/fs/config/config_test.go +++ b/fs/config/config_test.go @@ -3,16 +3,12 @@ package config_test import ( - "bytes" "context" - "io/ioutil" "testing" "github.com/rclone/rclone/fs/config" "github.com/rclone/rclone/fs/config/configfile" - "github.com/rclone/rclone/fs/rc" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestConfigLoad(t *testing.T) { @@ -31,22 +27,3 @@ func TestConfigLoad(t *testing.T) { expect = []string{"type", "nounc"} assert.Equal(t, expect, keys) } - -func TestFileRefresh(t *testing.T) { - ctx := context.Background() - defer testConfigFile(t, "refresh.conf")() - require.NoError(t, config.CreateRemote(ctx, "refresh_test", "config_test_remote", rc.Params{ - "bool": true, - }, false, false)) - b, err := ioutil.ReadFile(config.ConfigPath) - assert.NoError(t, err) - - b = bytes.Replace(b, []byte("refresh_test"), []byte("refreshed_test"), 1) - err = ioutil.WriteFile(config.ConfigPath, b, 0644) - assert.NoError(t, err) - - assert.NotEqual(t, []string{"refreshed_test"}, config.Data.GetSectionList()) - err = config.FileRefresh() - assert.NoError(t, err) - assert.Equal(t, []string{"refreshed_test"}, config.Data.GetSectionList()) -} diff --git a/fs/config/configfile/configfile.go b/fs/config/configfile/configfile.go index 8a87ecb0d..a889a3450 100644 --- a/fs/config/configfile/configfile.go +++ b/fs/config/configfile/configfile.go @@ -1,3 +1,4 @@ +// Package configfile implements a config file loader and saver package configfile import ( @@ -6,6 +7,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sync" "github.com/Unknwon/goconfig" "github.com/pkg/errors" @@ -22,28 +24,54 @@ func LoadConfig(ctx context.Context) { // Storage implements config.Storage for saving and loading config // data in a simple INI based file. type Storage struct { - gc *goconfig.ConfigFile + gc *goconfig.ConfigFile // config file loaded - thread safe + mu sync.Mutex // to protect the following variables + fi os.FileInfo // stat of the file when last loaded } -// Load the config from permanent storage, decrypting if necessary -func (s *Storage) Load() (err error) { +// Check to see if we need to reload the config +func (s *Storage) check() { + s.mu.Lock() + defer s.mu.Unlock() + + // Check to see if config file has changed since it was last loaded + fi, err := os.Stat(config.ConfigPath) + if err == nil { + // check to see if config file has changed and if it has, reload it + if s.fi == nil || !fi.ModTime().Equal(s.fi.ModTime()) || fi.Size() != s.fi.Size() { + fs.Debugf(nil, "Config file has changed externaly - reloading") + err := s._load() + if err != nil { + fs.Errorf(nil, "Failed to read config file - using previous config: %v", err) + } + } + } +} + +// _load the config from permanent storage, decrypting if necessary +// +// mu must be held when calling this +func (s *Storage) _load() (err error) { // Make sure we have a sensible default even when we error defer func() { - if err != nil { + if s.gc == nil { s.gc, _ = goconfig.LoadFromReader(bytes.NewReader([]byte{})) } }() - b, err := os.Open(config.ConfigPath) + fd, err := os.Open(config.ConfigPath) if err != nil { if os.IsNotExist(err) { return config.ErrorConfigFileNotFound } return err } - defer fs.CheckClose(b, &err) + defer fs.CheckClose(fd, &err) - cryptReader, err := config.Decrypt(b) + // Update s.fi with the current file info + s.fi, _ = os.Stat(config.ConfigPath) + + cryptReader, err := config.Decrypt(fd) if err != nil { return err } @@ -57,8 +85,18 @@ func (s *Storage) Load() (err error) { return nil } +// Load the config from permanent storage, decrypting if necessary +func (s *Storage) Load() (err error) { + s.mu.Lock() + defer s.mu.Unlock() + return s._load() +} + // Save the config to permanent storage, encrypting if necessary func (s *Storage) Save() error { + s.mu.Lock() + defer s.mu.Unlock() + dir, name := filepath.Split(config.ConfigPath) err := os.MkdirAll(dir, os.ModePerm) if err != nil { @@ -116,11 +154,15 @@ func (s *Storage) Save() error { fs.Errorf(nil, "Failed to remove backup config file: %v", err) } + // Update s.fi with the newly written file + s.fi, _ = os.Stat(config.ConfigPath) + return nil } // Serialize the config into a string func (s *Storage) Serialize() (string, error) { + s.check() var buf bytes.Buffer if err := goconfig.SaveConfigData(s.gc, &buf); err != nil { return "", errors.Errorf("Failed to save config file: %v", err) @@ -131,6 +173,7 @@ func (s *Storage) Serialize() (string, error) { // HasSection returns true if section exists in the config file func (s *Storage) HasSection(section string) bool { + s.check() _, err := s.gc.GetSection(section) if err != nil { return false @@ -141,22 +184,26 @@ func (s *Storage) HasSection(section string) bool { // DeleteSection removes the named section and all config from the // config file func (s *Storage) DeleteSection(section string) { + s.check() s.gc.DeleteSection(section) } // GetSectionList returns a slice of strings with names for all the // sections func (s *Storage) GetSectionList() []string { + s.check() return s.gc.GetSectionList() } // GetKeyList returns the keys in this section func (s *Storage) GetKeyList(section string) []string { + s.check() return s.gc.GetKeyList(section) } // GetValue returns the key in section with a found flag func (s *Storage) GetValue(section string, key string) (value string, found bool) { + s.check() value, err := s.gc.GetValue(section, key) if err != nil { return "", false @@ -166,11 +213,13 @@ func (s *Storage) GetValue(section string, key string) (value string, found bool // SetValue sets the value under key in section func (s *Storage) SetValue(section string, key string, value string) { + s.check() s.gc.SetValue(section, key, value) } // DeleteKey removes the key under section func (s *Storage) DeleteKey(section string, key string) bool { + s.check() return s.gc.DeleteKey(section, key) } diff --git a/fs/config/configfile/configfile_test.go b/fs/config/configfile/configfile_test.go new file mode 100644 index 000000000..52e441988 --- /dev/null +++ b/fs/config/configfile/configfile_test.go @@ -0,0 +1,215 @@ +package configfile + +import ( + "fmt" + "io/ioutil" + "os" + "runtime" + "strings" + "testing" + + "github.com/rclone/rclone/fs/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var configData = `[one] +type = number1 +fruit = potato + +[two] +type = number2 +fruit = apple +topping = nuts + +[three] +type = number3 +fruit = banana + +` + +// Fill up a temporary config file with the testdata filename passed in +func setConfigFile(t *testing.T, data string) func() { + out, err := ioutil.TempFile("", "rclone-configfile-test") + require.NoError(t, err) + filePath := out.Name() + + _, err = out.Write([]byte(data)) + require.NoError(t, err) + + require.NoError(t, out.Close()) + + old := config.ConfigPath + config.ConfigPath = filePath + return func() { + config.ConfigPath = old + _ = os.Remove(filePath) + } +} + +// toUnix converts \r\n to \n in buf +func toUnix(buf string) string { + if runtime.GOOS == "windows" { + return strings.Replace(buf, "\r\n", "\n", -1) + } + return buf +} + +func TestConfigFile(t *testing.T) { + defer setConfigFile(t, configData)() + data := &Storage{} + + require.NoError(t, data.Load()) + + t.Run("Read", func(t *testing.T) { + t.Run("Serialize", func(t *testing.T) { + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, configData, toUnix(buf)) + }) + t.Run("HasSection", func(t *testing.T) { + assert.True(t, data.HasSection("one")) + assert.False(t, data.HasSection("missing")) + }) + t.Run("GetSectionList", func(t *testing.T) { + assert.Equal(t, []string{ + "one", + "two", + "three", + }, data.GetSectionList()) + }) + t.Run("GetKeyList", func(t *testing.T) { + assert.Equal(t, []string{ + "type", + "fruit", + "topping", + }, data.GetKeyList("two")) + assert.Equal(t, []string(nil), data.GetKeyList("unicorn")) + }) + t.Run("GetValue", func(t *testing.T) { + value, ok := data.GetValue("one", "type") + assert.True(t, ok) + assert.Equal(t, "number1", value) + value, ok = data.GetValue("three", "fruit") + assert.True(t, ok) + assert.Equal(t, "banana", value) + value, ok = data.GetValue("one", "typeX") + assert.False(t, ok) + assert.Equal(t, "", value) + value, ok = data.GetValue("threeX", "fruit") + assert.False(t, ok) + assert.Equal(t, "", value) + }) + }) + + //defer setConfigFile(configData)() + + t.Run("Write", func(t *testing.T) { + t.Run("SetValue", func(t *testing.T) { + data.SetValue("one", "extra", "42") + data.SetValue("two", "fruit", "acorn") + + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, `[one] +type = number1 +fruit = potato +extra = 42 + +[two] +type = number2 +fruit = acorn +topping = nuts + +[three] +type = number3 +fruit = banana + +`, toUnix(buf)) + t.Run("DeleteKey", func(t *testing.T) { + data.DeleteKey("one", "type") + data.DeleteKey("two", "missing") + data.DeleteKey("three", "fruit") + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, `[one] +fruit = potato +extra = 42 + +[two] +type = number2 +fruit = acorn +topping = nuts + +[three] +type = number3 + +`, toUnix(buf)) + t.Run("DeleteSection", func(t *testing.T) { + data.DeleteSection("two") + data.DeleteSection("missing") + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, `[one] +fruit = potato +extra = 42 + +[three] +type = number3 + +`, toUnix(buf)) + t.Run("Save", func(t *testing.T) { + require.NoError(t, data.Save()) + buf, err := ioutil.ReadFile(config.ConfigPath) + require.NoError(t, err) + assert.Equal(t, `[one] +fruit = potato +extra = 42 + +[three] +type = number3 + +`, toUnix(string(buf))) + }) + }) + }) + }) + }) +} + +func TestConfigFileReload(t *testing.T) { + defer setConfigFile(t, configData)() + data := &Storage{} + + require.NoError(t, data.Load()) + + value, ok := data.GetValue("three", "appended") + assert.False(t, ok) + assert.Equal(t, "", value) + + // Now write a new value on the end + out, err := os.OpenFile(config.ConfigPath, os.O_APPEND|os.O_WRONLY, 0777) + require.NoError(t, err) + fmt.Fprintln(out, "appended = what magic") + require.NoError(t, out.Close()) + + // And check we magically reloaded it + value, ok = data.GetValue("three", "appended") + assert.True(t, ok) + assert.Equal(t, "what magic", value) +} + +func TestConfigFileDoesNotExist(t *testing.T) { + defer setConfigFile(t, configData)() + data := &Storage{} + + require.NoError(t, os.Remove(config.ConfigPath)) + + err := data.Load() + require.Equal(t, config.ErrorConfigFileNotFound, err) + + // check that using data doesn't crash + value, ok := data.GetValue("three", "appended") + assert.False(t, ok) + assert.Equal(t, "", value) +} diff --git a/lib/oauthutil/oauthutil.go b/lib/oauthutil/oauthutil.go index f332d8af3..d04aed11f 100644 --- a/lib/oauthutil/oauthutil.go +++ b/lib/oauthutil/oauthutil.go @@ -148,14 +148,8 @@ func PutToken(name string, m configmap.Mapper, token *oauth2.Token, newSection b tokenString := string(tokenBytes) old, ok := m.Get(config.ConfigToken) if !ok || tokenString != old { - err = config.SetValueAndSave(name, config.ConfigToken, tokenString) - if newSection && err != nil { - fs.Debugf(name, "Added new token to config, still needs to be saved") - } else if err != nil { - fs.Errorf(nil, "Failed to save new token in config file: %v", err) - } else { - fs.Debugf(name, "Saved new token in config file") - } + m.Set(config.ConfigToken, tokenString) + fs.Debugf(name, "Saved new token in config file") } return nil } @@ -175,13 +169,13 @@ type TokenSource struct { // If token has expired then first try re-reading it from the config // file in case a concurrently running rclone has updated it already func (ts *TokenSource) reReadToken() bool { - tokenString, err := config.FileGetFresh(ts.name, config.ConfigToken) - if err != nil { - fs.Debugf(ts.name, "Failed to read token out of config file: %v", err) + tokenString, found := ts.m.Get(config.ConfigToken) + if !found || tokenString == "" { + fs.Debugf(ts.name, "Failed to read token out of config file") return false } newToken := new(oauth2.Token) - err = json.Unmarshal([]byte(tokenString), newToken) + err := json.Unmarshal([]byte(tokenString), newToken) if err != nil { fs.Debugf(ts.name, "Failed to parse token out of config file: %v", err) return false