Merge branch 'maint-0.4.3'

6 files changed, 105 insertions, 45 deletions

diff --git a/changes/ticket33460 b/changes/ticket33460
new file mode 100644
index 0000000000..21e0fc966c
--- /dev/null
+++ b/changes/ticket33460
@@ -0,0 +1,4 @@
+  o Minor features (usability):
+    - Include more information when failing to parse a configuration value.
+      This should make it easier to tell what's going wrong when a
+      configuration file doesn't parse.  Closes ticket 33460.
diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c
index 3f66cba61a..d9e5e1e4c2 100644
--- a/src/lib/confmgt/type_defs.c
+++ b/src/lib/confmgt/type_defs.c
@@ -121,8 +121,9 @@ int_parse(void *target, const char *value, char **errmsg, const void *params)
   int ok=0;
   *p = (int)tor_parse_long(value, 10, pp->minval, pp->maxval, &ok, NULL);
   if (!ok) {
-    tor_asprintf(errmsg, "Integer %s is malformed or out of bounds.",
-                 value);
+    tor_asprintf(errmsg, "Integer %s is malformed or out of bounds. "
+                 "Allowed values are between %d and %d.",
+                 value, pp->minval, pp->maxval);
     return -1;
   }
   return 0;
@@ -228,11 +229,17 @@ units_parse_u64(void *target, const char *value, char **errmsg,
   tor_assert(table);
   uint64_t *v = (uint64_t*)target;
   int ok=1;
-  *v = config_parse_units(value, table, &ok);
+  char *msg = NULL;
+  *v = config_parse_units(value, table, &ok, &msg);
   if (!ok) {
-    *errmsg = tor_strdup("Provided value is malformed or out of bounds.");
+    tor_asprintf(errmsg, "Provided value is malformed or out of bounds: %s",
+                 msg);
+    tor_free(msg);
     return -1;
   }
+  if (BUG(msg)) {
+    tor_free(msg);
+  }
   return 0;
 }
 
