From bf89bffb0b844cfafc8a38467870157e52e6f198 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Mon, 22 Nov 2021 08:45:29 +0100 Subject: [PATCH] lib/config: Decouple VerifyConfiguration from Committer (#7939) ... and remove 8/10 implementations, which were no-ops. This saves code and time copying configurations. --- lib/api/api.go | 2 ++ lib/config/wrapper.go | 23 ++++++++++++++++------- lib/connections/limiter.go | 4 ---- lib/connections/service.go | 4 ---- lib/discover/manager.go | 4 ---- lib/model/model.go | 2 ++ lib/model/progressemitter.go | 5 ----- lib/nat/service.go | 4 ---- lib/ur/failurereporting.go | 4 ---- lib/ur/usage_report.go | 4 ---- lib/watchaggregator/aggregator.go | 4 ---- 11 files changed, 20 insertions(+), 40 deletions(-) diff --git a/lib/api/api.go b/lib/api/api.go index 360e94212..6ba4235f3 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -94,6 +94,8 @@ type service struct { systemLog logger.Recorder } +var _ config.Verifier = &service{} + type Service interface { suture.Service config.Committer diff --git a/lib/config/wrapper.go b/lib/config/wrapper.go index 3482b135a..4d5c8f93c 100644 --- a/lib/config/wrapper.go +++ b/lib/config/wrapper.go @@ -31,14 +31,14 @@ const ( var errTooManyModifications = errors.New("too many concurrent config modifications") -// The Committer interface is implemented by objects that need to know about -// or have a say in configuration changes. +// The Committer and Verifier interfaces are implemented by objects +// that need to know about or have a say in configuration changes. // // When the configuration is about to be changed, VerifyConfiguration() is -// called for each subscribing object, with the old and new configuration. A -// nil error is returned if the new configuration is acceptable (i.e. does not -// contain any errors that would prevent it from being a valid config). -// Otherwise an error describing the problem is returned. +// called for each subscribing object that implements it, with copies of the +// old and new configuration. A nil error is returned if the new configuration +// is acceptable (i.e., does not contain any errors that would prevent it from +// being a valid config). Otherwise an error describing the problem is returned. // // If any subscriber returns an error from VerifyConfiguration(), the // configuration change is not committed and an error is returned to whoever @@ -55,11 +55,16 @@ var errTooManyModifications = errors.New("too many concurrent config modificatio // configuration (e.g. calling Wrapper.SetFolder), that are also acquired in any // methods of the Committer interface. type Committer interface { - VerifyConfiguration(from, to Configuration) error CommitConfiguration(from, to Configuration) (handled bool) String() string } +// A Verifier can determine if a new configuration is acceptable. +// See the description for Committer, above. +type Verifier interface { + VerifyConfiguration(from, to Configuration) error +} + // Waiter allows to wait for the given config operation to complete. type Waiter interface { Wait() @@ -300,6 +305,10 @@ func (w *wrapper) replaceLocked(to Configuration) (Waiter, error) { } for _, sub := range w.subs { + sub, ok := sub.(Verifier) + if !ok { + continue + } l.Debugln(sub, "verifying configuration") if err := sub.VerifyConfiguration(from.Copy(), to.Copy()); err != nil { l.Debugln(sub, "rejected config:", err) diff --git a/lib/connections/limiter.go b/lib/connections/limiter.go index eac63d70e..66331c221 100644 --- a/lib/connections/limiter.go +++ b/lib/connections/limiter.go @@ -122,10 +122,6 @@ func (lim *limiter) processDevicesConfigurationLocked(from, to config.Configurat } } -func (lim *limiter) VerifyConfiguration(from, to config.Configuration) error { - return nil -} - func (lim *limiter) CommitConfiguration(from, to config.Configuration) bool { // to ensure atomic update of configuration lim.mu.Lock() diff --git a/lib/connections/service.go b/lib/connections/service.go index 3848b2616..24a6f1420 100644 --- a/lib/connections/service.go +++ b/lib/connections/service.go @@ -706,10 +706,6 @@ func (s *service) logListenAddressesChangedEvent(l ListenerAddresses) { }) } -func (s *service) VerifyConfiguration(from, to config.Configuration) error { - return nil -} - func (s *service) CommitConfiguration(from, to config.Configuration) bool { newDevices := make(map[protocol.DeviceID]bool, len(to.Devices)) for _, dev := range to.Devices { diff --git a/lib/discover/manager.go b/lib/discover/manager.go index ec43e889c..686d67310 100644 --- a/lib/discover/manager.go +++ b/lib/discover/manager.go @@ -227,10 +227,6 @@ func (m *manager) Cache() map[protocol.DeviceID]CacheEntry { return res } -func (m *manager) VerifyConfiguration(_, _ config.Configuration) error { - return nil -} - func (m *manager) CommitConfiguration(_, to config.Configuration) (handled bool) { m.mut.Lock() defer m.mut.Unlock() diff --git a/lib/model/model.go b/lib/model/model.go index 28a3a5469..e722a90cc 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -168,6 +168,8 @@ type model struct { foldersRunning int32 } +var _ config.Verifier = &model{} + type folderFactory func(*model, *db.FileSet, *ignore.Matcher, config.FolderConfiguration, versioner.Versioner, events.Logger, *util.Semaphore) service var ( diff --git a/lib/model/progressemitter.go b/lib/model/progressemitter.go index 56bf92e27..fab9e72c9 100644 --- a/lib/model/progressemitter.go +++ b/lib/model/progressemitter.go @@ -212,11 +212,6 @@ func (t *ProgressEmitter) computeProgressUpdates() []progressUpdate { return progressUpdates } -// VerifyConfiguration implements the config.Committer interface -func (t *ProgressEmitter) VerifyConfiguration(from, to config.Configuration) error { - return nil -} - // CommitConfiguration implements the config.Committer interface func (t *ProgressEmitter) CommitConfiguration(_, to config.Configuration) bool { t.mut.Lock() diff --git a/lib/nat/service.go b/lib/nat/service.go index 078587961..5e2f1c704 100644 --- a/lib/nat/service.go +++ b/lib/nat/service.go @@ -45,10 +45,6 @@ func NewService(id protocol.DeviceID, cfg config.Wrapper) *Service { return s } -func (s *Service) VerifyConfiguration(from, to config.Configuration) error { - return nil -} - func (s *Service) CommitConfiguration(from, to config.Configuration) bool { s.mut.Lock() if !s.enabled && to.Options.NATEnabled { diff --git a/lib/ur/failurereporting.go b/lib/ur/failurereporting.go index 899bd0e30..f8d4d8850 100644 --- a/lib/ur/failurereporting.go +++ b/lib/ur/failurereporting.go @@ -188,10 +188,6 @@ func (h *failureHandler) addReport(data FailureData, evTime time.Time) { } } -func (h *failureHandler) VerifyConfiguration(_, _ config.Configuration) error { - return nil -} - func (h *failureHandler) CommitConfiguration(from, to config.Configuration) bool { if from.Options.CREnabled != to.Options.CREnabled || from.Options.CRURL != to.Options.CRURL { h.optsChan <- to.Options diff --git a/lib/ur/usage_report.go b/lib/ur/usage_report.go index a2d2b0923..f6537f20f 100644 --- a/lib/ur/usage_report.go +++ b/lib/ur/usage_report.go @@ -390,10 +390,6 @@ func (s *Service) Serve(ctx context.Context) error { } } -func (s *Service) VerifyConfiguration(from, to config.Configuration) error { - return nil -} - func (s *Service) CommitConfiguration(from, to config.Configuration) bool { if from.Options.URAccepted != to.Options.URAccepted || from.Options.URUniqueID != to.Options.URUniqueID || from.Options.URURL != to.Options.URURL { select { diff --git a/lib/watchaggregator/aggregator.go b/lib/watchaggregator/aggregator.go index 93a38c4ba..5c176fde8 100644 --- a/lib/watchaggregator/aggregator.go +++ b/lib/watchaggregator/aggregator.go @@ -419,10 +419,6 @@ func (a *aggregator) String() string { return fmt.Sprintf("aggregator/%s:", a.folderCfg.Description()) } -func (a *aggregator) VerifyConfiguration(from, to config.Configuration) error { - return nil -} - func (a *aggregator) CommitConfiguration(from, to config.Configuration) bool { for _, folderCfg := range to.Folders { if folderCfg.ID == a.folderID {