Simplify validation of the config

Get rid of the Check(), do it all in Parse().

config.go

 package tabacco
 
 import (
-	"crypto/rand"
 	"encoding/binary"
 	"errors"
 	"fmt"
@@ -11,9 +10,10 @@ import (
 	"sync"
 
 	"git.autistici.org/ai3/go-common/clientutil"
+	"gopkg.in/yaml.v2"
+
 	"git.autistici.org/ale/tabacco/jobs"
 	"git.autistici.org/ale/tabacco/util"
-	"gopkg.in/yaml.v2"
 )
 
 var defaultSeedFile = "/var/tmp/.tabacco_scheduler_seed"
@@ -22,22 +22,91 @@ var defaultSeedFile = "/var/tmp/.tabacco_scheduler_seed"
 // configuration is spread over multiple files and directories, this
 // holds it all together.
 type Config struct {
-	Hostname   string         `yaml:"hostname"`
-	Queue      jobs.QueueSpec `yaml:"queue_config"`
-	Repository RepositorySpec `yaml:"repository"`
-
-	DryRun           bool `yaml:"dry_run"`
-	DefaultNiceLevel int  `yaml:"default_nice_level"`
-	DefaultIOClass   int  `yaml:"default_io_class"`
-
+	Hostname             string                    `yaml:"hostname"`
+	Queue                jobs.QueueSpec            `yaml:"queue_config"`
+	Repository           RepositorySpec            `yaml:"repository"`
+	DryRun               bool                      `yaml:"dry_run"`
+	DefaultNiceLevel     int                       `yaml:"default_nice_level"`
+	DefaultIOClass       int                       `yaml:"default_io_class"`
+	WorkDir              string                    `yaml:"work_dir"`
+	RandomSeedFile       string                    `yaml:"random_seed_file"`
 	MetadataStoreBackend *clientutil.BackendConfig `yaml:"metadb"`
 
-	RandomSeedFile string `yaml:"random_seed_file"`
-
 	HandlerSpecs []HandlerSpec
 	SourceSpecs  []SourceSpec
 }
 
+type runtimeAssets struct {
+	handlerMap map[string]Handler
+	repo       Repository
+	seed       int64
+	shell      *Shell
+}
+
+func (a *runtimeAssets) Close() {
+	a.repo.Close() // nolint
+}
+
+func buildHandlerMap(specs []HandlerSpec, shell *Shell) (map[string]Handler, error) {
+	m := make(map[string]Handler)
+	merr := new(util.MultiError)
+	for _, spec := range specs {
+		h, err := spec.Parse(shell)
+		if err != nil {
+			merr.Add(err)
+			continue
+		}
+		m[spec.Name] = h
+	}
+	return m, merr.OrNil()
+}
+
+func (c *Config) parse() (*runtimeAssets, error) {
+	shell := NewShell(c.DryRun)
+	shell.SetNiceLevel(c.DefaultNiceLevel)
+	shell.SetIOClass(c.DefaultIOClass)
+
+	// Parse the repository config. An error here is fatal, as we
+	// don't have a way to operate without a repository.
+	repo, err := c.Repository.Parse(shell)
+	if err != nil {
+		return nil, err
+	}
+
+	merr := new(util.MultiError)
+
+	// Build the handlers.
+	handlerMap, err := buildHandlerMap(c.HandlerSpecs, shell)
+	if err != nil {
+		merr.Add(err)
+	}
+
+	// Validate the sources (Parse is called later at runtime).
+	var srcs []SourceSpec
+	for _, spec := range c.SourceSpecs {
+		if err := spec.Check(handlerMap); err != nil {
+			merr.Add(err)
+			continue
+		}
+		srcs = append(srcs, spec)
+	}
+	c.SourceSpecs = srcs
+
+	// Read (or create) the seed file.
+	seedFile := defaultSeedFile
+	if c.RandomSeedFile != "" {
+		seedFile = c.RandomSeedFile
+	}
+	seed := mustGetSeed(seedFile)
+
+	return &runtimeAssets{
+		shell:      shell,
+		repo:       repo,
+		handlerMap: handlerMap,
+		seed:       seed,
+	}, merr.OrNil()
+}
+
 func readHandlersFromDir(dir string) ([]HandlerSpec, error) {
 	var out []HandlerSpec
 	err := foreachYAMLFile(dir, func(path string) error {
@@ -46,23 +115,13 @@ func readHandlersFromDir(dir string) ([]HandlerSpec, error) {
 		if err := readYAMLFile(path, &spec); err != nil {
 			return err
 		}
-		if err := spec.Check(); err != nil {
-			return fmt.Errorf("%s: %v", path, err)
-		}
 		out = append(out, spec)
 		return nil
 	})
 	return out, err
 }
 
-func readSourcesFromDir(dir string, handlerSpecs []HandlerSpec) ([]SourceSpec, error) {
-	// Build a temporary set of handlers, so we can check for
-	// non-existing ones.
-	tmp := make(map[string]struct{})
-	for _, h := range handlerSpecs {
-		tmp[h.Name] = struct{}{}
-	}
-
+func readSourcesFromDir(dir string) ([]SourceSpec, error) {
 	var out []SourceSpec
 	err := foreachYAMLFile(dir, func(path string) error {
 		var spec SourceSpec
@@ -70,9 +129,6 @@ func readSourcesFromDir(dir string, handlerSpecs []HandlerSpec) ([]SourceSpec, e
 		if err := readYAMLFile(path, &spec); err != nil {
 			return err
 		}
-		if err := spec.Check(tmp); err != nil {
-			return fmt.Errorf("%s: %v", path, err)
-		}
 		out = append(out, spec)
 		return nil
 	})
@@ -91,9 +147,6 @@ func ReadConfig(path string) (*Config, error) {
 	if err := readYAMLFile(path, &config); err != nil {
 		return nil, err
 	}
-	if err := config.Repository.Check(); err != nil {
-		return nil, err
-	}
 
 	// Read handlers and sources from subdirectories of the
 	// directory containing 'path'.
@@ -108,7 +161,7 @@ func ReadConfig(path string) (*Config, error) {
 	}
 	config.HandlerSpecs = handlerSpecs
 
-	sourceSpecs, err := readSourcesFromDir(filepath.Join(dir, "sources"), handlerSpecs)
+	sourceSpecs, err := readSourcesFromDir(filepath.Join(dir, "sources"))
 	if err != nil {
 		logMultiError("warning: source configuration error: ", err)
 		if len(sourceSpecs) == 0 {
@@ -163,12 +216,9 @@ func foreachYAMLFile(dir string, f func(string) error) error {
 // when the configuration changes (there is currently no way to
 // unregister).
 type ConfigManager struct {
-	mx         sync.Mutex
-	config     *Config
-	handlerMap map[string]Handler
-	repo       Repository
-	seed       int64
-	shell      *Shell
+	mx     sync.Mutex
+	config *Config
+	assets *runtimeAssets
 
 	// Listeners are notified on every reload.
 	notifyCh  chan struct{}
@@ -199,41 +249,22 @@ func NewConfigManager(config *Config) (*ConfigManager, error) {
 // Reload the configuration (at least, the parts of it that can be
 // dynamically reloaded).
 func (m *ConfigManager) Reload(config *Config) error {
-	shell := NewShell(config.DryRun)
-	shell.SetNiceLevel(config.DefaultNiceLevel)
-	shell.SetIOClass(config.DefaultIOClass)
-
-	// First, parse the bits that need to be parsed, so that we
-	// can return an error early and config reloads are atomic.
-	handlerMap, err := buildHandlerMap(config.HandlerSpecs, shell)
-	if err != nil {
-		return err
-	}
-
-	repo, err := config.Repository.Parse(shell)
-	if err != nil {
+	assets, err := config.parse()
+	if assets == nil {
 		return err
 	}
 
-	seedFile := defaultSeedFile
-	if config.RandomSeedFile != "" {
-		seedFile = config.RandomSeedFile
-	}
-	seed := mustGetSeed(seedFile)
-
 	// Update config and notify listeners (in a separate
 	// goroutine, that does not hold the lock).
 	m.mx.Lock()
 	defer m.mx.Unlock()
-	if m.repo != nil {
-		m.repo.Close() // nolint
+	if m.assets != nil {
+		m.assets.Close() // nolint
 	}
-	log.Printf("loading new config: %d handlers, %d sources", len(handlerMap), len(config.SourceSpecs))
-	m.repo = repo
-	m.handlerMap = handlerMap
+
+	log.Printf("loaded new config: %d handlers, %d sources", len(assets.handlerMap), len(config.SourceSpecs))
+	m.assets = assets
 	m.config = config
-	m.seed = seed
-	m.shell = shell
 	m.notifyCh <- struct{}{}
 	return nil
 }
@@ -242,8 +273,8 @@ func (m *ConfigManager) Reload(config *Config) error {
 func (m *ConfigManager) Close() {
 	m.mx.Lock()
 	close(m.notifyCh)
-	if m.repo != nil {
-		m.repo.Close() // nolint
+	if m.assets != nil {
+		m.assets.Close()
 	}
 	m.mx.Unlock()
 }
@@ -264,14 +295,14 @@ func (m *ConfigManager) Notify() <-chan struct{} {
 func (m *ConfigManager) getHandler(name string) (Handler, bool) {
 	m.mx.Lock()
 	defer m.mx.Unlock()
-	h, ok := m.handlerMap[name]
+	h, ok := m.assets.handlerMap[name]
 	return h, ok
 }
 
 func (m *ConfigManager) getRepository() Repository {
 	m.mx.Lock()
 	defer m.mx.Unlock()
-	return m.repo
+	return m.assets.repo
 }
 
 func (m *ConfigManager) getQueueSpec() jobs.QueueSpec {
@@ -289,13 +320,19 @@ func (m *ConfigManager) getSourceSpecs() []SourceSpec {
 func (m *ConfigManager) getSeed() int64 {
 	m.mx.Lock()
 	defer m.mx.Unlock()
-	return m.seed
+	return m.assets.seed
 }
 
 func (m *ConfigManager) getShell() *Shell {
 	m.mx.Lock()
 	defer m.mx.Unlock()
-	return m.shell
+	return m.assets.shell
+}
+
+func (m *ConfigManager) getWorkDir() string {
+	m.mx.Lock()
+	defer m.mx.Unlock()
+	return m.config.WorkDir
 }
 
 func mustGetSeed(path string) int64 {
@@ -305,21 +342,9 @@ func mustGetSeed(path string) int64 {
 		}
 	}
 	log.Printf("generating new random seed for this host")
-	seed, data := randomSeed()
+	seed, data := util.RandomSeed()
 	if err := ioutil.WriteFile(path, data, 0600); err != nil {
 		log.Printf("warning: can't write random seed file: %v", err)
 	}
-	return int64(seed)
-}
-
-// Generate a random uint64, and return it along with its byte
-// representation (encoding/binary, little-endian).
-func randomSeed() (uint64, []byte) {
-	// Initialize the seed from a secure source.
-	var b [8]byte
-	if _, err := rand.Read(b[:]); err != nil { // nolint: gosec
-		panic(err)
-	}
-	seed := binary.LittleEndian.Uint64(b[:])
-	return seed, b[:]
+	return seed
 }

handlers.go

@@ -23,6 +23,10 @@ type HandlerSpec struct {
 
 // Parse a HandlerSpec and return a Handler instance.
 func (spec *HandlerSpec) Parse(shell *Shell) (Handler, error) {
+	if spec.Name == "" {
+		return nil, errors.New("name is empty")
+	}
+
 	switch spec.Type {
 	case "file":
 		return newFileHandler(*spec, shell)
@@ -32,25 +36,3 @@ func (spec *HandlerSpec) Parse(shell *Shell) (Handler, error) {
 		return nil, fmt.Errorf("%s: unknown handler type '%s'", spec.Name, spec.Type)
 	}
 }
-
-// Check that the configuration is valid. Not an alternative to
-// validation at usage time, but it provides an early warning to the
-// user.
-func (spec *HandlerSpec) Check() error {
-	if spec.Name == "" {
-		return errors.New("name is empty")
-	}
-	return nil
-}
-
-func buildHandlerMap(specs []HandlerSpec, shell *Shell) (map[string]Handler, error) {
-	m := make(map[string]Handler)
-	for _, spec := range specs {
-		h, err := spec.Parse(shell)
-		if err != nil {
-			return nil, err
-		}
-		m[spec.Name] = h
-	}
-	return m, nil
-}

repository.go

@@ -14,6 +14,10 @@ type RepositorySpec struct {
 
 // Parse a RepositorySpec and return a Repository instance.
 func (spec *RepositorySpec) Parse(shell *Shell) (Repository, error) {
+	if spec.Name == "" {
+		return nil, errors.New("name is empty")
+	}
+
 	switch spec.Type {
 	case "restic":
 		return newResticRepository(spec.Params, shell)
@@ -22,13 +26,3 @@ func (spec *RepositorySpec) Parse(shell *Shell) (Repository, error) {
 		return nil, fmt.Errorf("unknown repository type '%s'", spec.Type)
 	}
 }
-
-// Check that the configuration is valid. Not an alternative to
-// validation at usage time, but it provides an early warning to the
-// user.
-func (spec *RepositorySpec) Check() error {
-	if spec.Name == "" {
-		return errors.New("name is empty")
-	}
-	return nil
-}

source.go

@@ -60,7 +60,7 @@ func (spec *SourceSpec) Parse(ctx context.Context) (ds Dataset, err error) {
 // validation at usage time, but it provides an early warning to the
 // user. Checks the handler name against a string set of handler
 // names.
-func (spec *SourceSpec) Check(handlers map[string]struct{}) error {
+func (spec *SourceSpec) Check(handlers map[string]Handler) error {
 	if spec.Name == "" {
 		return errors.New("name is empty")
 	}