Merge branch 'ticket31241_v3'

changes/ticket31241

@@ -0,0 +1,5 @@
+  o Minor features (configuration validation):
+    - Configuration validation can now be done by per-module callbacks,
+      rather than a global validation function.  This will let us reduce the
+      size of config.c and some of its more cumbersome functions.  Closes
+      ticket 31241.

scripts/maint/practracker/exceptions.txt

@@ -38,7 +38,7 @@ problem include-count /src/app/config/config.c 89
 problem function-size /src/app/config/config.c:options_act_reversible() 296
 problem function-size /src/app/config/config.c:options_act() 589
 problem function-size /src/app/config/config.c:resolve_my_address() 190
-problem function-size /src/app/config/config.c:options_validate() 1209
+problem function-size /src/app/config/config.c:options_validate_cb() 1209
 problem function-size /src/app/config/config.c:options_init_from_torrc() 207
 problem function-size /src/app/config/config.c:options_init_from_string() 113
 problem function-size /src/app/config/config.c:options_init_logs() 145

src/app/config/config.c

@@ -822,10 +822,10 @@ static const config_deprecation_t option_deprecation_notes_[] = {
 #ifdef _WIN32
 static char *get_windows_conf_root(void);
 #endif
-static int options_act_reversible(const or_options_t *old_options, char **msg);
-static int options_transition_allowed(const or_options_t *old,
-                                      const or_options_t *new,
-                                      char **msg);
+
+static int options_check_transition_cb(const void *old,
+                                       const void *new,
+                                       char **msg);
 static int options_transition_affects_workers(
       const or_options_t *old_options, const or_options_t *new_options);
 static int options_transition_affects_descriptor(
@@ -858,25 +858,28 @@ static int options_validate_cb(const void *old_options, void *options,
 static void cleanup_protocol_warning_severity_level(void);
 static void set_protocol_warning_severity_level(int warning_severity);
 static void options_clear_cb(const config_mgr_t *mgr, void *opts);
+static setopt_err_t options_validate_and_set(const or_options_t *old_options,
+                                             or_options_t *new_options,
+                                             char **msg_out);
 
 /** Magic value for or_options_t. */
 #define OR_OPTIONS_MAGIC 9090909
 
 /** Configuration format for or_options_t. */
 static const config_format_t options_format = {
-  sizeof(or_options_t),
-  {
+  .size = sizeof(or_options_t),
+  .magic = {
    "or_options_t",
    OR_OPTIONS_MAGIC,
    offsetof(or_options_t, magic_),
   },
-  option_abbrevs_,
-  option_deprecation_notes_,
-  option_vars_,
-  options_validate_cb,
-  options_clear_cb,
-  NULL,
-  offsetof(or_options_t, subconfigs_),
+  .abbrevs = option_abbrevs_,
+  .deprecations = option_deprecation_notes_,
+  .vars = option_vars_,
+  .legacy_validate_fn = options_validate_cb,
+  .check_transition_fn = options_check_transition_cb,
+  .clear_fn = options_clear_cb,
+  .config_suite_offset = offsetof(or_options_t, subconfigs_),
 };
 
 /*
@@ -918,6 +921,10 @@ get_options_mgr(void)
   return options_mgr;
 }
 
+#define CHECK_OPTIONS_MAGIC(opt) STMT_BEGIN                      \
+    config_check_toplevel_magic(get_options_mgr(), (opt));       \
+  STMT_END
+
 /** Return the contents of our frontpage string, or NULL if not configured. */
 MOCK_IMPL(const char*,
 get_dirportfrontpage, (void))
@@ -1024,6 +1031,7 @@ static void
 options_clear_cb(const config_mgr_t *mgr, void *opts)
 {
   (void)mgr;
+  CHECK_OPTIONS_MAGIC(opts);
   or_options_t *options = opts;
 
   routerset_free(options->ExcludeExitNodesUnion_);
@@ -1425,8 +1433,8 @@ static int have_low_ports = -1;
  *
  * Return 0 if all goes well, return -1 if things went badly.
  */
-static int
-options_act_reversible(const or_options_t *old_options, char **msg)
+MOCK_IMPL(STATIC int,
+options_act_reversible,(const or_options_t *old_options, char **msg))
 {
   smartlist_t *new_listeners = smartlist_new();
   or_options_t *options = get_options_mutable();
@@ -1854,8 +1862,8 @@ options_transition_affects_dirauth_timing(const or_options_t *old_options,
  * Note: We haven't moved all the "act on new configuration" logic
  * here yet.  Some is still in do_hup() and other places.
  */
-STATIC int
-options_act(const or_options_t *old_options)
+MOCK_IMPL(STATIC int,
+options_act,(const or_options_t *old_options))
 {
   config_line_t *cl;
   or_options_t *options = get_options_mutable();
@@ -2685,37 +2693,9 @@ options_trial_assign(config_line_t *list, unsigned flags, char **msg)
     or_options_free(trial_options);
     return r;
   }
+  const or_options_t *cur_options = get_options();
 
-  setopt_err_t rv;
-  or_options_t *cur_options = get_options_mutable();
-
-  in_option_validation = 1;
-
-  if (options_validate(cur_options, trial_options,
-                       msg) < 0) {
-    or_options_free(trial_options);
-    rv = SETOPT_ERR_PARSE; /*XXX make this a separate return value. */
-    goto done;
-  }
-
-  if (options_transition_allowed(cur_options, trial_options, msg) < 0) {
-    or_options_free(trial_options);
-    rv = SETOPT_ERR_TRANSITION;
-    goto done;
-  }
-  in_option_validation = 0;
-
-  if (set_options(trial_options, msg)<0) {
-    or_options_free(trial_options);
-    rv = SETOPT_ERR_SETTING;
-    goto done;
-  }
-
-  /* we liked it. put it in place. */
-  rv = SETOPT_OK;
- done:
-  in_option_validation = 0;
-  return rv;
+  return options_validate_and_set(cur_options, trial_options, msg);
 }
 
 /** Print a usage message for tor. */
@@ -3240,15 +3220,68 @@ compute_publishserverdescriptor(or_options_t *options)
  * */
 #define RECOMMENDED_MIN_CIRCUIT_BUILD_TIMEOUT (10)
 
-static int
-options_validate_cb(const void *old_options, void *options, char **msg)
+/**
+ * Validate <b>new_options</b>.  If it is valid, and it is a reasonable
+ * replacement for <b>old_options</b>, replace the previous value of the
+ * global options, and return return SETOPT_OK.
+ *
+ * If it is not valid, then free <b>new_options</b>, set *<b>msg_out</b> to a
+ * newly allocated error message, and return an error code.
+ */
+static setopt_err_t
+options_validate_and_set(const or_options_t *old_options,
+                         or_options_t *new_options,
+                         char **msg_out)
 {
+  setopt_err_t rv;
+  validation_status_t vs;
+
   in_option_validation = 1;
-  int rv = options_validate(old_options, options, msg);
+  vs = config_validate(get_options_mgr(), old_options, new_options, msg_out);
+
+  if (vs == VSTAT_TRANSITION_ERR) {
+    rv = SETOPT_ERR_TRANSITION;
+    goto err;
+  } else if (vs < 0) {
+    rv = SETOPT_ERR_PARSE;
+    goto err;
+  }
   in_option_validation = 0;
+
+  if (set_options(new_options, msg_out)) {
+    rv = SETOPT_ERR_SETTING;
+    goto err;
+  }
+
+  rv = SETOPT_OK;
+  new_options = NULL; /* prevent free */
+ err:
+  in_option_validation = 0;
+  tor_assert(new_options == NULL || rv != SETOPT_OK);
+  or_options_free(new_options);
   return rv;
 }
 
+#ifdef TOR_UNIT_TESTS
+/**
+ * Return 0 if every setting in <b>options</b> is reasonable, is a
+ * permissible transition from <b>old_options</b>, and none of the
+ * testing-only settings differ from <b>default_options</b> unless in
+ * testing mode.  Else return -1.  Should have no side effects, except for
+ * normalizing the contents of <b>options</b>.
+ *
+ * On error, tor_strdup an error explanation into *<b>msg</b>.
+ */
+int
+options_validate(const or_options_t *old_options, or_options_t *options,
+                 char **msg)
+{
+  validation_status_t vs;
+  vs = config_validate(get_options_mgr(), old_options, options, msg);
+  return vs < 0 ? -1 : 0;
+}
+#endif
+
 #define REJECT(arg) \
   STMT_BEGIN *msg = tor_strdup(arg); return -1; STMT_END
 #if defined(__GNUC__) && __GNUC__ <= 3
@@ -3432,18 +3465,19 @@ options_validate_single_onion(or_options_t *options, char **msg)
   return 0;
 }
 
-/** Return 0 if every setting in <b>options</b> is reasonable, is a
- * permissible transition from <b>old_options</b>, and none of the
- * testing-only settings differ from <b>default_options</b> unless in
- * testing mode.  Else return -1.  Should have no side effects, except for
- * normalizing the contents of <b>options</b>.
- *
- * On error, tor_strdup an error explanation into *<b>msg</b>.
+/**
+ * Legacy validation/normalization callback for or_options_t.  See
+ * legacy_validate_fn_t for more information.
  */
-STATIC int
-options_validate(const or_options_t *old_options, or_options_t *options,
-                 char **msg)
+static int
+options_validate_cb(const void *old_options_, void *options_, char **msg)
 {
+  if (old_options_)
+    CHECK_OPTIONS_MAGIC(old_options_);
+  CHECK_OPTIONS_MAGIC(options_);
+  const or_options_t *old_options = old_options_;
+  or_options_t *options = options_;
+
   config_line_t *cl;
   const char *uname = get_uname();
   int n_ports=0;
@@ -4791,11 +4825,17 @@ opt_streq(const char *s1, const char *s2)
 
 /** Check if any of the previous options have changed but aren't allowed to. */
 static int
-options_transition_allowed(const or_options_t *old,
-                           const or_options_t *new_val,
-                           char **msg)
+options_check_transition_cb(const void *old_,
+                            const void *new_val_,
+                            char **msg)
 {
-  if (!old)
+  CHECK_OPTIONS_MAGIC(old_);
+  CHECK_OPTIONS_MAGIC(new_val_);
+
+  const or_options_t *old = old_;
+  const or_options_t *new_val = new_val_;
+
+  if (BUG(!old))
     return 0;
 
 #define BAD_CHANGE_TO(opt, how) do {                                    \
@@ -5523,25 +5563,12 @@ options_init_from_string(const char *cf_defaults, const char *cf,
   }
 
   newoptions->IncludeUsed = cf_has_include;
-  in_option_validation = 1;
   newoptions->FilesOpenedByIncludes = opened_files;
+  opened_files = NULL; // prevent double-free.
 
-  /* Validate newoptions */
-  if (options_validate(oldoptions, newoptions, msg) < 0) {
-    err = SETOPT_ERR_PARSE; /*XXX make this a separate return value.*/
+  err = options_validate_and_set(oldoptions, newoptions, msg);
+  if (err < 0)
     goto err;
-  }
-
-  if (options_transition_allowed(oldoptions, newoptions, msg) < 0) {
-    err = SETOPT_ERR_TRANSITION;
-    goto err;
-  }
-  in_option_validation = 0;
-
-  if (set_options(newoptions, msg)) {
-    err = SETOPT_ERR_SETTING;
-    goto err; /* frees and replaces old options */
-  }
 
   or_options_free(global_default_options);
   global_default_options = newdefaultoptions;
@@ -5554,9 +5581,6 @@ options_init_from_string(const char *cf_defaults, const char *cf,
     SMARTLIST_FOREACH(opened_files, char *, f, tor_free(f));
     smartlist_free(opened_files);
   }
-  // may have been set to opened_files, avoid double free
-  newoptions->FilesOpenedByIncludes = NULL;
-  or_options_free(newoptions);
   or_options_free(newdefaultoptions);
   if (*msg) {
     char *old_msg = *msg;

src/app/config/config.h

@@ -264,7 +264,9 @@ int options_any_client_port_set(const or_options_t *options);
 #define CL_PORT_IS_UNIXSOCKET (1u<<6)
 #define CL_PORT_DFLT_GROUP_WRITABLE (1u<<7)
 
-STATIC int options_act(const or_options_t *old_options);
+MOCK_DECL(STATIC int, options_act,(const or_options_t *old_options));
+MOCK_DECL(STATIC int, options_act_reversible,(const or_options_t *old_options,
+                                             char **msg));
 struct config_mgr_t;
 STATIC const struct config_mgr_t *get_options_mgr(void);
 
@@ -277,9 +279,6 @@ STATIC void port_cfg_free_(port_cfg_t *port);
 STATIC void or_options_free_(or_options_t *options);
 STATIC int options_validate_single_onion(or_options_t *options,
                                          char **msg);
-STATIC int options_validate(const or_options_t *old_options,
-                            or_options_t *options,
-                            char **msg);
 STATIC int parse_transport_line(const or_options_t *options,
                                 const char *line, int validate_only,
                                 int server);
@@ -311,6 +310,12 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity,
 STATIC int options_init_logs(const or_options_t *old_options,
                              or_options_t *options, int validate_only);
 
+#ifdef TOR_UNIT_TESTS
+int options_validate(const or_options_t *old_options,
+                     or_options_t *options,
+                     char **msg);
+#endif
+
 #endif /* defined(CONFIG_PRIVATE) */
 
 #endif /* !defined(TOR_CONFIG_H) */

src/app/config/statefile.c

@@ -157,19 +157,17 @@ static struct_member_t state_extra_var = {
 
 /** Configuration format for or_state_t. */
 static const config_format_t state_format = {
-  sizeof(or_state_t),
-  {
+  .size = sizeof(or_state_t),
+  .magic = {
    "or_state_t",
    OR_STATE_MAGIC,
    offsetof(or_state_t, magic_),
   },
-  state_abbrevs_,
-  NULL,
-  state_vars_,
-  or_state_validate_cb,
-  NULL,
-  &state_extra_var,
-  offsetof(or_state_t, substates_),
+  .abbrevs = state_abbrevs_,
+  .vars = state_vars_,
+  .legacy_validate_fn = or_state_validate_cb,
+  .extra = &state_extra_var,
+  .config_suite_offset = offsetof(or_state_t, substates_),
 };
 
 /* A global configuration manager for state-file objects */
@@ -186,6 +184,10 @@ get_state_mgr(void)
   return state_mgr;
 }
 
+#define CHECK_STATE_MAGIC(s) STMT_BEGIN                        \
+    config_check_toplevel_magic(get_state_mgr(), (s));         \
+  STMT_END
+
 /** Persistent serialized state. */
 static or_state_t *global_state = NULL;
 
@@ -267,16 +269,6 @@ validate_transports_in_state(or_state_t *state)
   return 0;
 }
 
-static int
-or_state_validate_cb(const void *old_state, void *state, char **msg)
-{
-  /* We don't use these; only options do. Still, we need to match that
-   * signature. */
-  (void) old_state;
-
-  return or_state_validate(state, msg);
-}
-
 /** Return 0 if every setting in <b>state</b> is reasonable, and a
  * permissible transition from <b>old_state</b>.  Else warn and return -1.
  * Should have no side effects, except for normalizing the contents of
@@ -285,6 +277,23 @@ or_state_validate_cb(const void *old_state, void *state, char **msg)
 static int
 or_state_validate(or_state_t *state, char **msg)
 {
+  return config_validate(get_state_mgr(), NULL, state, msg);
+}
+
+/**
+ * Legacy validation/normalization callback for or_state_t.  See
+ * legacy_validate_fn_t for more information.
+ */
+static int
+or_state_validate_cb(const void *old_state, void *state_, char **msg)
+{
+  /* There is not a meaningful concept of a state-to-state transition,
+   * since we do not reload the state after we start. */
+  (void) old_state;
+  CHECK_STATE_MAGIC(state_);
+
+  or_state_t *state = state_;
+
   if (entry_guards_parse_state(state, 0, msg)<0)
     return -1;
 

src/feature/dirauth/shared_random_state.c

@@ -59,9 +59,6 @@ DUMMY_TYPECHECK_INSTANCE(sr_disk_state_t);
 /** Our persistent state magic number. */
 #define SR_DISK_STATE_MAGIC 0x98AB1254
 
-static int
-disk_state_validate_cb(const void *old_state, void *state, char **msg);
-
 /** Array of variables that are saved to disk as a persistent state. */
 static const config_var_t state_vars[] = {
   V(Version,                    POSINT, "0"),
@@ -87,19 +84,15 @@ static const struct_member_t state_extra_var = {
 
 /** Configuration format of sr_disk_state_t. */
 static const config_format_t state_format = {
-  sizeof(sr_disk_state_t),
-  {
+  .size = sizeof(sr_disk_state_t),
+  .magic = {
    "sr_disk_state_t",
    SR_DISK_STATE_MAGIC,
    offsetof(sr_disk_state_t, magic_),
   },
-  NULL,
-  NULL,
-  state_vars,
-  disk_state_validate_cb,
-  NULL,
-  &state_extra_var,
-  -1,
+  .vars = state_vars,
+  .extra = &state_extra_var,
+  .config_suite_offset = -1,
 };
 
 /** Global configuration manager for the shared-random state file */
@@ -342,21 +335,6 @@ disk_state_validate(const sr_disk_state_t *state)
   return -1;
 }
 
-/** Validate the disk state (NOP for now). */
-static int
-disk_state_validate_cb(const void *old_state, void *state, char **msg)
-{
-  /* We don't use these; only options do. */
-  (void) old_state;
-
-  /* This is called by config_dump which is just before we are about to
-   * write it to disk. At that point, our global memory state has been
-   * copied to the disk state so it's fair to assume it's trustable. */
-  (void) state;
-  (void) msg;
-  return 0;
-}
-
 /** Parse the Commit line(s) in the disk state and translate them to the
  * the memory state. Return 0 on success else -1 on error. */
 static int

src/lib/confmgt/confparse.c

@@ -334,6 +334,17 @@ config_mgr_list_deprecated_vars(const config_mgr_t *mgr)
   return result;
 }
 
+/**
+ * Check the magic number on <b>object</b> to make sure it's a valid toplevel
+ * object, created with <b>mgr</b>.  Exit with an assertion if it isn't.
+ **/
+void
+config_check_toplevel_magic(const config_mgr_t *mgr,
+                            const void *object)
+{
+  struct_check_magic(object, &mgr->toplevel_magic);
+}
+
 /** Assert that the magic fields in <b>options</b> and its subsidiary
  * objects are all okay. */
 static void
@@ -1142,6 +1153,105 @@ config_init(const config_mgr_t *mgr, void *options)
   } SMARTLIST_FOREACH_END(mv);
 }
 
+/**
+ * Normalize and validate a single object `options` within a configuration
+ * suite, according to its format.  `options` may be modified as appropriate
+ * in order to set ancillary data.  If `old_options` is provided, make sure
+ * that the transition from `old_options` to `options` is permitted.
+ *
+ * On success return VSTAT_OK; on failure set *msg_out to a newly allocated
+ * string explaining what is wrong, and return a different validation_status_t
+ * to describe which step failed.
+ **/
+static validation_status_t
+config_validate_single(const config_format_t *fmt,
+                       const void *old_options, void *options,
+                       char **msg_out)
+{
+  tor_assert(fmt);
+  tor_assert(options);
+
+  if (fmt->pre_normalize_fn) {
+    if (fmt->pre_normalize_fn(options, msg_out) < 0) {
+      return VSTAT_PRE_NORMALIZE_ERR;
+    }
+  }
+
+  if (fmt->legacy_validate_fn) {
+    if (fmt->legacy_validate_fn(old_options, options, msg_out) < 0) {
+      return VSTAT_LEGACY_ERR;
+    }
+  }
+
+  if (fmt->validate_fn) {
+    if (fmt->validate_fn(options, msg_out) < 0) {
+      return VSTAT_VALIDATE_ERR;
+    }
+  }
+
+  if (fmt->check_transition_fn && old_options) {
+    if (fmt->check_transition_fn(old_options, options, msg_out) < 0) {
+      return VSTAT_TRANSITION_ERR;
+    }
+  }
+
+  if (fmt->post_normalize_fn) {
+    if (fmt->post_normalize_fn(options, msg_out) < 0) {
+      return VSTAT_POST_NORMALIZE_ERR;
+    }
+  }
+
+  return VSTAT_OK;
+}
+
+/**
+ * Normalize and validate all the options in configuration object `options`
+ * and its sub-objects. `options` may be modified as appropriate in order to
+ * set ancillary data.  If `old_options` is provided, make sure that the
+ * transition from `old_options` to `options` is permitted.
+ *
+ * On success return VSTAT_OK; on failure set *msg_out to a newly allocated
+ * string explaining what is wrong, and return a different validation_status_t
+ * to describe which step failed.
+ **/
+validation_status_t
+config_validate(const config_mgr_t *mgr,
+                const void *old_options, void *options,
+                char **msg_out)
+{
+  validation_status_t rv;
+  CONFIG_CHECK(mgr, options);
+  if (old_options) {
+    CONFIG_CHECK(mgr, old_options);
+  }
+
+  config_suite_t **suitep_new = config_mgr_get_suite_ptr(mgr, options);
+  config_suite_t **suitep_old = NULL;
+  if (old_options)
+    suitep_old = config_mgr_get_suite_ptr(mgr, (void*) old_options);
+
+  /* Validate the sub-objects */
+  if (suitep_new) {
+    SMARTLIST_FOREACH_BEGIN(mgr->subconfigs, const config_format_t *, fmt) {
+      void *obj = smartlist_get((*suitep_new)->configs, fmt_sl_idx);
+      const void *obj_old=NULL;
+      if (suitep_old)
+        obj_old = smartlist_get((*suitep_old)->configs, fmt_sl_idx);
+
+      rv = config_validate_single(fmt, obj_old, obj, msg_out);
+      if (rv < 0)
+        return rv;
+    } SMARTLIST_FOREACH_END(fmt);
+  }
+
+  /* Validate the top-level object. */
+  rv = config_validate_single(mgr->toplevel, old_options, options, msg_out);
+  if (rv < 0)
+    return rv;
+
+  return VSTAT_OK;
+}
+
 /** Allocate and return a new string holding the written-out values of the vars
  * in 'options'.  If 'minimal', do not write out any default-valued vars.
  * Else, if comment_defaults, write default values as comments.
@@ -1166,7 +1276,7 @@ config_dump(const config_mgr_t *mgr, const void *default_options,
 
   /* XXX use a 1 here so we don't add a new log line while dumping */
   if (default_options == NULL) {
-    if (fmt->validate_fn(NULL, defaults_tmp, &msg) < 0) {
+    if (config_validate(mgr, NULL, defaults_tmp, &msg) < 0) {
       // LCOV_EXCL_START
       log_err(LD_BUG, "Failed to validate default config: %s", msg);
       tor_free(msg);

src/lib/confmgt/confparse.h

@@ -52,25 +52,59 @@ typedef struct config_deprecation_t {
 #define PLURAL(tok) { #tok, #tok "s", 0, 0 }
 
 /**
- * Type of a callback to validate whether a given configuration is
+ * Validation function: verify whether a configuation object is well-formed
+ * and consistent.
+ *
+ * On success, return 0.  On failure, set <b>msg_out</b> to a newly allocated
+ * string containing an error message, and return -1. */
+typedef int (*validate_fn_t)(const void *value, char **msg_out);
+/**
+ * Validation function: verify whether a configuration object (`value`) is an
+ * allowable value given the previous configuration value (`old_value`).
+ *
+ * On success, return 0.  On failure, set <b>msg_out</b> to a newly allocated
+ * string containing an error message, and return -1. */
+typedef int (*check_transition_fn_t)(const void *old_value, const void *value,
+                                     char **msg_out);
+/**
+ * Validation function: normalize members of `value`, and compute derived
+ * members.
+ *
+ * This function is called before any other validation of `value`, and must
+ * not assume that validate_fn or check_transition_fn has passed.
+ *
+ * On success, return 0.  On failure, set <b>msg_out</b> to a newly allocated
+ * string containing an error message, and return -1. */
+typedef int (*pre_normalize_fn_t)(void *value, char **msg_out);
+/**
+ * Validation function: normalize members of `value`, and compute derived
+ * members.
+ *
+ * This function is called after validation of `value`, and may
+ * assume that validate_fn or check_transition_fn has passed.
+ *
+ * On success, return 0.  On failure, set <b>msg_out</b> to a newly allocated
+ * string containing an error message, and return -1. */
+typedef int (*post_normalize_fn_t)(void *value, char **msg_out);
+
+/**
+ * Legacy function to validate whether a given configuration is
  * well-formed and consistent.
  *
  * The configuration to validate is passed as <b>newval</b>. The previous
- * configuration, if any, is provided in <b>oldval</b>.  The
- * <b>default_val</b> argument receives a configuration object initialized
- * with default values for all its fields.  The <b>from_setconf</b> argument
- * is true iff the input comes from a SETCONF controller command.
+ * configuration, if any, is provided in <b>oldval</b>.
+ *
+ * This API is deprecated, since it mixes the responsibilities of
+ * pre_normalize_fn_t, post_normalize_fn_t, validate_fn_t, and
+ * check_transition_fn_t.  No new instances of this function type should
+ * be written.
  *
  * On success, return 0.  On failure, set *<b>msg_out</b> to a newly allocated
  * error message, and return -1.
- *
- * REFACTORING NOTE: Currently, this callback type is only used from inside
- * config_dump(); later in our refactoring, it will be cleaned up and used
- * more generally.
  */
-typedef int (*validate_fn_t)(const void *oldval,
-                             void *newval,
-                             char **msg_out);
+typedef int (*legacy_validate_fn_t)(const void *oldval,
+                                    void *newval,
+                                    char **msg_out);
 
 struct config_mgr_t;
 
@@ -98,7 +132,18 @@ typedef struct config_format_t {
   const config_var_t *vars; /**< List of variables we recognize, their default
                              * values, and where we stick them in the
                              * structure. */
-  validate_fn_t validate_fn; /**< Function to validate config. */
+
+  /** Early-stage normalization callback. Invoked by config_validate(). */
+  pre_normalize_fn_t pre_normalize_fn;
+  /** Configuration validation function. Invoked by config_validate(). */
+  validate_fn_t validate_fn;
+    /** Legacy validation function. Invoked by config_validate(). */
+  legacy_validate_fn_t legacy_validate_fn;
+  /** Transition checking function. Invoked by config_validate(). */
+  check_transition_fn_t check_transition_fn;
+  /** Late-stage normalization callback. Invoked by config_validate(). */
+  post_normalize_fn_t post_normalize_fn;
+
   clear_cfg_fn_t clear_fn; /**< Function to clear the configuration. */
   /** If present, extra denotes a LINELIST variable for unrecognized
    * lines.  Otherwise, unrecognized lines are an error. */
@@ -169,10 +214,26 @@ int config_is_same(const config_mgr_t *fmt,
 struct config_line_t *config_get_changes(const config_mgr_t *mgr,
                                   const void *options1, const void *options2);
 void config_init(const config_mgr_t *mgr, void *options);
+
+/** An enumeration to report which validation step failed. */
+typedef enum {
+  VSTAT_PRE_NORMALIZE_ERR = -5,
+  VSTAT_VALIDATE_ERR = -4,
+  VSTAT_LEGACY_ERR = -3,
+  VSTAT_TRANSITION_ERR = -2,
+  VSTAT_POST_NORMALIZE_ERR = -1,
+  VSTAT_OK = 0,
+} validation_status_t;
+
+validation_status_t config_validate(const config_mgr_t *mgr,
+                                    const void *old_options, void *options,
+                                    char **msg_out);
 void *config_dup(const config_mgr_t *mgr, const void *old);
 char *config_dump(const config_mgr_t *mgr, const void *default_options,
                   const void *options, int minimal,
                   int comment_defaults);
+void config_check_toplevel_magic(const config_mgr_t *mgr,
+                                 const void *object);
 bool config_check_ok(const config_mgr_t *mgr, const void *options,
                      int severity);
 int config_assign(const config_mgr_t *mgr, void *options,

src/test/test_confmgr.c

@@ -43,6 +43,8 @@ typedef struct {
   int fuzziness;
   char *alpacaname;
   int n_wings; /* deprecated; alpacas don't have wings. */
+
+  int square_fuzziness; /* Derived from fuzziness. */
 } alpaca_cfg_t;
 
 /*
@@ -105,6 +107,84 @@ static config_abbrev_t llama_abbrevs[] = {
   { NULL, NULL, 0, 0 },
 };
 
+static int
+legacy_validate_pasture(const void *old_, void *obj, char **msg_out)
+{
+  const pasture_cfg_t *old = old_;
+  pasture_cfg_t *p = obj;
+
+  // llamas can't find their way home if the letters are lowercase.
+  if (p->address)
+    tor_strupper(p->address);
+
+  if (old && old->address &&
+      (!p->address || strcmp(old->address, p->address))) {
+    *msg_out = tor_strdup("You can't move a pasture.");
+    return -1;
+  }
+
+  return 0;
+}
+
+static int
+validate_llama(const void *obj, char **msg_out)
+{
+  const llama_cfg_t *llama = obj;
+  tor_assert(llama->magic == 0x11aa11);
+
+  if (! llama->llamaname || strlen(llama->llamaname) == 0) {
+    *msg_out = tor_strdup("A llama has no name!?");
+    return -1;
+  }
+
+  if (strspn(llama->llamaname, "0123456789") == strlen(llama->llamaname)) {
+    *msg_out = tor_strdup("It is not a number; it is a free llama!");
+    return -1;
+  }
+
+  return 0;
+}
+
+static int
+check_transition_alpaca(const void *old_, const void *new_, char **msg_out)
+{
+  const alpaca_cfg_t *old_alpaca = old_;
+  const alpaca_cfg_t *new_alpaca = new_;
+
+  tor_assert(old_alpaca && new_alpaca);
+  tor_assert(old_alpaca->magic == 0xa15aca);
+  tor_assert(new_alpaca->magic == 0xa15aca);
+
+  if (old_alpaca->fuzziness > new_alpaca->fuzziness) {
+    *msg_out = tor_strdup("An alpaca only becomes more fuzzy over time.");
+    return -1;
+  }
+
+  return 0;
+}
+
+static int
+post_normalize_llama(void *obj, char **msg_out)
+{
+  (void)msg_out;
+  llama_cfg_t *llama = obj;
+  tor_assert(llama->magic == 0x11aa11);
+  tor_assert(llama->llamaname); // we have already checked for a NULL name.
+  tor_free(llama->description);
+  tor_asprintf(&llama->description, "A llama called %s.", llama->llamaname);
+  return 0;
+}
+
+static int
+pre_normalize_alpaca(void *obj, char **msg_out)
+{
+  (void)msg_out;
+  alpaca_cfg_t *alpaca = obj;
+  tor_assert(alpaca->magic == 0xa15aca);
+  alpaca->square_fuzziness = alpaca->fuzziness * alpaca->fuzziness;
+  return 0;
+}
+
 static const config_format_t pasture_fmt = {
   sizeof(pasture_cfg_t),
   {
@@ -114,6 +194,7 @@ static const config_format_t pasture_fmt = {
   },
   .vars = pasture_vars,
   .config_suite_offset = offsetof(pasture_cfg_t, subobjs),
+  .legacy_validate_fn = legacy_validate_pasture,
 };
 
 static const config_format_t llama_fmt = {
@@ -128,6 +209,8 @@ static const config_format_t llama_fmt = {
   .deprecations = llama_deprecations,
   .abbrevs = llama_abbrevs,
   .clear_fn = clear_llama_cfg,
+  .validate_fn = validate_llama,
+  .post_normalize_fn = post_normalize_llama,
 };
 
 static const config_format_t alpaca_fmt = {
@@ -140,6 +223,8 @@ static const config_format_t alpaca_fmt = {
   .vars = alpaca_vars,
   .config_suite_offset = -1,
   .deprecations = alpaca_deprecations,
+  .pre_normalize_fn = pre_normalize_alpaca,
+  .check_transition_fn = check_transition_alpaca,
 };
 
 #define LLAMA_IDX 0
@@ -313,6 +398,95 @@ test_confmgr_dump(void *arg)
   tor_free(s);
 }
 
+static pasture_cfg_t *
+parse_and_validate(config_mgr_t *mgr,
+                   const char *inp, const pasture_cfg_t *old, char **msg_out)
+{
+  pasture_cfg_t *p = config_new(mgr);
+  pasture_cfg_t *result = NULL;
+  config_line_t *lines = NULL;
+
+  config_init(mgr, p); // set defaults.
+  int r = config_get_lines(inp, &lines, 0);
+  tt_int_op(r, OP_EQ, 0);
+  r = config_assign(mgr, p, lines, 0, msg_out);
+  tt_int_op(r, OP_EQ, 0);
+  tor_free(*msg_out); // sets it to NULL
+  r = config_validate(mgr, old, p, msg_out);
+  if (r < 0)
+    goto done;
+
+  tt_ptr_op(*msg_out, OP_EQ, NULL);
+  result = p;
+  p = NULL; // prevent free
+ done:
+  config_free(mgr, p);
+  config_free_lines(lines);
+  return result;
+}
+
+static void
+test_confmgr_validate(void *arg)
+{
+  (void)arg;
+  char *msg = NULL;
+  config_mgr_t *mgr = get_mgr(true);
+  pasture_cfg_t *p_orig, *p=NULL;
+
+  p_orig = parse_and_validate(mgr, "Llamaname Quest\n"
+                                   "Address 99 camelid way\n"
+                                   "Fuzziness 8\n", NULL, &msg);
+  tt_assert(p_orig);
+
+  // Make sure normalization code was run.
+  const alpaca_cfg_t *ac0 = config_mgr_get_obj(mgr, p_orig, ALPACA_IDX);
+  const llama_cfg_t *lc0 = config_mgr_get_obj(mgr, p_orig, LLAMA_IDX);
+  tt_int_op(ac0->fuzziness, OP_EQ, 8);
+  tt_int_op(ac0->square_fuzziness, OP_EQ, 64);
+  tt_str_op(lc0->description, OP_EQ, "A llama called Quest.");
+  tt_str_op(p_orig->address, OP_EQ, "99 CAMELID WAY");
+
+  // try a bad llamaname.
+  p = parse_and_validate(mgr, "llamaname 123", p_orig, &msg);
+  tt_assert(!p);
+  tt_str_op(msg, OP_EQ, "It is not a number; it is a free llama!");
+  tor_free(msg);
+
+  // try a llamaname that would crash the post_normalize step, if it ran.
+  p = parse_and_validate(mgr, "", p_orig, &msg);
+  tt_assert(!p);
+  tt_str_op(msg, OP_EQ, "A llama has no name!?");
+  tor_free(msg);
+
+  // Verify that a transition to a less fuzzy alpaca fails.
+  p = parse_and_validate(mgr, "Llamaname Quest\n"
+                              "Address 99 camelid way\n"
+                              "Fuzziness 4\n", p_orig, &msg);
+  tt_assert(!p);
+  tt_str_op(msg, OP_EQ, "An alpaca only becomes more fuzzy over time.");
+  tor_free(msg);
+
+  // Try a transition to a more fuzzy alpaca; it should work fine.
+  p = parse_and_validate(mgr, "Llamaname Mercutio\n"
+                              // the default fuzziness is 50
+                              "Address 99 camelid way\n", p_orig, &msg);
+  tt_assert(p);
+  config_free(mgr, p);
+
+  // Verify that we can't move the pasture.
+  p = parse_and_validate(mgr, "Llamaname Montague\n"
+                              // the default fuzziness is 50
+                              "Address 99 ungulate st\n", p_orig, &msg);
+  tt_assert(!p);
+  tt_str_op(msg, OP_EQ, "You can't move a pasture.");
+
+ done:
+  config_free(mgr, p);
+  config_free(mgr, p_orig);
+  config_mgr_free(mgr);
+  tor_free(msg);
+}
+
 #define CONFMGR_TEST(name, flags)                       \
   { #name, test_confmgr_ ## name, flags, NULL, NULL }
 
@@ -321,5 +495,6 @@ struct testcase_t confmgr_tests[] = {
   CONFMGR_TEST(magic, 0),
   CONFMGR_TEST(parse, 0),
   CONFMGR_TEST(dump, 0),
+  CONFMGR_TEST(validate, 0),
   END_OF_TESTCASES
 };

src/test/test_confparse.c

@@ -119,19 +119,17 @@ test_validate_cb(const void *old_options, void *options, char **msg)
 #define TEST_MAGIC 0x1337
 
 static const config_format_t test_fmt = {
-  sizeof(test_struct_t),
-  {
+  .size = sizeof(test_struct_t),
+  .magic = {
    "test_struct_t",
    TEST_MAGIC,
    offsetof(test_struct_t, magic),
   },
-  test_abbrevs,
-  test_deprecation_notes,
-  test_vars,
-  test_validate_cb,
-  NULL,
-  NULL,
-  -1,
+  .abbrevs = test_abbrevs,
+  .deprecations = test_deprecation_notes,
+  .vars = test_vars,
+  .legacy_validate_fn = test_validate_cb,
+  .config_suite_offset = -1,
 };
 
 /* Make sure that config_init sets everything to the right defaults. */
@@ -815,19 +813,18 @@ static struct_member_t extra = {
 };
 
 static config_format_t etest_fmt = {
-  sizeof(test_struct_t),
-  {
+  .size = sizeof(test_struct_t),
+  .magic = {
    "test_struct_t (with extra lines)",
    ETEST_MAGIC,
    offsetof(test_struct_t, magic),
   },
-  test_abbrevs,
-  test_deprecation_notes,
-  test_vars,
-  test_validate_cb,
-  NULL,
-  &extra,
-  -1,
+  .abbrevs = test_abbrevs,
+  .deprecations = test_deprecation_notes,
+  .vars = test_vars,
+  .legacy_validate_fn = test_validate_cb,
+  .extra = &extra,
+  .config_suite_offset = -1,
 };
 
 /* Try out the feature where we can store unrecognized lines and dump them

src/test/testing_common.c

@@ -326,6 +326,7 @@ main(int c, const char **v)
   initialize_mainloop_events();
   options_init(options);
   options->DataDirectory = tor_strdup(temp_dir);
+  options->DataDirectory_option = tor_strdup(temp_dir);
   tor_asprintf(&options->KeyDirectory, "%s"PATH_SEPARATOR"keys",
                options->DataDirectory);
   options->CacheDirectory = tor_strdup(temp_dir);