lttng: list valid condition / action names if missing or unknown

I think it would be helpful to the user to list the condition and action
names, when the condition or action name is missing or unrecognized.
This patch implements that, here are some examples of the result:

    $ lttng add-trigger --action notify --condition
    Error: While parsing argument #4: Missing required argument for option `--condition`
    Error: Valid condition names are:
    Error:   event-rule-matches

    $ lttng add-trigger --action notify --condition pouet
    Error: While parsing argument #5: Unknown condition name 'pouet'
    Error: Valid condition names are:
    Error:   event-rule-matches

    $ lttng add-trigger --condition event-rule-matches --action
    Error: While parsing argument #4: Missing required argument for option `--action`
    Error: Valid action names are:
    Error:   notify

    $ lttng add-trigger --condition event-rule-matches --action pouet
    Error: While parsing argument #5: Unknown action name: pouet
    Error: Valid action names are:
    Error:   notify

To achieve this, add a new optional out argument to parse_next_item, to
allow the caller to get the argpar_error object if a parsing error
happened.  Because of this, the callers must now be able to
differentiate parsing error from memory errors: in the latter case, no
argpar_error object is returned.  So, add a new
PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY status, and make users of
parse_next_item handle it.

In the add-trigger command implementation, handle the "missing opt arg"
case of OPT_ACTION and OPT_CONDITION specially to print the valid names.
Handle unknown names in parse_action and parse_condition.

Add a test for an unknown action name, it seems to be missing.  Change
the error message format slightly to make it match the messages for
unknown condition names.