@@ -244,11 +251,17 @@ units_parse_int(void *target, const char *value, char **errmsg,
   tor_assert(table);
   int *v = (int*)target;
   int ok=1;
-  uint64_t u64 = config_parse_units(value, table, &ok);
+  char *msg = NULL;
+  uint64_t u64 = config_parse_units(value, table, &ok, &msg);
   if (!ok) {
-    *errmsg = tor_strdup("Provided value is malformed or out of bounds.");
+    tor_asprintf(errmsg, "Provided value is malformed or out of bounds: %s",
+                 msg);
+    tor_free(msg);
     return -1;
   }
+  if (BUG(msg)) {
+    tor_free(msg);
+  }
   if (u64 > INT_MAX) {
     tor_asprintf(errmsg, "Provided value %s is too large", value);
     return -1;
@@ -348,11 +361,17 @@ typedef struct enumeration_table_t {
   int value;
 } enumeration_table_t;
 
+typedef struct enumeration_params_t {
+  const char *allowed_val_string;
+  const enumeration_table_t *table;
+} enumeration_params_t;
+
 static int
 enum_parse(void *target, const char *value, char **errmsg,
-           const void *params)
+           const void *params_)
 {
-  const enumeration_table_t *table = params;
+  const enumeration_params_t *params = params_;
+  const enumeration_table_t *table = params->table;
   int *p = (int *)target;
   for (; table->name; ++table) {
     if (!strcasecmp(value, table->name)) {
@@ -360,15 +379,17 @@ enum_parse(void *target, const char *value, char **errmsg,
       return 0;
     }
   }
-  tor_asprintf(errmsg, "Unrecognized value %s.", value);
+  tor_asprintf(errmsg, "Unrecognized value %s. %s",
+               value, params->allowed_val_string);
   return -1;
 }
 
 static char *
-enum_encode(const void *value, const void *params)
+enum_encode(const void *value, const void *params_)
 {
   int v = *(const int*)value;
-  const enumeration_table_t *table = params;
+  const enumeration_params_t *params = params_;
+  const enumeration_table_t *table = params->table;
   for (; table->name; ++table) {
     if (v == table->value)
       return tor_strdup(table->name);
@@ -377,19 +398,21 @@ enum_encode(const void *value, const void *params)
 }
 
 static void
-enum_clear(void *value, const void *params)
+enum_clear(void *value, const void *params_)
 {
   int *p = (int*)value;
-  const enumeration_table_t *table = params;
+  const enumeration_params_t *params = params_;
+  const enumeration_table_t *table = params->table;
   tor_assert(table->name);
   *p = table->value;
 }
 
 static bool
-enum_ok(const void *value, const void *params)
+enum_ok(const void *value, const void *params_)
 {
   int v = *(const int*)value;
-  const enumeration_table_t *table = params;
+  const enumeration_params_t *params = params_;
+  const enumeration_table_t *table = params->table;
   for (; table->name; ++table) {
     if (v == table->value)
       return true;
@@ -403,6 +426,11 @@ static const enumeration_table_t enum_table_bool[] = {
   { NULL, 0 },
 };
 
+static const enumeration_params_t enum_params_bool = {
+  "Allowed values are 0 and 1.",
+  enum_table_bool
+};
+
 static const enumeration_table_t enum_table_autobool[] = {
   { "0", 0 },
   { "1", 1 },
@@ -410,6 +438,11 @@ static const enumeration_table_t enum_table_autobool[] = {
   { NULL, 0 },
 };
 
+static const enumeration_params_t enum_params_autobool = {
+  "Allowed values are 0, 1, and auto.",
+  enum_table_autobool
+};
+
 static const var_type_fns_t enum_fns = {
   .parse = enum_parse,
   .encode = enum_encode,
@@ -740,10 +773,10 @@ const var_type_def_t DOUBLE_type_defn = {
   .name="Float", .fns=&double_fns, };
 const var_type_def_t BOOL_type_defn = {
   .name="Boolean", .fns=&enum_fns,
-  .params=&enum_table_bool };
+  .params=&enum_params_bool };
 const var_type_def_t AUTOBOOL_type_defn = {
   .name="Boolean+Auto", .fns=&enum_fns,
-  .params=&enum_table_autobool };
+  .params=&enum_params_autobool };
 const var_type_def_t ISOTIME_type_defn = {
   .name="Time", .fns=&time_fns, };
 const var_type_def_t CSV_type_defn = {
diff --git a/src/lib/confmgt/typedvar.c b/src/lib/confmgt/typedvar.c
index 3a188db953..1955302cdc 100644
--- a/src/lib/confmgt/typedvar.c
+++ b/src/lib/confmgt/typedvar.c
@@ -24,6 +24,7 @@
 #include "lib/log/log.h"
 #include "lib/log/util_bug.h"
 #include "lib/malloc/malloc.h"
+#include "lib/string/printf.h"
 #include "lib/string/util_string.h"
 
 #include "lib/confmgt/var_type_def_st.h"
@@ -75,7 +76,15 @@ typed_var_kvassign(void *target, const config_line_t *line,
     return def->fns->kv_parse(target, line, errmsg, def->params);
   }
 
-  return typed_var_assign(target, line->value, errmsg, def);
+  int rv = typed_var_assign(target, line->value, errmsg, def);
+  if (rv < 0 && *errmsg != NULL) {
+    /* typed_var_assign() didn't know the line's keyword, but we do.
+     * Let's add it to the error message. */
+    char *oldmsg = *errmsg;
+    tor_asprintf(errmsg, "Could not parse %s: %s", line->key, oldmsg);
+    tor_free(oldmsg);
+  }
+  return rv;
 }
 
 /**
diff --git a/src/lib/confmgt/unitparse.c b/src/lib/confmgt/unitparse.c
index 995cb93834..99716e8d9d 100644
--- a/src/lib/confmgt/unitparse.c
+++ b/src/lib/confmgt/unitparse.c
@@ -13,7 +13,9 @@
 #include "lib/confmgt/unitparse.h"
 #include "lib/log/log.h"
 #include "lib/log/util_bug.h"
+#include "lib/malloc/malloc.h"
 #include "lib/string/parse_int.h"
+#include "lib/string/printf.h"
 #include "lib/string/util_string.h"
 #include "lib/intmath/muldiv.h"
 
@@ -116,23 +118,30 @@ const struct unit_table_t time_msec_units[] = {
  * table <b>u</b>, then multiply the number by the unit multiplier.
  * On success, set *<b>ok</b> to 1 and return this product.
  * Otherwise, set *<b>ok</b> to 0.
- * Warns user when overflow or a negative value is detected.
+ *
+ * If an error (like overflow or a negative value is detected), put an error
+ * message in *<b>errmsg_out</b> if that pointer is non-NULL, and otherwise
+ * log a warning.
  */
 uint64_t
-config_parse_units(const char *val, const unit_table_t *u, int *ok)
+config_parse_units(const char *val, const unit_table_t *u, int *ok,
+                   char **errmsg_out)
 {
   uint64_t v = 0;
   double d = 0;
   int use_float = 0;
   char *cp;
+  char *errmsg = NULL;
 
   tor_assert(ok);
 
   v = tor_parse_uint64(val, 10, 0, UINT64_MAX, ok, &cp);
   if (!*ok || (cp && *cp == '.')) {
     d = tor_parse_double(val, 0, (double)UINT64_MAX, ok, &cp);
-    if (!*ok)
+    if (!*ok) {
+      tor_asprintf(&errmsg, "Unable to parse %s as a number", val);
       goto done;
+    }
     use_float = 1;
   }
 
@@ -154,8 +163,8 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
         d = u->multiplier * d;
 
         if (d < 0) {
-          log_warn(LD_CONFIG, "Got a negative value while parsing %s %s",
-                   val, u->unit);
+          tor_asprintf(&errmsg, "Got a negative value while parsing %s %s",
+                       val, u->unit);
           *ok = 0;
           goto done;
         }
@@ -163,8 +172,8 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
         // Some compilers may warn about casting a double to an unsigned type
         // because they don't know if d is >= 0
         if (d >= 0 && (d > (double)INT64_MAX || (uint64_t)d > INT64_MAX)) {
-          log_warn(LD_CONFIG, "Overflow detected while parsing %s %s",
-                   val, u->unit);
+          tor_asprintf(&errmsg, "Overflow while parsing %s %s",
+                       val, u->unit);
           *ok = 0;
           goto done;
         }
@@ -174,8 +183,8 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
         v = tor_mul_u64_nowrap(v, u->multiplier);
 
         if (v > INT64_MAX) {
-          log_warn(LD_CONFIG, "Overflow detected while parsing %s %s",
-                   val, u->unit);
+          tor_asprintf(&errmsg, "Overflow while parsing %s %s",
+                       val, u->unit);
           *ok = 0;
           goto done;
         }
@@ -185,10 +194,20 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
       goto done;
     }
   }
-  log_warn(LD_CONFIG, "Unknown unit '%s'.", cp);
+  tor_asprintf(&errmsg, "Unknown unit in %s", val);
   *ok = 0;
  done:
 
+  if (errmsg) {
+    tor_assert_nonfatal(!*ok);
+    if (errmsg_out) {
+      *errmsg_out = errmsg;
+    } else {
+      log_warn(LD_CONFIG, "%s", errmsg);
+      tor_free(errmsg);
+    }
+  }
+
   if (*ok)
     return v;
   else
@@ -202,7 +221,7 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
 uint64_t
 config_parse_memunit(const char *s, int *ok)
 {
-  uint64_t u = config_parse_units(s, memory_units, ok);
+  uint64_t u = config_parse_units(s, memory_units, ok, NULL);
   return u;
 }
 
@@ -214,7 +233,7 @@ int
 config_parse_msec_interval(const char *s, int *ok)
 {
   uint64_t r;
-  r = config_parse_units(s, time_msec_units, ok);
+  r = config_parse_units(s, time_msec_units, ok, NULL);
   if (r > INT_MAX) {
     log_warn(LD_CONFIG, "Msec interval '%s' is too long", s);
     *ok = 0;
@@ -231,7 +250,7 @@ int
 config_parse_interval(const char *s, int *ok)
 {
   uint64_t r;
-  r = config_parse_units(s, time_units, ok);
+  r = config_parse_units(s, time_units, ok, NULL);
   if (r > INT_MAX) {
     log_warn(LD_CONFIG, "Interval '%s' is too long", s);
     *ok = 0;
diff --git a/src/lib/confmgt/unitparse.h b/src/lib/confmgt/unitparse.h
index 3f4f039a0e..047e11b424 100644
--- a/src/lib/confmgt/unitparse.h
+++ b/src/lib/confmgt/unitparse.h
@@ -25,7 +25,8 @@ extern const unit_table_t memory_units[];
 extern const unit_table_t time_units[];
 extern const struct unit_table_t time_msec_units[];
 
-uint64_t config_parse_units(const char *val, const unit_table_t *u, int *ok);
+uint64_t config_parse_units(const char *val, const unit_table_t *u, int *ok,
+                            char **errmsg_out);
 
 uint64_t config_parse_memunit(const char *s, int *ok);
 int config_parse_msec_interval(const char *s, int *ok);
diff --git a/src/test/test_options.c b/src/test/test_options.c
index b6a9a21501..636d3c0e54 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -307,18 +307,10 @@ test_options_validate(void *arg)
   WANT_ERR("BridgeRelay 1\nDirCache 0",
            "We're a bridge but DirCache is disabled.", PH_VALIDATE);
 
-  // XXXX We should replace this with a more full error message once #29211
-  // XXXX is done.  It is truncated for now because at the current stage
-  // XXXX of refactoring, we can't give a full error message like before.
-  WANT_ERR_LOG("HeartbeatPeriod 21 snarks",
-               "malformed or out of bounds", LOG_WARN,
-               "Unknown unit 'snarks'.",
-               PH_ASSIGN);
-  // XXXX As above.
-  WANT_ERR_LOG("LogTimeGranularity 21 snarks",
-               "malformed or out of bounds", LOG_WARN,
-               "Unknown unit 'snarks'.",
-               PH_ASSIGN);
+  WANT_ERR("HeartbeatPeriod 21 snarks",
+           "Unknown unit in 21 snarks", PH_ASSIGN);
+  WANT_ERR("LogTimeGranularity 21 snarks",
+           "Unknown unit in 21 snarks", PH_ASSIGN);
   OK("HeartbeatPeriod 1 hour", PH_VALIDATE);
   OK("LogTimeGranularity 100 milliseconds", PH_VALIDATE);
 
@@ -4276,7 +4268,9 @@ test_options_trial_assign(void *arg)
   tt_int_op(r, OP_EQ, 0);
   v = options_trial_assign(lines, 0, &msg);
   tt_int_op(v, OP_EQ, SETOPT_ERR_PARSE);
-  tt_str_op(msg, OP_EQ, "Unrecognized value ambidextrous.");
+  tt_str_op(msg, OP_EQ,
+            "Could not parse UseBridges: Unrecognized value ambidextrous. "
+            "Allowed values are 0 and 1.");
   tor_free(msg);
   config_free_lines(lines);