Merge branch 'tor-github/pr/1296'

changes/ticket31625

@@ -0,0 +1,4 @@
+  o Code simplification and refactoring:
+    - Replace our ad-hoc set of flags for configuration variables and
+      configuration variable types with fine-grained orthogonal flags
+      corresponding to the actual behavior we want. Closes ticket 31625.

src/app/config/config.c

@@ -267,10 +267,10 @@ DUMMY_TYPECHECK_INSTANCE(or_options_t);
 
 #define VAR_NODUMP(varname,conftype,member,initvalue)             \
   CONFIG_VAR_ETYPE(or_options_t, varname, conftype, member,       \
-                   CVFLAG_NODUMP, initvalue)
+                   CFLG_NODUMP, initvalue)
 #define VAR_INVIS(varname,conftype,member,initvalue)              \
   CONFIG_VAR_ETYPE(or_options_t, varname, conftype, member,       \
-                   CVFLAG_NODUMP|CVFLAG_INVISIBLE, initvalue)
+                   CFLG_NODUMP | CFLG_NOSET | CFLG_NOLIST, initvalue)
 
 #define V(member,conftype,initvalue)            \
   VAR(#member, conftype, member, initvalue)
@@ -2671,6 +2671,9 @@ list_torrc_options(void)
 {
   smartlist_t *vars = config_mgr_list_vars(get_options_mgr());
   SMARTLIST_FOREACH_BEGIN(vars, const config_var_t *, var) {
+    /* Possibly this should check listable, rather than (or in addition to)
+     * settable. See ticket 31654.
+     */
     if (! config_var_is_settable(var)) {
       /* This variable cannot be set, or cannot be set by this name. */
       continue;
@@ -2685,6 +2688,8 @@ static void
 list_deprecated_options(void)
 {
   smartlist_t *deps = config_mgr_list_deprecated_vars(get_options_mgr());
+  /* Possibly this should check whether the variables are listable,
+   * but currently it does not.  See ticket 31654. */
   SMARTLIST_FOREACH(deps, const char *, name,
                     printf("%s\n", name));
   smartlist_free(deps);
@@ -8133,7 +8138,7 @@ getinfo_helper_config(control_connection_t *conn,
     smartlist_t *vars = config_mgr_list_vars(get_options_mgr());
     SMARTLIST_FOREACH_BEGIN(vars, const config_var_t *, var) {
       /* don't tell controller about invisible options */
-      if (config_var_is_invisible(var))
+      if (! config_var_is_listable(var))
         continue;
       const char *type = struct_var_get_typename(&var->member);
       if (!type)
@@ -8147,6 +8152,8 @@ getinfo_helper_config(control_connection_t *conn,
   } else if (!strcmp(question, "config/defaults")) {
     smartlist_t *sl = smartlist_new();
     int dirauth_lines_seen = 0, fallback_lines_seen = 0;
+    /* Possibly this should check whether the variables are listable,
+     * but currently it does not.  See ticket 31654. */
     smartlist_t *vars = config_mgr_list_vars(get_options_mgr());
     SMARTLIST_FOREACH_BEGIN(vars, const config_var_t *, var) {
       if (var->initvalue != NULL) {

src/app/config/confparse.c

@@ -511,32 +511,112 @@ config_count_options(const config_mgr_t *mgr)
   return smartlist_len(mgr->all_vars);
 }
 
-bool
-config_var_is_cumulative(const config_var_t *var)
+/**
+ * Return true iff at least one bit from <b>flag</b> is set on <b>var</b>,
+ * either in <b>var</b>'s flags, or on the flags of its type.
+ **/
+static bool
+config_var_has_flag(const config_var_t *var, uint32_t flag)
 {
-  return struct_var_is_cumulative(&var->member);
+  uint32_t have_flags = var->flags | struct_var_get_flags(&var->member);
+
+  return (have_flags & flag) != 0;
 }
+
+/**
+ * Return true if assigning a value to <b>var</b> replaces the previous
+ * value.  Return false if assigning a value to <b>var</b> appends
+ * to the previous value.
+ **/
+static bool
+config_var_is_replaced_on_set(const config_var_t *var)
+{
+  return ! config_var_has_flag(var, CFLG_NOREPLACE);
+}
+
+/**
+ * Return true iff <b>var</b> may be assigned by name (e.g., via the
+ * CLI, the configuration files, or the controller API).
+ **/
 bool
 config_var_is_settable(const config_var_t *var)
 {
-  if (var->flags & CVFLAG_OBSOLETE)
-    return false;
-  return struct_var_is_settable(&var->member);
+  return ! config_var_has_flag(var, CFLG_NOSET);
 }
-bool
-config_var_is_contained(const config_var_t *var)
+
+/**
+ * Return true iff the controller is allowed to fetch the value of
+ * <b>var</b>.
+ **/
+static bool
+config_var_is_gettable(const config_var_t *var)
 {
-  return struct_var_is_contained(&var->member);
+  /* Arguably, invisible or obsolete options should not be gettable.  However,
+   * they have been gettable for a long time, and making them ungettable could
+   * have compatibility effects.  For now, let's leave them alone.
+   */
+
+  // return ! config_var_has_flag(var, CVFLAG_OBSOLETE|CFGLAGS_INVISIBLE);
+  (void)var;
+  return true;
 }
-bool
-config_var_is_invisible(const config_var_t *var)
+
+/**
+ * Return true iff we need to check <b>var</b> for changes when we are
+ * comparing config options for changes.
+ *
+ * A false result might mean that the variable is a derived variable, and that
+ * comparing the variable it derives from compares this one too-- or it might
+ * mean that there is no data to compare.
+ **/
+static bool
+config_var_should_list_changes(const config_var_t *var)
 {
-  return (var->flags & CVFLAG_INVISIBLE) != 0;
+  return ! config_var_has_flag(var, CFLG_NOCMP);
 }
+
+/**
+ * Return true iff we need to copy the data for <b>var</b> when we are
+ * copying a config option.
+ *
+ * A false option might mean that the variable is a derived variable, and that
+ * copying the variable it derives from copies it-- or it might mean that
+ * there is no data to copy.
+ **/
+static bool
+config_var_needs_copy(const config_var_t *var)
+{
+  return ! config_var_has_flag(var, CFLG_NOCOPY);
+}
+
+/**
+ * Return true iff variable <b>var</b> should appear on list of variable
+ * names given to the controller or the CLI.
+ *
+ * (Note that this option is imperfectly obeyed. The
+ * --list-torrc-options command looks at the "settable" flag, whereas
+ * "GETINFO config/defaults" and "list_deprecated_*()" do not filter
+ * their results. It would be good for consistency to try to converge
+ * these behaviors in the future.)
+ **/
 bool
+config_var_is_listable(const config_var_t *var)
+{
+  return ! config_var_has_flag(var, CFLG_NOLIST);
+}
+
+/**
+ * Return true iff variable <b>var</b> should be written out when we
+ * are writing our configuration to disk, to a controller, or via the
+ * --dump-config command.
+ *
+ * This option may be set because a variable is hidden, or because it is
+ * derived from another variable which will already be written out.
+ **/
+static bool
 config_var_is_dumpable(const config_var_t *var)
 {
-  return (var->flags & CVFLAG_NODUMP) == 0;
+  return ! config_var_has_flag(var, CFLG_NODUMP);
 }
 
 /*
@@ -650,7 +730,8 @@ config_assign_line(const config_mgr_t *mgr, void *options,
   if (!strlen(c->value)) {
     /* reset or clear it, then return */
     if (!clear_first) {
-      if (config_var_is_cumulative(cvar) && c->command != CONFIG_LINE_CLEAR) {
+      if (! config_var_is_replaced_on_set(cvar) &&
+          c->command != CONFIG_LINE_CLEAR) {
         /* We got an empty linelist from the torrc or command line.
            As a special case, call this an error. Warn and ignore. */
         log_warn(LD_CONFIG,
@@ -671,7 +752,7 @@ config_assign_line(const config_mgr_t *mgr, void *options,
     // LCOV_EXCL_STOP
   }
 
-  if (options_seen && ! config_var_is_cumulative(cvar)) {
+  if (options_seen && config_var_is_replaced_on_set(cvar)) {
     /* We're tracking which options we've seen, and this option is not
      * supposed to occur more than once. */
     tor_assert(var_index >= 0);
@@ -750,6 +831,11 @@ config_get_assigned_option(const config_mgr_t *mgr, const void *options,
     log_warn(LD_CONFIG, "Unknown option '%s'.  Failing.", key);
     return NULL;
   }
+  if (! config_var_is_gettable(var->cvar)) {
+    log_warn(LD_CONFIG, "Option '%s' is obsolete or unfetchable. Failing.",
+             key);
+    return NULL;
+  }
   const void *object = config_mgr_get_obj(mgr, options, var->object_idx);
 
   result = struct_var_kvencode(object, &var->cvar->member);
@@ -989,7 +1075,7 @@ config_get_changes(const config_mgr_t *mgr,
   config_line_t *result = NULL;
   config_line_t **next = &result;
   SMARTLIST_FOREACH_BEGIN(mgr->all_vars, managed_var_t *, mv) {
-    if (config_var_is_contained(mv->cvar)) {
+    if (! config_var_should_list_changes(mv->cvar)) {
       /* something else will check this var, or it doesn't need checking */
       continue;
     }
@@ -1025,7 +1111,7 @@ config_dup(const config_mgr_t *mgr, const void *old)
 
   newopts = config_new(mgr);
   SMARTLIST_FOREACH_BEGIN(mgr->all_vars, managed_var_t *, mv) {
-    if (config_var_is_contained(mv->cvar)) {
+    if (! config_var_needs_copy(mv->cvar)) {
       // Something else will copy this option, or it doesn't need copying.
       continue;
     }
@@ -1092,10 +1178,6 @@ config_dump(const config_mgr_t *mgr, const void *default_options,
   elements = smartlist_new();
   SMARTLIST_FOREACH_BEGIN(mgr->all_vars, managed_var_t *, mv) {
     int comment_option = 0;
-    if (config_var_is_contained(mv->cvar)) {
-      // Something else will dump this option, or it doesn't need dumping.
-      continue;
-    }
     /* Don't save 'hidden' control variables. */
     if (! config_var_is_dumpable(mv->cvar))
       continue;

src/app/config/confparse.h

@@ -189,11 +189,8 @@ const char *config_expand_abbrev(const config_mgr_t *mgr,
                                  int command_line, int warn_obsolete);
 void warn_deprecated_option(const char *what, const char *why);
 
-bool config_var_is_cumulative(const config_var_t *var);
 bool config_var_is_settable(const config_var_t *var);
-bool config_var_is_contained(const config_var_t *var);
-bool config_var_is_invisible(const config_var_t *var);
-bool config_var_is_dumpable(const config_var_t *var);
+bool config_var_is_listable(const config_var_t *var);
 
 /* Helper macros to compare an option across two configuration objects */
 #define CFG_EQ_BOOL(a,b,opt) ((a)->opt == (b)->opt)

src/lib/conf/confmacros.h

@@ -61,7 +61,7 @@
 
 #define CONFIG_VAR_OBSOLETE(varname)            \
   { .member = { .name = varname, .type = CONFIG_TYPE_OBSOLETE },        \
-    .flags = CVFLAG_OBSOLETE                                            \
+    .flags = CFLG_GROUP_OBSOLETE                                        \
   }
 
 #endif /* !defined(TOR_LIB_CONF_CONFMACROS_H) */

src/lib/conf/conftypes.h

@@ -132,22 +132,58 @@ typedef struct struct_magic_decl_t {
 } struct_magic_decl_t;
 
 /**
- * Flag to indicate that an option is obsolete. Any attempt to set or
- * fetch this option should produce a warning.
+ * Flag to indicate that an option or type is "undumpable". An
+ * undumpable option is never saved to disk.
+ *
+ * For historical reasons its name is usually is prefixed with __.
  **/
-#define CVFLAG_OBSOLETE  (1u<<0)
+#define CFLG_NODUMP    (1u<<0)
 /**
- * Flag to indicate that an option is undumpable. An undumpable option is
- * never saved to disk. For historical reasons it is prefixed with __ but
- * not with ___.
+ * Flag to indicate that an option or type is "unlisted".
+ *
+ * We don't tell the controller about unlisted options when it asks for a
+ * list of them.
  **/
-#define CVFLAG_NODUMP    (1u<<1)
+#define CFLG_NOLIST (1u<<1)
 /**
- * Flag to indicate that an option is "invisible". An invisible option
- * is always undumpable, and we don't tell the controller about it.
- * For historical reasons it is prefixed with ___.
+ * Flag to indicate that an option or type is "unsettable".
+ *
+ * An unsettable option can never be set directly by name.
  **/
-#define CVFLAG_INVISIBLE (1u<<2)
+#define CFLG_NOSET (1u<<2)
+/**
+ * Flag to indicate that an option or type does not need to be copied when
+ * copying the structure that contains it.
+ *
+ * (Usually, if an option does not need to be copied, then either it contains
+ * no data, or the data that it does contain is completely contained within
+ * another option.)
+ **/
+#define CFLG_NOCOPY (1u<<3)
+/**
+ * Flag to indicate that an option or type does not need to be compared
+ * when telling the controller about the differences between two
+ * configurations.
+ *
+ * (Usually, if an option does not need to be compared, then either it
+ * contains no data, or the data that it does contain is completely contained
+ * within another option.)
+ **/
+#define CFLG_NOCMP (1u<<4)
+/**
+ * Flag to indicate that an option or type should not be replaced when setting
+ * it.
+ *
+ * For most options, setting them replaces their old value.  For some options,
+ * however, setting them appends to their old value.
+ */
+#define CFLG_NOREPLACE    (1u<<5)
+
+/**
+ * A group of flags that should be set on all obsolete options and types.
+ **/
+#define CFLG_GROUP_OBSOLETE \
+  (CFLG_NOCOPY|CFLG_NOCMP|CFLG_NODUMP|CFLG_NOSET|CFLG_NOLIST)
 
 /** A variable allowed in the configuration file or on the command line. */
 typedef struct config_var_t {

src/lib/confmgt/structvar.c

@@ -211,26 +211,11 @@ struct_var_get_typename(const struct_member_t *member)
   return def ? def->name : NULL;
 }
 
-bool
-struct_var_is_cumulative(const struct_member_t *member)
+/** Return all of the flags set for this struct member. */
+uint32_t
+struct_var_get_flags(const struct_member_t *member)
 {
   const var_type_def_t *def = get_type_def(member);
 
-  return def ? def->is_cumulative : false;
-}
-
-bool
-struct_var_is_settable(const struct_member_t *member)
-{
-  const var_type_def_t *def = get_type_def(member);
-
-  return def ? !def->is_unsettable : true;
-}
-
-bool
-struct_var_is_contained(const struct_member_t *member)
-{
-  const var_type_def_t *def = get_type_def(member);
-
-  return def ? def->is_contained : false;
+  return def ? def->flags : 0;
 }

src/lib/confmgt/structvar.h

@@ -17,6 +17,7 @@ struct struct_member_t;
 struct config_line_t;
 
 #include <stdbool.h>
+#include "lib/cc/torint.h"
 
 void struct_set_magic(void *object,
                       const struct struct_magic_decl_t *decl);
@@ -41,9 +42,7 @@ void struct_var_mark_fragile(void *object,
 
 const char *struct_var_get_name(const struct struct_member_t *member);
 const char *struct_var_get_typename(const struct struct_member_t *member);
-bool struct_var_is_cumulative(const struct struct_member_t *member);
-bool struct_var_is_settable(const struct struct_member_t *member);
-bool struct_var_is_contained(const struct struct_member_t *member);
+uint32_t struct_var_get_flags(const struct struct_member_t *member);
 
 int struct_var_kvassign(void *object, const struct config_line_t *line,
                         char **errmsg,

src/lib/confmgt/type_defs.c

@@ -725,16 +725,22 @@ static const var_type_def_t type_definitions_table[] = {
   [CONFIG_TYPE_CSV_INTERVAL] = { .name="TimeInterval",
                                  .fns=&legacy_csv_interval_fns, },
   [CONFIG_TYPE_LINELIST] = { .name="LineList", .fns=&linelist_fns,
-                             .is_cumulative=true},
+                             .flags=CFLG_NOREPLACE },
+  /*
+   * A "linelist_s" is a derived view of a linelist_v: inspecting
+   * it gets part of a linelist_v, and setting it adds to the linelist_v.
+   */
   [CONFIG_TYPE_LINELIST_S] = { .name="Dependent", .fns=&linelist_s_fns,
-                               .is_cumulative=true,
-                               .is_contained=true, },
+                               .flags=CFLG_NOREPLACE|
+                               /* The operations we disable here are
+                                * handled by the linelist_v. */
+                               CFLG_NOCOPY|CFLG_NOCMP|CFLG_NODUMP },
   [CONFIG_TYPE_LINELIST_V] = { .name="Virtual", .fns=&linelist_v_fns,
-                               .is_cumulative=true,
-                               .is_unsettable=true },
-  [CONFIG_TYPE_OBSOLETE] = { .name="Obsolete", .fns=&ignore_fns,
-                             .is_unsettable=true,
-                             .is_contained=true, }
+                               .flags=CFLG_NOREPLACE|CFLG_NOSET },
+  [CONFIG_TYPE_OBSOLETE] = {
+         .name="Obsolete", .fns=&ignore_fns,
+         .flags=CFLG_GROUP_OBSOLETE,
+  }
 };
 
 /**

src/lib/confmgt/typedvar.c

@@ -225,32 +225,3 @@ typed_var_mark_fragile(void *value, const var_type_def_t *def)
   if (def->fns->mark_fragile)
     def->fns->mark_fragile(value, def->params);
 }
-
-/**
- * Return true iff multiple assignments to a variable will extend its
- * value, rather than replacing it.
- **/
-bool
-var_type_is_cumulative(const var_type_def_t *def)
-{
-  return def->is_cumulative;
-}
-
-/**
- * Return true iff this variable type is always contained in another variable,
- * and as such doesn't need to be dumped or copied independently.
- **/
-bool
-var_type_is_contained(const var_type_def_t *def)
-{
-  return def->is_contained;
-}
-
-/**
- * Return true iff this type can not be assigned directly by the user.
- **/
-bool
-var_type_is_settable(const var_type_def_t *def)
-{
-  return ! def->is_unsettable;
-}

src/lib/confmgt/typedvar.h

@@ -35,8 +35,4 @@ struct config_line_t *typed_var_kvencode(const char *key, const void *value,
 
 void typed_var_mark_fragile(void *value, const var_type_def_t *def);
 
-bool var_type_is_cumulative(const var_type_def_t *def);
-bool var_type_is_contained(const var_type_def_t *def);
-bool var_type_is_settable(const var_type_def_t *def);
-
 #endif /* !defined(TOR_LIB_CONFMGT_TYPEDVAR_H) */

src/lib/confmgt/var_type_def_st.h

@@ -151,17 +151,11 @@ struct var_type_def_t {
    * calling the functions in this type's function table.
    */
   const void *params;
-
-  /** True iff a variable of this type can never be set directly by name. */
-  bool is_unsettable;
-  /** True iff a variable of this type is always contained in another
-   * variable, and as such doesn't need to be dumped or copied
-   * independently. */
-  bool is_contained;
-  /** True iff a variable of this type can be set more than once without
-   * destroying older values. Such variables should implement "mark_fragile".
-   */
-  bool is_cumulative;
+  /**
+   * A bitwise OR of one or more VTFLAG_* values, describing properties
+   * for all values of this type.
+   **/
+  uint32_t flags;
 };
 
 #endif /* !defined(TOR_LIB_CONFMGT_VAR_TYPE_DEF_ST_H) */

src/test/include.am

@@ -23,7 +23,8 @@ TESTSCRIPTS = \
 	src/test/test_workqueue_pipe.sh \
 	src/test/test_workqueue_pipe2.sh \
 	src/test/test_workqueue_socketpair.sh \
-	src/test/test_switch_id.sh
+	src/test/test_switch_id.sh \
+	src/test/test_cmdline.sh
 
 if USE_RUST
 TESTSCRIPTS += \
@@ -412,7 +413,8 @@ EXTRA_DIST += \
 	src/test/test_workqueue_efd2.sh \
 	src/test/test_workqueue_pipe.sh \
 	src/test/test_workqueue_pipe2.sh \
-	src/test/test_workqueue_socketpair.sh
+	src/test/test_workqueue_socketpair.sh \
+	src/test/test_cmdline.sh
 
 test-rust:
 	$(TESTS_ENVIRONMENT) "$(abs_top_srcdir)/src/test/test_rust.sh"

src/test/test_cmdline.sh

@@ -0,0 +1,48 @@
+#!/bin/sh
+
+umask 077
+set -e
+
+if [ $# -ge 1 ]; then
+  TOR_BINARY="${1}"
+  shift
+else
+  TOR_BINARY="${TESTING_TOR_BINARY:-./src/app/tor}"
+fi
+
+echo "TOR BINARY IS ${TOR_BINARY}"
+
+die() { echo "$1" >&2 ; exit 5; }
+
+echo "A"
+
+DATA_DIR=$(mktemp -d -t tor_cmdline_tests.XXXXXX)
+trap 'rm -rf "$DATA_DIR"' 0
+
+# 1. Test list-torrc-options.
+OUT="${DATA_DIR}/output"
+
+echo "B"
+"${TOR_BINARY}" --list-torrc-options > "$OUT"
+
+echo "C"
+
+# regular options are given.
+grep -i "SocksPort" "$OUT" >/dev/null || die "Did not find SocksPort"
+
+
+echo "D"
+
+# unlisted options are given, since they do not have the NOSET flag.
+grep -i "__SocksPort" "$OUT" > /dev/null || die "Did not find __SocksPort"
+
+echo "E"
+
+# unsettable options are not given.
+if grep -i "DisableIOCP" "$OUT"  /dev/null; then
+    die "Found DisableIOCP"
+fi
+if grep -i "HiddenServiceOptions" "$OUT" /dev/null ; then
+    die "Found HiddenServiceOptions"
+fi
+echo "OK"

src/test/test_config.c

@@ -5983,6 +5983,31 @@ test_config_kvline_parse(void *arg)
   tor_free(enc);
 }
 
+static void
+test_config_getinfo_config_names(void *arg)
+{
+  (void)arg;
+  char *answer = NULL;
+  const char *error = NULL;
+  int rv;
+
+  rv = getinfo_helper_config(NULL, "config/names", &answer, &error);
+  tt_int_op(rv, OP_EQ, 0);
+  tt_ptr_op(error, OP_EQ, NULL);
+
+  // ContactInfo should be listed.
+  tt_assert(strstr(answer, "\nContactInfo String\n"));
+
+  // V1AuthoritativeDirectory should not be listed, since it is obsolete.
+  tt_assert(! strstr(answer, "V1AuthoritativeDirectory"));
+
+  // ___UsingTestNetworkDefaults should not be listed, since it is invisible.
+  tt_assert(! strstr(answer, "UsingTestNetworkDefaults"));
+
+ done:
+  tor_free(answer);
+}
+
 #define CONFIG_TEST(name, flags)                          \
   { #name, test_config_ ## name, flags, NULL, NULL }
 
@@ -6035,5 +6060,6 @@ struct testcase_t config_tests[] = {
   CONFIG_TEST(compute_max_mem_in_queues, 0),
   CONFIG_TEST(extended_fmt, 0),
   CONFIG_TEST(kvline_parse, 0),
+  CONFIG_TEST(getinfo_config_names, 0),
   END_OF_TESTCASES
 };