From 5f5ce3e62ef9681799c187493ea3bcd0143230f1 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 25 Aug 2021 14:34:25 -0400 Subject: [PATCH] lttng: fix argument numbers in add-trigger error messages MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Argument numbers in add-trigger argument parsing error messages are wrong. For example: $ ./src/bin/lttng/lttng add-trigger --condition event-rule-matches --foo Error: While parsing argument #1 (`--foo`): Unknown option `--foo` This is due to the fact that multiple separate argpar iterators are created to parse an add-trigger command line. Iterators are created at the top-level, to parse the top-level arguments. Iterators are also created when parsing a condition or an action, to parse the arguments specific to that condition or action. As a result, iterators are passed a subset of the full command line, and the argument indices in the error messages are off. Fix that by passing around an "argc offset", which represents by how much what's being parsed is offset from the full command-line. Use that to adjust the error messages to give indices that make sense to the user: $ ./src/bin/lttng/lttng add-trigger --condition event-rule-matches --foo Error: While parsing argument #4 (`--foo`): Unknown option `--foo` Change-Id: I1d312593d338641d0ec10abe515b640771a1f160 Signed-off-by: Simon Marchi Signed-off-by: Jérémie Galarneau --- src/bin/lttng/commands/add_trigger.c | 66 +++++++++++-------- src/bin/lttng/commands/list_triggers.c | 2 +- src/bin/lttng/commands/remove_trigger.c | 2 +- src/common/argpar-utils/argpar-utils.c | 22 ++++--- src/common/argpar-utils/argpar-utils.h | 8 ++- .../tools/trigger/test_add_trigger_cli | 10 +-- 6 files changed, 65 insertions(+), 45 deletions(-) diff --git a/src/bin/lttng/commands/add_trigger.c b/src/bin/lttng/commands/add_trigger.c index cc4eb1a05..109daba97 100644 --- a/src/bin/lttng/commands/add_trigger.c +++ b/src/bin/lttng/commands/add_trigger.c @@ -649,7 +649,8 @@ struct parse_event_rule_res { }; static -struct parse_event_rule_res parse_event_rule(int *argc, const char ***argv) +struct parse_event_rule_res parse_event_rule(int *argc, const char ***argv, + int argc_offset) { enum lttng_event_rule_type event_rule_type = LTTNG_EVENT_RULE_TYPE_UNKNOWN; @@ -695,8 +696,8 @@ struct parse_event_rule_res parse_event_rule(int *argc, const char ***argv) while (true) { enum parse_next_item_status status; - status = parse_next_item(argpar_iter, &argpar_item, *argv, - false, NULL); + status = parse_next_item(argpar_iter, &argpar_item, + argc_offset, *argv, false, NULL); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; } else if (status == PARSE_NEXT_ITEM_STATUS_END) { @@ -1347,13 +1348,14 @@ end: } static -struct lttng_condition *handle_condition_event(int *argc, const char ***argv) +struct lttng_condition *handle_condition_event(int *argc, const char ***argv, + int argc_offset) { struct parse_event_rule_res res; struct lttng_condition *c; size_t i; - res = parse_event_rule(argc, argv); + res = parse_event_rule(argc, argv, argc_offset); if (!res.er) { c = NULL; goto error; @@ -1403,7 +1405,8 @@ end: struct condition_descr { const char *name; - struct lttng_condition *(*handler) (int *argc, const char ***argv); + struct lttng_condition *(*handler) (int *argc, const char ***argv, + int argc_offset); }; static const @@ -1413,7 +1416,7 @@ struct condition_descr condition_descrs[] = { static struct lttng_condition *parse_condition(const char *condition_name, int *argc, - const char ***argv) + const char ***argv, int argc_offset) { int i; struct lttng_condition *cond; @@ -1431,7 +1434,7 @@ struct lttng_condition *parse_condition(const char *condition_name, int *argc, goto error; } - cond = descr->handler(argc, argv); + cond = descr->handler(argc, argv, argc_offset); if (!cond) { /* The handler has already printed an error message. */ goto error; @@ -1524,7 +1527,8 @@ static const struct argpar_opt_descr notify_action_opt_descrs[] = { }; static -struct lttng_action *handle_action_notify(int *argc, const char ***argv) +struct lttng_action *handle_action_notify(int *argc, const char ***argv, + int argc_offset) { struct lttng_action *action = NULL; struct argpar_iter *argpar_iter = NULL; @@ -1540,8 +1544,9 @@ struct lttng_action *handle_action_notify(int *argc, const char ***argv) while (true) { enum parse_next_item_status status; - status = parse_next_item(argpar_iter, &argpar_item, *argv, - false, "While parsing `notify` action:"); + status = parse_next_item(argpar_iter, &argpar_item, + argc_offset, *argv, false, + "While parsing `notify` action:"); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; } else if (status == PARSE_NEXT_ITEM_STATUS_END) { @@ -1612,6 +1617,7 @@ end: static struct lttng_action *handle_action_simple_session_with_policy(int *argc, const char ***argv, + int argc_offset, struct lttng_action *(*create_action_cb)(void), enum lttng_action_status (*set_session_name_cb)( struct lttng_action *, const char *), @@ -1644,8 +1650,9 @@ static struct lttng_action *handle_action_simple_session_with_policy(int *argc, while (true) { enum parse_next_item_status status; - status = parse_next_item(argpar_iter, &argpar_item, *argv, - false, "While parsing `%s` action:", action_name); + status = parse_next_item(argpar_iter, &argpar_item, argc_offset, + *argv, false, + "While parsing `%s` action:", action_name); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; } else if (status == PARSE_NEXT_ITEM_STATUS_END) { @@ -1730,9 +1737,10 @@ end: static struct lttng_action *handle_action_start_session(int *argc, - const char ***argv) + const char ***argv, int argc_offset) { return handle_action_simple_session_with_policy(argc, argv, + argc_offset, lttng_action_start_session_create, lttng_action_start_session_set_session_name, lttng_action_start_session_set_rate_policy, "start"); @@ -1740,9 +1748,10 @@ struct lttng_action *handle_action_start_session(int *argc, static struct lttng_action *handle_action_stop_session(int *argc, - const char ***argv) + const char ***argv, int argc_offset) { return handle_action_simple_session_with_policy(argc, argv, + argc_offset, lttng_action_stop_session_create, lttng_action_stop_session_set_session_name, lttng_action_stop_session_set_rate_policy, "stop"); @@ -1750,9 +1759,10 @@ struct lttng_action *handle_action_stop_session(int *argc, static struct lttng_action *handle_action_rotate_session(int *argc, - const char ***argv) + const char ***argv, int argc_offset) { return handle_action_simple_session_with_policy(argc, argv, + argc_offset, lttng_action_rotate_session_create, lttng_action_rotate_session_set_session_name, lttng_action_rotate_session_set_rate_policy, @@ -1772,7 +1782,7 @@ static const struct argpar_opt_descr snapshot_action_opt_descrs[] = { static struct lttng_action *handle_action_snapshot_session(int *argc, - const char ***argv) + const char ***argv, int argc_offset) { struct lttng_action *action = NULL; struct argpar_iter *argpar_iter = NULL; @@ -1800,8 +1810,8 @@ struct lttng_action *handle_action_snapshot_session(int *argc, while (true) { enum parse_next_item_status status; - status = parse_next_item(argpar_iter, &argpar_item, *argv, - false, "While parsing `snapshot` action:"); + status = parse_next_item(argpar_iter, &argpar_item, argc_offset, + *argv, false, "While parsing `snapshot` action:"); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; } else if (status == PARSE_NEXT_ITEM_STATUS_END) { @@ -2072,7 +2082,8 @@ end: struct action_descr { const char *name; - struct lttng_action *(*handler) (int *argc, const char ***argv); + struct lttng_action *(*handler) (int *argc, const char ***argv, + int argc_offset); }; static const @@ -2085,7 +2096,8 @@ struct action_descr action_descrs[] = { }; static -struct lttng_action *parse_action(const char *action_name, int *argc, const char ***argv) +struct lttng_action *parse_action(const char *action_name, int *argc, + const char ***argv, int argc_offset) { int i; struct lttng_action *action; @@ -2103,7 +2115,7 @@ struct lttng_action *parse_action(const char *action_name, int *argc, const char goto error; } - action = descr->handler(argc, argv); + action = descr->handler(argc, argv, argc_offset); if (!action) { /* The handler has already printed an error message. */ goto error; @@ -2194,8 +2206,8 @@ int cmd_add_trigger(int argc, const char **argv) goto error; } - status = parse_next_item(argpar_iter, &argpar_item, my_argv, - true, NULL); + status = parse_next_item(argpar_iter, &argpar_item, + argc - my_argc, my_argv, true, NULL); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; } else if (status == PARSE_NEXT_ITEM_STATUS_END) { @@ -2234,7 +2246,8 @@ int cmd_add_trigger(int argc, const char **argv) goto error; } - condition = parse_condition(arg, &my_argc, &my_argv); + condition = parse_condition(arg, &my_argc, &my_argv, + argc - my_argc); if (!condition) { /* * An error message was already printed by @@ -2247,7 +2260,8 @@ int cmd_add_trigger(int argc, const char **argv) } case OPT_ACTION: { - action = parse_action(arg, &my_argc, &my_argv); + action = parse_action(arg, &my_argc, &my_argv, + argc - my_argc); if (!action) { /* * An error message was already printed by diff --git a/src/bin/lttng/commands/list_triggers.c b/src/bin/lttng/commands/list_triggers.c index 69d1e0f0a..f64bd27e9 100644 --- a/src/bin/lttng/commands/list_triggers.c +++ b/src/bin/lttng/commands/list_triggers.c @@ -1335,7 +1335,7 @@ int cmd_list_triggers(int argc, const char **argv) while (true) { enum parse_next_item_status status; - status = parse_next_item(argpar_iter, &argpar_item, argv, + status = parse_next_item(argpar_iter, &argpar_item, 1, argv, true, NULL); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; diff --git a/src/bin/lttng/commands/remove_trigger.c b/src/bin/lttng/commands/remove_trigger.c index 6f234e626..4c8ab7fd4 100644 --- a/src/bin/lttng/commands/remove_trigger.c +++ b/src/bin/lttng/commands/remove_trigger.c @@ -111,7 +111,7 @@ int cmd_remove_trigger(int argc, const char **argv) while (true) { enum parse_next_item_status status; - status = parse_next_item(argpar_iter, &argpar_item, argv, + status = parse_next_item(argpar_iter, &argpar_item, 1, argv, true, NULL); if (status == PARSE_NEXT_ITEM_STATUS_ERROR) { goto error; diff --git a/src/common/argpar-utils/argpar-utils.c b/src/common/argpar-utils/argpar-utils.c index 6e58ab605..c2fac926f 100644 --- a/src/common/argpar-utils/argpar-utils.c +++ b/src/common/argpar-utils/argpar-utils.c @@ -23,10 +23,12 @@ * `context_fmt`, if non-NULL, is formatted using `args` and prepended to the * error message. * + * Add `argc_offset` the the argument index mentioned in the error message. + * * The returned string must be freed by the caller. */ -static ATTR_FORMAT_PRINTF(3, 0) -char *format_arg_error_v(const struct argpar_error *error, +static +char *format_arg_error_v(const struct argpar_error *error, int argc_offset, const char **argv, const char *context_fmt, va_list args) { char *str = NULL; @@ -59,7 +61,7 @@ char *format_arg_error_v(const struct argpar_error *error, ret = strutils_appendf(&str, WHILE_PARSING_ARG_N_ARG_FMT "Missing required argument for option `%s`", - orig_index + 1, arg, arg); + orig_index + 1 + argc_offset, argv[orig_index], arg); if (ret < 0) { goto end; } @@ -77,11 +79,11 @@ char *format_arg_error_v(const struct argpar_error *error, if (is_short) { ret = strutils_appendf(&str, WHILE_PARSING_ARG_N_ARG_FMT "Unexpected argument for option `-%c`", - orig_index + 1, arg, descr->short_name); + orig_index + 1 + argc_offset, arg, descr->short_name); } else { ret = strutils_appendf(&str, WHILE_PARSING_ARG_N_ARG_FMT "Unexpected argument for option `--%s`", - orig_index + 1, arg, descr->long_name); + orig_index + 1 + argc_offset, arg, descr->long_name); } if (ret < 0) { @@ -97,7 +99,7 @@ char *format_arg_error_v(const struct argpar_error *error, ret = strutils_appendf(&str, WHILE_PARSING_ARG_N_ARG_FMT "Unknown option `%s`", - orig_index + 1, argv[orig_index], unknown_opt); + orig_index + 1 + argc_offset, argv[orig_index], unknown_opt); if (ret < 0) { goto end; @@ -119,8 +121,9 @@ end: LTTNG_HIDDEN enum parse_next_item_status parse_next_item(struct argpar_iter *iter, - const struct argpar_item **item, const char **argv, - bool unknown_opt_is_error, const char *context_fmt, ...) + const struct argpar_item **item, int argc_offset, + const char **argv, bool unknown_opt_is_error, + const char *context_fmt, ...) { enum argpar_iter_next_status status; const struct argpar_error *error = NULL; @@ -146,7 +149,8 @@ enum parse_next_item_status parse_next_item(struct argpar_iter *iter, } va_start(args, context_fmt); - err_str = format_arg_error_v(error, argv, context_fmt, args); + err_str = format_arg_error_v(error, argc_offset, argv, + context_fmt, args); va_end(args); if (err_str) { diff --git a/src/common/argpar-utils/argpar-utils.h b/src/common/argpar-utils/argpar-utils.h index 358c5f64b..4d7009e59 100644 --- a/src/common/argpar-utils/argpar-utils.h +++ b/src/common/argpar-utils/argpar-utils.h @@ -35,13 +35,15 @@ enum parse_next_item_status * On error, print a descriptive error message and return * PARSE_NEXT_ITEM_STATUS_ERROR. If `context_fmt` is non-NULL, it is formatted * using the following arguments and prepended to the error message. + * Add `argc_offset` to the argument index mentioned in the error message. * * If `unknown_opt_is_error` is true, an unknown option is considered an error. * Otherwise, it is considered as the end of the argument list. */ -LTTNG_HIDDEN ATTR_FORMAT_PRINTF(5, 6) +LTTNG_HIDDEN enum parse_next_item_status parse_next_item(struct argpar_iter *iter, - const struct argpar_item **item, const char **argv, - bool unknown_opt_is_error, const char *context_fmt, ...); + const struct argpar_item **item, int argc_offset, + const char **argv, bool unknown_opt_is_error, + const char *context_fmt, ...); #endif diff --git a/tests/regression/tools/trigger/test_add_trigger_cli b/tests/regression/tools/trigger/test_add_trigger_cli index ecf04dd46..624302795 100755 --- a/tests/regression/tools/trigger/test_add_trigger_cli +++ b/tests/regression/tools/trigger/test_add_trigger_cli @@ -409,7 +409,7 @@ test_success "--action snapshot-session with ctrl/data URIs" "notify-15"\ test_failure "no args" "Error: Missing --condition." test_failure "unknown option" \ - "Error: While parsing argument #1 (\`--hello\`): Unknown option \`--hello\`" \ + "Error: While parsing argument #2 (\`--hello\`): Unknown option \`--hello\`" \ --hello test_failure "missing --action" \ @@ -423,7 +423,7 @@ test_failure "two --condition" \ --action notify test_failure "missing argument to --name" \ - "Error: While parsing argument #1 (\`--name\`): Missing required argument for option \`--name\`" \ + "Error: While parsing argument #2 (\`--name\`): Missing required argument for option \`--name\`" \ --name for cmd in rate-policy=once-after rate-policy=every; do @@ -450,7 +450,7 @@ test_failure "invalid argument to --rate-policy: unknown policy type" \ # `--condition` failures test_failure "missing args after --condition" \ - "Error: While parsing argument #1 (\`--condition\`): Missing required argument for option \`--condition\`" \ + "Error: While parsing argument #2 (\`--condition\`): Missing required argument for option \`--condition\`" \ --condition test_failure "unknown --condition" \ "Error: Unknown condition name 'zoofest'" \ @@ -502,7 +502,7 @@ test_failure "--exclude-name with non-glob name" \ --action notify test_failure "--condition event-rule-matches --capture: missing argument (end of arg list)" \ - 'Error: While parsing argument #2 (`--capture`): Missing required argument for option `--capture`' \ + 'Error: While parsing argument #7 (`--capture`): Missing required argument for option `--capture`' \ --action notify \ --condition event-rule-matches --type=user --capture @@ -548,7 +548,7 @@ test_failure "--condition event-rule-matches --capture: missing colon in app-spe # `--action` failures test_failure "missing args after --action" \ - "Error: While parsing argument #1 (\`--action\`): Missing required argument for option \`--action\`" \ + "Error: While parsing argument #5 (\`--action\`): Missing required argument for option \`--action\`" \ --condition event-rule-matches --type=user \ --action -- 2.34.1