lttng: fix argument numbers in add-trigger error messages

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 <simon.marchi@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>

diff --git a/src/bin/lttng/commands/add_trigger.c b/src/bin/lttng/commands/add_trigger.c
index cc4eb1a05f75018215a4c9924db9bc0e05c9db2e..109daba974008515c35a21d49d08bda0244bbf34 100644 (file)
--- 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 69d1e0f0afdd2ceacb2ef3dca130a306339bc14d..f64bd27e963bcaf666dafc5410de5cf118de9896 100644 (file)
--- 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 6f234e626ee73e57e784138a820a5e370d7d3172..4c8ab7fd4f5b4316cd83536a8d0c7ba2841fdac8 100644 (file)
--- 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 6e58ab605aec8da2511053bbc076f9b245b10131..c2fac926f6b24a4ca12cb9dc94628dc81b1992c6 100644 (file)
--- 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 358c5f64bff21101f8acea35a310210d559c76fe..4d7009e59ba530d9db0c570de30100dd8579cf0a 100644 (file)
--- 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 ecf04dd46589a20dbb60225063b42563c82a5c6d..62430279505173b766d531f6061fbd8a75d2debc 100755 (executable)
--- 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