From b133325a3ecae9f405127d4b6c0406056723029f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 26 Feb 2020 12:40:53 -0500 Subject: [PATCH 1/4] confmgt: when an int/enum value is invalid, say which values are ok. Part of 33460. --- src/lib/confmgt/type_defs.c | 47 +++++++++++++++++++++++++++---------- src/test/test_options.c | 3 ++- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c index 3f66cba61a..dd0a846f3e 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; @@ -348,11 +349,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 +367,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 +386,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 +414,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 +426,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 +761,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/test/test_options.c b/src/test/test_options.c index b6a9a21501..b67c37f505 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4276,7 +4276,8 @@ 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, + "Unrecognized value ambidextrous. Allowed values are 0 and 1."); tor_free(msg); config_free_lines(lines); From 7e7aff9b6a3bcf240fe30320e1bcd5ffdab92f13 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 26 Feb 2020 12:40:53 -0500 Subject: [PATCH 2/4] confmgt: when a units value is invalid, include a meaningful error. Part of 33460. --- src/lib/confmgt/type_defs.c | 20 +++++++++++++---- src/lib/confmgt/unitparse.c | 45 ++++++++++++++++++++++++++----------- src/lib/confmgt/unitparse.h | 3 ++- src/test/test_options.c | 16 ++++--------- 4 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c index dd0a846f3e..d9e5e1e4c2 100644 --- a/src/lib/confmgt/type_defs.c +++ b/src/lib/confmgt/type_defs.c @@ -229,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; } @@ -245,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; diff --git a/src/lib/confmgt/unitparse.c b/src/lib/confmgt/unitparse.c index 52c0eabb69..61edc60694 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" @@ -110,23 +112,30 @@ const struct unit_table_t time_msec_units[] = { * table u, then multiply the number by the unit multiplier. * On success, set *ok to 1 and return this product. * Otherwise, set *ok 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 *errmsg_out 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; } @@ -148,8 +157,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; } @@ -157,8 +166,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; } @@ -168,8 +177,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; } @@ -179,10 +188,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 @@ -196,7 +215,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; } @@ -208,7 +227,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; @@ -225,7 +244,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 b67c37f505..129a8299e3 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); From 2c792d1e0e6f92c33287ba8e87055142745a6532 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 26 Feb 2020 14:16:55 -0500 Subject: [PATCH 3/4] In typed_var_kvassign, include config names in error messages. This should improve the usability of our configuration error messages. --- src/lib/confmgt/typedvar.c | 11 ++++++++++- src/test/test_options.c | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) 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/test/test_options.c b/src/test/test_options.c index 129a8299e3..636d3c0e54 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4269,7 +4269,8 @@ test_options_trial_assign(void *arg) 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. Allowed values are 0 and 1."); + "Could not parse UseBridges: Unrecognized value ambidextrous. " + "Allowed values are 0 and 1."); tor_free(msg); config_free_lines(lines); From 87e08730565d0da3b598b2ecc20ed4c8791414ef Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 26 Feb 2020 14:19:53 -0500 Subject: [PATCH 4/4] changes file for ticket 33460. --- changes/ticket33460 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket33460 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.