Change-Id: I4c13cecacb3a2ff4367e391c4aba0d05f1f28f22
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 b1985e95c9100f89fba780ee42552baaffd18b2a..b09a1bf82dce418468c7e03fb52e32d8415a1873 100644 (file)
--- a/src/bin/lttng/commands/add_trigger.cpp
+++ b/src/bin/lttng/commands/add_trigger.cpp
@@ -697,8 +697,9 @@ struct parse_event_rule_res parse_event_rule(int *argc, const char ***argv,
                enum parse_next_item_status status;
 
                status = parse_next_item(argpar_iter, &argpar_item,
-                       argc_offset, *argv, false, NULL);
-               if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+                       argc_offset, *argv, false, NULL, NULL);
+               if (status == PARSE_NEXT_ITEM_STATUS_ERROR ||
+                               status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;
@@ -1414,6 +1415,18 @@ struct condition_descr condition_descrs[] = {
        { "event-rule-matches", handle_condition_event },
 };
 
+static
+void print_valid_condition_names(void)
+{
+       unsigned int i;
+
+       ERR("Valid condition names are:");
+
+       for (i = 0; i < ARRAY_SIZE(condition_descrs); ++i) {
+               ERR("  %s", condition_descrs[i].name);
+       }
+}
+
 static
 struct lttng_condition *parse_condition(const char *condition_name, int *argc,
                const char ***argv, int argc_offset, int orig_arg_index,
@@ -1433,6 +1446,7 @@ struct lttng_condition *parse_condition(const char *condition_name, int *argc,
        if (!descr) {
                ERR(WHILE_PARSING_ARG_N_ARG_FMT "Unknown condition name '%s'",
                        orig_arg_index + 1, orig_arg, condition_name);
+               print_valid_condition_names();
                goto error;
        }
 
@@ -1547,9 +1561,10 @@ struct lttng_action *handle_action_notify(int *argc, const char ***argv,
                enum parse_next_item_status status;
 
                status = parse_next_item(argpar_iter, &argpar_item,
-                       argc_offset, *argv, false,
+                       argc_offset, *argv, false, NULL,
                        "While parsing `notify` action:");
-               if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+               if (status == PARSE_NEXT_ITEM_STATUS_ERROR ||
+                               status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;
@@ -1653,9 +1668,10 @@ static struct lttng_action *handle_action_simple_session_with_policy(int *argc,
                enum parse_next_item_status status;
 
                status = parse_next_item(argpar_iter, &argpar_item, argc_offset,
-                       *argv, false,
+                       *argv, false, NULL,
                        "While parsing `%s` action:", action_name);
-               if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+               if (status == PARSE_NEXT_ITEM_STATUS_ERROR ||
+                               status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;
@@ -1813,8 +1829,9 @@ struct lttng_action *handle_action_snapshot_session(int *argc,
                enum parse_next_item_status status;
 
                status = parse_next_item(argpar_iter, &argpar_item, argc_offset,
-                       *argv, false, "While parsing `snapshot` action:");
-               if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+                       *argv, false, NULL, "While parsing `snapshot` action:");
+               if (status == PARSE_NEXT_ITEM_STATUS_ERROR ||
+                               status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;
@@ -2097,6 +2114,18 @@ struct action_descr action_descrs[] = {
        { "snapshot-session", handle_action_snapshot_session },
 };
 
+static
+void print_valid_action_names(void)
+{
+       unsigned int i;
+
+       ERR("Valid action names are:");
+
+       for (i = 0; i < ARRAY_SIZE(condition_descrs); ++i) {
+               ERR("  %s", action_descrs[i].name);
+       }
+}
+
 static
 struct lttng_action *parse_action(const char *action_name, int *argc,
                const char ***argv, int argc_offset, int orig_arg_index,
@@ -2114,8 +2143,9 @@ struct lttng_action *parse_action(const char *action_name, int *argc,
        }
 
        if (!descr) {
-               ERR(WHILE_PARSING_ARG_N_ARG_FMT "Unknown action name: %s",
+               ERR(WHILE_PARSING_ARG_N_ARG_FMT "Unknown action name '%s'",
                        orig_arg_index + 1, orig_arg, action_name);
+               print_valid_action_names();
                goto error;
        }
 
@@ -2160,6 +2190,7 @@ int cmd_add_trigger(int argc, const char **argv)
        struct lttng_dynamic_pointer_array actions;
        struct argpar_iter *argpar_iter = NULL;
        const struct argpar_item *argpar_item = NULL;
+       const struct argpar_error *argpar_error = NULL;
        struct lttng_action *action_list = NULL;
        struct lttng_action *action = NULL;
        struct lttng_trigger *trigger = NULL;
@@ -2211,8 +2242,22 @@ int cmd_add_trigger(int argc, const char **argv)
                }
 
                status = parse_next_item(argpar_iter, &argpar_item,
-                       argc - my_argc, my_argv, true, NULL);
+                       argc - my_argc, my_argv, true, &argpar_error, NULL);
                if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+
+                       if (argpar_error_type(argpar_error) ==
+                                       ARGPAR_ERROR_TYPE_MISSING_OPT_ARG) {
+                               int opt_id = argpar_error_opt_descr(argpar_error, NULL)->id;
+
+                               if (opt_id == OPT_CONDITION) {
+                                       print_valid_condition_names();
+                               } else if (opt_id == OPT_ACTION) {
+                                       print_valid_action_names();
+                               }
+                       }
+
+                       goto error;
+               } else if (status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;
@@ -2432,6 +2477,7 @@ end:
        }
 
 cleanup:
+       argpar_error_destroy(argpar_error);
        argpar_iter_destroy(argpar_iter);
        argpar_item_destroy(argpar_item);
        lttng_dynamic_pointer_array_reset(&actions);

diff --git a/src/bin/lttng/commands/list_triggers.cpp b/src/bin/lttng/commands/list_triggers.cpp
index 0691a982c9839a7a3ddb2fde9d13e143c8d6cd76..274bae47d0af90e1301af4e4ad4443e99e31c84e 100644 (file)
--- a/src/bin/lttng/commands/list_triggers.cpp
+++ b/src/bin/lttng/commands/list_triggers.cpp
@@ -1336,8 +1336,9 @@ int cmd_list_triggers(int argc, const char **argv)
                enum parse_next_item_status status;
 
                status = parse_next_item(argpar_iter, &argpar_item, 1, argv,
-                       true, NULL);
-               if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+                       true, NULL, NULL);
+               if (status == PARSE_NEXT_ITEM_STATUS_ERROR ||
+                               status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;

diff --git a/src/bin/lttng/commands/remove_trigger.cpp b/src/bin/lttng/commands/remove_trigger.cpp
index a98fe978ca8bab2a33d99553467c7da4d7076fd9..a84b552dfb1d3aab583ebac0fbe525f828c7f3dd 100644 (file)
--- a/src/bin/lttng/commands/remove_trigger.cpp
+++ b/src/bin/lttng/commands/remove_trigger.cpp
@@ -112,8 +112,9 @@ int cmd_remove_trigger(int argc, const char **argv)
                enum parse_next_item_status status;
 
                status = parse_next_item(argpar_iter, &argpar_item, 1, argv,
-                       true, NULL);
-               if (status == PARSE_NEXT_ITEM_STATUS_ERROR) {
+                       true, NULL, NULL);
+               if (status == PARSE_NEXT_ITEM_STATUS_ERROR ||
+                               status == PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY) {
                        goto error;
                } else if (status == PARSE_NEXT_ITEM_STATUS_END) {
                        break;

diff --git a/src/common/argpar-utils/argpar-utils.c b/src/common/argpar-utils/argpar-utils.c
index af3e76102c57fe0d17421f448beaf7091bbede82..493598693632ec32556a0260488a7d16662c8a84 100644 (file)
--- a/src/common/argpar-utils/argpar-utils.c
+++ b/src/common/argpar-utils/argpar-utils.c
@@ -120,6 +120,7 @@ end:
 enum parse_next_item_status parse_next_item(struct argpar_iter *iter,
                const struct argpar_item **item, int argc_offset,
                const char **argv, bool unknown_opt_is_error,
+               const struct argpar_error **error_out,
                const char *context_fmt, ...)
 {
        enum argpar_iter_next_status status;
@@ -132,7 +133,7 @@ enum parse_next_item_status parse_next_item(struct argpar_iter *iter,
        switch (status) {
        case ARGPAR_ITER_NEXT_STATUS_ERROR_MEMORY:
                ERR("Failed to get next argpar item.");
-               ret = PARSE_NEXT_ITEM_STATUS_ERROR;
+               ret = PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY;
                break;
        case ARGPAR_ITER_NEXT_STATUS_ERROR:
        {
@@ -170,6 +171,12 @@ enum parse_next_item_status parse_next_item(struct argpar_iter *iter,
                abort();
        }
 
+       if (error_out) {
+               argpar_error_destroy(*error_out);
+               *error_out = error;
+               error = NULL;
+       }
+
        argpar_error_destroy(error);
 
        return ret;

diff --git a/src/common/argpar-utils/argpar-utils.h b/src/common/argpar-utils/argpar-utils.h
index 32c60c6dd7f16a85430a9bc99c73e54c07fd5052..1bdff7e19822b065e056acfe348ab34c985de1f8 100644 (file)
--- a/src/common/argpar-utils/argpar-utils.h
+++ b/src/common/argpar-utils/argpar-utils.h
@@ -25,6 +25,7 @@ enum parse_next_item_status
        PARSE_NEXT_ITEM_STATUS_OK = 0,
        PARSE_NEXT_ITEM_STATUS_END = 1,
        PARSE_NEXT_ITEM_STATUS_ERROR = -1,
+       PARSE_NEXT_ITEM_STATUS_ERROR_MEMORY = -2,
 };
 
 /*
@@ -45,11 +46,16 @@ enum parse_next_item_status
  *
  * 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.
+ *
+ * If `error_out` is given and PARSE_NEXT_ITEM_STATUS_ERROR is returned, set
+ * `*error_out` to the argpar_error object corresponding to the error.  The
+ * caller must free the object with `argpar_error_destroy`.
  */
-ATTR_FORMAT_PRINTF(6, 7)
+ATTR_FORMAT_PRINTF(7, 8)
 enum parse_next_item_status parse_next_item(struct argpar_iter *iter,
                const struct argpar_item **item, int argc_offset,
                const char **argv, bool unknown_opt_is_error,
+               const struct argpar_error **error_out,
                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 03f8b42422f4287de5fc1e70f36947a2ca13cadf..a12e15ce811702ee4052141a9972ebc0f297869f 100755 (executable)
--- a/tests/regression/tools/trigger/test_add_trigger_cli
+++ b/tests/regression/tools/trigger/test_add_trigger_cli
@@ -23,7 +23,7 @@ TESTDIR="$CURDIR/../../.."
 # shellcheck source=../../../utils/utils.sh
 source "$TESTDIR/utils/utils.sh"
 
-plan_tests 289
+plan_tests 295
 
 FULL_LTTNG_BIN="${TESTDIR}/../src/bin/lttng/${LTTNG_BIN}"
 
@@ -450,13 +450,19 @@ test_failure "invalid argument to --rate-policy: unknown policy type" \
 
 # `--condition` failures
 test_failure "missing args after --condition" \
-       "Error: While parsing argument #2 (\`--condition\`): Missing required argument for option \`--condition\`" \
+       "Error: While parsing argument #2 (\`--condition\`): Missing required argument for option \`--condition\`
+Error: Valid condition names are:
+Error:   event-rule-matches" \
        --condition
 test_failure "unknown --condition" \
-       "Error: While parsing argument #2 (\`--condition\`): Unknown condition name 'zoofest'" \
+       "Error: While parsing argument #2 (\`--condition\`): Unknown condition name 'zoofest'
+Error: Valid condition names are:
+Error:   event-rule-matches" \
        --condition zoofest
 test_failure "unknown --condition=" \
-       "Error: While parsing argument #2 (\`--condition=zoofest\`): Unknown condition name 'zoofest'" \
+       "Error: While parsing argument #2 (\`--condition=zoofest\`): Unknown condition name 'zoofest'
+Error: Valid condition names are:
+Error:   event-rule-matches" \
        --condition=zoofest
 
 # `--condition event-rule-matches` failures
@@ -551,10 +557,26 @@ test_failure "--condition event-rule-matches --capture: missing colon in app-spe
 
 # `--action` failures
 test_failure "missing args after --action" \
-       "Error: While parsing argument #5 (\`--action\`): Missing required argument for option \`--action\`" \
+       "Error: While parsing argument #5 (\`--action\`): Missing required argument for option \`--action\`
+Error: Valid action names are:
+Error:   notify" \
        --condition event-rule-matches --type=user \
        --action
 
+test_failure "unknown --action" \
+       "Error: While parsing argument #5 (\`--action\`): Unknown action name 'zoofest'
+Error: Valid action names are:
+Error:   notify" \
+       --condition event-rule-matches --type=user \
+       --action zoofest
+
+test_failure "unknown --action=" \
+       "Error: While parsing argument #5 (\`--action=zoofest\`): Unknown action name 'zoofest'
+Error: Valid action names are:
+Error:   notify" \
+       --condition event-rule-matches --type=user \
+       --action=zoofest
+
 # `--action notify` failures
 test_failure "extra arg after --action notify" \
        "Error: Unexpected argument \`bob\`." \