From 4037594c81be528a3d5d52dd118204efa5982cf4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 25 Jul 2023 11:09:43 +0930 Subject: [PATCH] common/json_param: use assert() for param correctness checks. And do them on the first run (where we check parameters), instead of every time. Might as well do them in non-developer mode too, since they're simply programmer correctness. Signed-off-by: Rusty Russell --- common/json_param.c | 68 +++++++++++++------------------- common/test/run-param.c | 86 +++++++++++++++++++++++------------------ 2 files changed, 75 insertions(+), 79 deletions(-) diff --git a/common/json_param.c b/common/json_param.c index e346e9594..d47c116c1 100644 --- a/common/json_param.c +++ b/common/json_param.c @@ -15,6 +15,11 @@ #include #include +/* Overridden by run-param.c */ +#ifndef paramcheck_assert +#define paramcheck_assert assert +#endif + struct param { const char *name; bool is_set; @@ -23,17 +28,17 @@ struct param { void *arg; }; -static bool param_add(struct param **params, +static void param_add(struct param **params, const char *name, enum param_style style, param_cbx cbx, void *arg) { -#if DEVELOPER - if (!(name && cbx && arg)) - return false; -#endif struct param last; + paramcheck_assert(name); + paramcheck_assert(cbx); + paramcheck_assert(arg); + last.is_set = false; last.name = name; last.style = style; @@ -41,7 +46,6 @@ static bool param_add(struct param **params, last.arg = arg; tal_arr_expand(params, last); - return true; } /* FIXME: To support the deprecated p_req_dup_ok */ @@ -175,7 +179,6 @@ static struct command_result *parse_by_name(struct command *cmd, return post_check(cmd, params); } -#if DEVELOPER static int comp_by_name(const struct param *a, const struct param *b, void *unused) { @@ -208,54 +211,47 @@ static int comp_req_order(const struct param *a, const struct param *b, * Make sure 2 sequential items in @params are not equal (based on * provided comparator). */ -static bool check_distinct(struct param *params, - int (*compar) (const struct param *a, - const struct param *b, void *unused)) +static void check_distinct(const struct param *params, + int (*compar)(const struct param *a, + const struct param *b, void *unused)) { - struct param *first = params; - struct param *last = first + tal_count(params); + const struct param *first = params; + const struct param *last = first + tal_count(params); first++; while (first != last) { - if (compar(first - 1, first, NULL) == 0) - return false; + paramcheck_assert(compar(first - 1, first, NULL) != 0); first++; } - return true; } -static bool check_unique(struct param *copy, +static void check_unique(struct param *copy, int (*compar) (const struct param *a, const struct param *b, void *unused)) { asort(copy, tal_count(copy), compar, NULL); - return check_distinct(copy, compar); + check_distinct(copy, compar); } /* * Verify consistent internal state. */ -static bool check_params(struct param *params) +static void check_params(const struct param *params) { if (tal_count(params) < 2) - return true; + return; /* make sure there are no required params following optional */ - if (!check_distinct(params, comp_req_order)) - return false; + check_distinct(params, comp_req_order); /* duplicate so we can sort */ struct param *copy = tal_dup_talarr(params, struct param, params); /* check for repeated names and args */ - if (!check_unique(copy, comp_by_name)) - return false; - if (!check_unique(copy, comp_by_arg)) - return false; + check_unique(copy, comp_by_name); + check_unique(copy, comp_by_arg); tal_free(copy); - return true; } -#endif static char *param_usage(const tal_t *ctx, const struct param *params) @@ -279,12 +275,6 @@ static struct command_result *param_arr(struct command *cmd, const char *buffer, struct param *params, bool allow_extra) { -#if DEVELOPER - if (!check_params(params)) { - return command_fail(cmd, PARAM_DEV_ERROR, - "developer error: check_params"); - } -#endif if (tokens->type == JSMN_ARRAY) return parse_by_position(cmd, params, buffer, tokens, allow_extra); else if (tokens->type == JSMN_OBJECT) @@ -315,6 +305,7 @@ const char *param_subcommand(struct command *cmd, const char *buffer, for (size_t i = 0; i < tal_count(names); i++) tal_append_fmt(&usage, "%c%s", i == 0 ? '=' : '|', names[i]); + check_params(params); command_set_usage(cmd, usage); return NULL; } @@ -354,19 +345,12 @@ bool param(struct command *cmd, const char *buffer, allow_extra = true; continue; } - if (!param_add(¶ms, name, style, cbx, arg)) { - /* We really do ignore this return! */ - struct command_result *ignore; - ignore = command_fail(cmd, PARAM_DEV_ERROR, - "developer error: param_add %s", name); - assert(ignore); - va_end(ap); - return false; - } + param_add(¶ms, name, style, cbx, arg); } va_end(ap); if (command_usage_only(cmd)) { + check_params(params); command_set_usage(cmd, param_usage(cmd, params)); return false; } diff --git a/common/test/run-param.c b/common/test/run-param.c index 342e68f4e..6ee699a29 100644 --- a/common/test/run-param.c +++ b/common/test/run-param.c @@ -3,12 +3,24 @@ #include "../json_filter.c" #include "../json_parse.c" #include "../json_parse_simple.c" -#include "../json_param.c" +#include #include #include #include #include +/* We want to catch parameter checs for bad_programmer() */ +#define paramcheck_assert save_paramcheck_assert + +static bool paramcheck_assert_failed; +static void save_paramcheck_assert(bool cond) +{ + if (!cond) + paramcheck_assert_failed = true; +} + +#include "../json_param.c" + char *fail_msg = NULL; bool failed = false; @@ -299,8 +311,6 @@ static void no_params(void) assert(!param(cmd, j->buffer, j->toks, NULL)); } - -#if DEVELOPER /* * Check to make sure there are no programming mistakes. */ @@ -311,56 +321,60 @@ static void bad_programmer(void) u64 *fpval; struct json *j = json_parse(cmd, "[ '25', '546', '26' ]"); + /* Usage mode makes it check parameters are sane */ + cmd->mode = CMD_USAGE; + /* check for repeated names */ - assert(!param(cmd, j->buffer, j->toks, - p_req("repeat", param_u64, &ival), - p_req("fp", param_millionths, &fpval), - p_req("repeat", param_u64, &ival2), NULL)); - assert(check_fail()); - assert(strstr(fail_msg, "developer error")); + paramcheck_assert_failed = false; + param(cmd, j->buffer, j->toks, + p_req("repeat", param_u64, &ival), + p_req("fp", param_millionths, &fpval), + p_req("repeat", param_u64, &ival2), NULL); + assert(paramcheck_assert_failed); - assert(!param(cmd, j->buffer, j->toks, - p_req("repeat", param_u64, &ival), - p_req("fp", param_millionths, &fpval), - p_req("repeat", param_u64, &ival), NULL)); - assert(check_fail()); - assert(strstr(fail_msg, "developer error")); + paramcheck_assert_failed = false; + param(cmd, j->buffer, j->toks, + p_req("repeat", param_u64, &ival), + p_req("fp", param_millionths, &fpval), + p_req("repeat", param_u64, &ival), NULL); + assert(paramcheck_assert_failed); - assert(!param(cmd, j->buffer, j->toks, - p_req("u64", param_u64, &ival), - p_req("repeat", param_millionths, &fpval), - p_req("repeat", param_millionths, &fpval), NULL)); - assert(check_fail()); - assert(strstr(fail_msg, "developer error")); + paramcheck_assert_failed = false; + param(cmd, j->buffer, j->toks, + p_req("u64", param_u64, &ival), + p_req("repeat", param_millionths, &fpval), + p_req("repeat", param_millionths, &fpval), NULL); + assert(paramcheck_assert_failed); /* check for repeated arguments */ - assert(!param(cmd, j->buffer, j->toks, - p_req("u64", param_u64, &ival), - p_req("repeated-arg", param_u64, &ival), NULL)); - assert(check_fail()); - assert(strstr(fail_msg, "developer error")); + paramcheck_assert_failed = false; + param(cmd, j->buffer, j->toks, + p_req("u64", param_u64, &ival), + p_req("repeated-arg", param_u64, &ival), NULL); + assert(paramcheck_assert_failed); - assert(!param(cmd, j->buffer, j->toks, - p_req("u64", (param_cbx) NULL, NULL), NULL)); - assert(check_fail()); - assert(strstr(fail_msg, "developer error")); + paramcheck_assert_failed = false; + param(cmd, j->buffer, j->toks, + p_req("u64", (param_cbx) NULL, NULL), NULL); + assert(paramcheck_assert_failed); /* Add required param after optional */ j = json_parse(cmd, "[ '25', '546', '26', '1.1' ]"); unsigned int *msatoshi; u64 *riskfactor_millionths; - assert(!param( + paramcheck_assert_failed = false; + param( cmd, j->buffer, j->toks, p_req("u64", param_u64, &ival), p_req("fp", param_millionths, &fpval), p_opt_def("msatoshi", param_number, &msatoshi, 100), p_req("riskfactor", param_millionths, &riskfactor_millionths), - NULL)); + NULL); assert(*msatoshi); assert(*msatoshi == 100); - assert(check_fail()); - assert(strstr(fail_msg, "developer error")); + assert(paramcheck_assert_failed); + + cmd->mode = CMD_NORMAL; } -#endif static void add_members(struct param **params, char **obj, @@ -665,9 +679,7 @@ int main(int argc, char *argv[]) tok_tok(); null_params(); no_params(); -#if DEVELOPER bad_programmer(); -#endif dup_names(); five_hundred_params(); sendpay();