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.cpp b/src/bin/lttng/commands/add_trigger.cpp
index 79303f6944087188f0935ae09119514ddf22953c..58041666a3c692812e79f5d2d784636c09c5189f 100644 (file)
--- a/src/bin/lttng/commands/add_trigger.cpp
+++ b/src/bin/lttng/commands/add_trigger.cpp
@@ -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.cpp b/src/bin/lttng/commands/list_triggers.cpp
index f7798b15033f6285ddda88ea06fca61fd9be188e..0691a982c9839a7a3ddb2fde9d13e143c8d6cd76 100644 (file)
--- a/src/bin/lttng/commands/list_triggers.cpp
+++ b/src/bin/lttng/commands/list_triggers.cpp
@@ -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.cpp b/src/bin/lttng/commands/remove_trigger.cpp
index 0c817d188bbfba83fcb9d0cd9506fbb2d4503d1e..a98fe978ca8bab2a33d99553467c7da4d7076fd9 100644 (file)
--- a/src/bin/lttng/commands/remove_trigger.cpp
+++ b/src/bin/lttng/commands/remove_trigger.cpp
@@ -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 6f5548917abf755f2c7fda3c9776f08a7d2de2d6..9ac246f81bd7d659a98bb3e89615c1b2302bbd22 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 ATTR_FORMAT_PRINTF(4, 0)
+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;
@@ -118,8 +120,9 @@ end:
 }
 
 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;
@@ -145,7 +148,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 b9ae510c9e138e4e9db9afa099399db99a966151..89c5db3118d1d90f8c7efac24912bdd52885459b 100644 (file)
--- a/src/common/argpar-utils/argpar-utils.h
+++ b/src/common/argpar-utils/argpar-utils.h
@@ -39,14 +39,16 @@ 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.
  */
-ATTR_FORMAT_PRINTF(5, 6)
+ATTR_FORMAT_PRINTF(6, 7)
 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, ...);
 
 #ifdef __cplusplus
 }

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