cli: handle leftovers in the same loop as components

[this patch is better viewed with `git show -w`]

Handle leftovers (put them in the leftovers array) at the same time as
we process --component options and things that apply to components
(--params and --log-level).  When we handle a leftover, assign the
current_item_type variable, to override the previous current item.

This change makes it so that we prevent processing --params or
--log-level after a leftover.  Previously, this command line:

    babeltrace2 -c src.bon.jour some-leftover --params=a=2

Would apply the `a=2` param to the `src.bon.jour` instance, since it was
the last component declared.  This is confusing, however, because of the
some-leftover leftover in between.

With this patch, the `some-leftover` leftover becomes the "currently"
processed item when we reach it.  And since it's not possible (for the
moment) to apply --params or --log-level to a leftover, the command line
shown above now result in the error:

    No current component of which to set parameters:
        a=2

I improved test_convert_args to be able to check the error message of a
bt2 run we expect to fail.  This lets us avoid cases where babeltrace
fails for an unexpected reason, but the test still passes.

Change-Id: I0e065c1cd5f32f59292c9a40c6a8077a52d35237
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/1806
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
Tested-by: jenkins <jenkins@lttng.org>

diff --git a/src/cli/babeltrace2-cfg-cli-args.c b/src/cli/babeltrace2-cfg-cli-args.c
index 13602d45f57ca58418e569912bdbc6b2fb749202..36b781d3564cb18b830ab61aab2a3563a526683d 100644 (file)
--- a/src/cli/babeltrace2-cfg-cli-args.c
+++ b/src/cli/babeltrace2-cfg-cli-args.c
@@ -3234,6 +3234,9 @@ enum convert_current_item_type {
 
        /* Current item is a component. */
        CONVERT_CURRENT_ITEM_TYPE_COMPONENT,
+
+       /* Current item is a leftover. */
+       CONVERT_CURRENT_ITEM_TYPE_LEFTOVER,
 };
 
 /*
@@ -3424,226 +3427,242 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
                char *comp_cls_name = NULL;
                const char *arg;
 
-               if (argpar_item->type != BT_ARGPAR_ITEM_TYPE_OPT) {
-                       continue;
-               }
-
-               argpar_item_opt = (struct bt_argpar_item_opt *) argpar_item;
-               arg = argpar_item_opt->arg;
-
-               switch (argpar_item_opt->descr->id) {
-               case OPT_COMPONENT:
-               {
-                       bt_component_class_type type;
+               if (argpar_item->type == BT_ARGPAR_ITEM_TYPE_OPT) {
+                       argpar_item_opt = (struct bt_argpar_item_opt *) argpar_item;
+                       arg = argpar_item_opt->arg;
 
-                       current_item_type = CONVERT_CURRENT_ITEM_TYPE_COMPONENT;
+                       switch (argpar_item_opt->descr->id) {
+                       case OPT_COMPONENT:
+                       {
+                               bt_component_class_type type;
 
-                       /* Parse the argument */
-                       plugin_comp_cls_names(arg, &name, &plugin_name,
-                               &comp_cls_name, &type);
-                       if (!plugin_name || !comp_cls_name) {
-                               BT_CLI_LOGE_APPEND_CAUSE(
-                                       "Invalid format for --component option's argument:\n    %s",
-                                       arg);
-                               goto error;
-                       }
+                               current_item_type = CONVERT_CURRENT_ITEM_TYPE_COMPONENT;
 
-                       if (name) {
-                               /*
-                                * Name was given by the user, verify it isn't
-                                * taken.
-                                */
-                               if (bt_value_map_has_entry(all_names, name)) {
+                               /* Parse the argument */
+                               plugin_comp_cls_names(arg, &name, &plugin_name,
+                                       &comp_cls_name, &type);
+                               if (!plugin_name || !comp_cls_name) {
                                        BT_CLI_LOGE_APPEND_CAUSE(
-                                               "Duplicate component instance name:\n    %s",
-                                               name);
+                                               "Invalid format for --component option's argument:\n    %s",
+                                               arg);
                                        goto error;
                                }
 
-                               name_gstr = g_string_new(name);
-                               if (!name_gstr) {
+                               if (name) {
+                                       /*
+                                        * Name was given by the user, verify it isn't
+                                        * taken.
+                                        */
+                                       if (bt_value_map_has_entry(all_names, name)) {
+                                               BT_CLI_LOGE_APPEND_CAUSE(
+                                                       "Duplicate component instance name:\n    %s",
+                                                       name);
+                                               goto error;
+                                       }
+
+                                       name_gstr = g_string_new(name);
+                                       if (!name_gstr) {
+                                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                               goto error;
+                                       }
+
+                                       g_string_assign(component_arg_for_run, arg);
+                               } else {
+                                       /* Name not given by user, generate one. */
+                                       name_gstr = get_component_auto_name(arg, all_names);
+                                       if (!name_gstr) {
+                                               goto error;
+                                       }
+
+                                       g_string_printf(component_arg_for_run, "%s:%s",
+                                               name_gstr->str, arg);
+                               }
+
+                               if (bt_value_array_append_string_element(run_args,
+                                               "--component")) {
                                        BT_CLI_LOGE_APPEND_CAUSE_OOM();
                                        goto error;
                                }
 
-                               g_string_assign(component_arg_for_run, arg);
-                       } else {
-                               /* Name not given by user, generate one. */
-                               name_gstr = get_component_auto_name(arg, all_names);
-                               if (!name_gstr) {
+                               if (bt_value_array_append_string_element(run_args,
+                                               component_arg_for_run->str)) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
                                        goto error;
                                }
 
-                               g_string_printf(component_arg_for_run, "%s:%s",
-                                       name_gstr->str, arg);
-                       }
-
-                       if (bt_value_array_append_string_element(run_args,
-                                       "--component")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                               /*
+                                * Remember this name globally, for the uniqueness of
+                                * all component names.
+                                */
+                               if (bt_value_map_insert_entry(all_names,
+                                               name_gstr->str, bt_value_null)) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
 
-                       if (bt_value_array_append_string_element(run_args,
-                                       component_arg_for_run->str)) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
+                               /*
+                                * Remember this name specifically for the type of the
+                                * component. This is to create connection arguments.
+                                *
+                                * The list takes ownership of `name_gstr`.
+                                */
+                               switch (type) {
+                               case BT_COMPONENT_CLASS_TYPE_SOURCE:
+                                       source_names = g_list_append(source_names, name_gstr);
+                                       break;
+                               case BT_COMPONENT_CLASS_TYPE_FILTER:
+                                       filter_names = g_list_append(filter_names, name_gstr);
+                                       break;
+                               case BT_COMPONENT_CLASS_TYPE_SINK:
+                                       sink_names = g_list_append(sink_names, name_gstr);
+                                       break;
+                               default:
+                                       abort();
+                               }
+                               name_gstr = NULL;
+
+                               free(name);
+                               free(plugin_name);
+                               free(comp_cls_name);
+                               name = NULL;
+                               plugin_name = NULL;
+                               comp_cls_name = NULL;
+                               break;
                        }
+                       case OPT_PARAMS:
+                               if (current_item_type != CONVERT_CURRENT_ITEM_TYPE_COMPONENT) {
+                                       BT_CLI_LOGE_APPEND_CAUSE(
+                                               "No current component (--component option) of which to set parameters:\n    %s",
+                                               arg);
+                                       goto error;
+                               }
 
-                       /*
-                        * Remember this name globally, for the uniqueness of
-                        * all component names.
-                        */
-                       if (bt_value_map_insert_entry(all_names,
-                                       name_gstr->str, bt_value_null)) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                               if (bt_value_array_append_string_element(run_args,
+                                               "--params")) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
 
-                       /*
-                        * Remember this name specifically for the type of the
-                        * component. This is to create connection arguments.
-                        *
-                        * The list takes ownership of `name_gstr`.
-                        */
-                       switch (type) {
-                       case BT_COMPONENT_CLASS_TYPE_SOURCE:
-                               source_names = g_list_append(source_names, name_gstr);
-                               break;
-                       case BT_COMPONENT_CLASS_TYPE_FILTER:
-                               filter_names = g_list_append(filter_names, name_gstr);
-                               break;
-                       case BT_COMPONENT_CLASS_TYPE_SINK:
-                               sink_names = g_list_append(sink_names, name_gstr);
+                               if (bt_value_array_append_string_element(run_args, arg)) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
                                break;
-                       default:
-                               abort();
-                       }
-                       name_gstr = NULL;
-
-                       free(name);
-                       free(plugin_name);
-                       free(comp_cls_name);
-                       name = NULL;
-                       plugin_name = NULL;
-                       comp_cls_name = NULL;
-                       break;
-               }
-               case OPT_PARAMS:
-                       if (current_item_type != CONVERT_CURRENT_ITEM_TYPE_COMPONENT) {
-                               BT_CLI_LOGE_APPEND_CAUSE("No current component of which to set parameters:\n    %s",
-                                       arg);
-                               goto error;
-                       }
+                       case OPT_LOG_LEVEL:
+                               if (current_item_type != CONVERT_CURRENT_ITEM_TYPE_COMPONENT) {
+                                       BT_CLI_LOGE_APPEND_CAUSE(
+                                               "No current component (--component option) to assign a log level to:\n    %s",
+                                               arg);
+                                       goto error;
+                               }
 
-                       if (bt_value_array_append_string_element(run_args,
-                                       "--params")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                               if (bt_value_array_append_string_element(run_args, "--log-level")) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
 
-                       if (bt_value_array_append_string_element(run_args, arg)) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
-                       break;
-               case OPT_LOG_LEVEL:
-                       if (current_item_type != CONVERT_CURRENT_ITEM_TYPE_COMPONENT) {
-                               BT_CLI_LOGE_APPEND_CAUSE("No current component to assign a log level to:\n    %s",
-                                       arg);
-                               goto error;
-                       }
+                               if (bt_value_array_append_string_element(run_args, arg)) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
 
-                       if (bt_value_array_append_string_element(run_args, "--log-level")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                               break;
+                       case OPT_OMIT_HOME_PLUGIN_PATH:
+                               force_omit_home_plugin_path = true;
 
-                       if (bt_value_array_append_string_element(run_args, arg)) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                               if (bt_value_array_append_string_element(run_args,
+                                               "--omit-home-plugin-path")) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
+                               break;
+                       case OPT_RETRY_DURATION:
+                               if (bt_value_array_append_string_element(run_args,
+                                               "--retry-duration")) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
 
-                       break;
-               case OPT_OMIT_HOME_PLUGIN_PATH:
-                       force_omit_home_plugin_path = true;
+                               if (bt_value_array_append_string_element(run_args, arg)) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
+                               break;
+                       case OPT_OMIT_SYSTEM_PLUGIN_PATH:
+                               force_omit_system_plugin_path = true;
 
-                       if (bt_value_array_append_string_element(run_args,
-                                       "--omit-home-plugin-path")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
-                       break;
-               case OPT_RETRY_DURATION:
-                       if (bt_value_array_append_string_element(run_args,
-                                       "--retry-duration")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                               if (bt_value_array_append_string_element(run_args,
+                                               "--omit-system-plugin-path")) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
+                               break;
+                       case OPT_PLUGIN_PATH:
+                               if (bt_config_append_plugin_paths_check_setuid_setgid(
+                                               plugin_paths, arg)) {
+                                       goto error;
+                               }
 
-                       if (bt_value_array_append_string_element(run_args, arg)) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
-                       break;
-               case OPT_OMIT_SYSTEM_PLUGIN_PATH:
-                       force_omit_system_plugin_path = true;
+                               if (bt_value_array_append_string_element(run_args,
+                                               "--plugin-path")) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
 
-                       if (bt_value_array_append_string_element(run_args,
-                                       "--omit-system-plugin-path")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
-                       break;
-               case OPT_PLUGIN_PATH:
-                       if (bt_config_append_plugin_paths_check_setuid_setgid(
-                                       plugin_paths, arg)) {
+                               if (bt_value_array_append_string_element(run_args, arg)) {
+                                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
+                                       goto error;
+                               }
+                               break;
+                       case OPT_BEGIN:
+                       case OPT_CLOCK_CYCLES:
+                       case OPT_CLOCK_DATE:
+                       case OPT_CLOCK_FORCE_CORRELATE:
+                       case OPT_CLOCK_GMT:
+                       case OPT_CLOCK_OFFSET:
+                       case OPT_CLOCK_OFFSET_NS:
+                       case OPT_CLOCK_SECONDS:
+                       case OPT_COLOR:
+                       case OPT_DEBUG:
+                       case OPT_DEBUG_INFO:
+                       case OPT_DEBUG_INFO_DIR:
+                       case OPT_DEBUG_INFO_FULL_PATH:
+                       case OPT_DEBUG_INFO_TARGET_PREFIX:
+                       case OPT_END:
+                       case OPT_FIELDS:
+                       case OPT_INPUT_FORMAT:
+                       case OPT_NAMES:
+                       case OPT_NO_DELTA:
+                       case OPT_OUTPUT_FORMAT:
+                       case OPT_OUTPUT:
+                       case OPT_RUN_ARGS:
+                       case OPT_RUN_ARGS_0:
+                       case OPT_STREAM_INTERSECTION:
+                       case OPT_TIMERANGE:
+                       case OPT_VERBOSE:
+                               /* Ignore in this pass */
+                               break;
+                       default:
+                               BT_CLI_LOGE_APPEND_CAUSE("Unknown command-line option specified (option code %d).",
+                                       argpar_item_opt->descr->id);
                                goto error;
                        }
+               } else if (argpar_item->type == BT_ARGPAR_ITEM_TYPE_NON_OPT) {
+                       struct bt_argpar_item_non_opt *argpar_item_non_opt;
+                       bt_value_array_append_element_status append_status;
 
-                       if (bt_value_array_append_string_element(run_args,
-                                       "--plugin-path")) {
-                               BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                               goto error;
-                       }
+                       current_item_type = CONVERT_CURRENT_ITEM_TYPE_LEFTOVER;
+
+                       argpar_item_non_opt = (struct bt_argpar_item_non_opt *) argpar_item;
 
-                       if (bt_value_array_append_string_element(run_args, arg)) {
+                       append_status = bt_value_array_append_string_element(leftovers,
+                               argpar_item_non_opt->arg);
+                       if (append_status != BT_VALUE_ARRAY_APPEND_ELEMENT_STATUS_OK) {
                                BT_CLI_LOGE_APPEND_CAUSE_OOM();
                                goto error;
                        }
-                       break;
-               case OPT_BEGIN:
-               case OPT_CLOCK_CYCLES:
-               case OPT_CLOCK_DATE:
-               case OPT_CLOCK_FORCE_CORRELATE:
-               case OPT_CLOCK_GMT:
-               case OPT_CLOCK_OFFSET:
-               case OPT_CLOCK_OFFSET_NS:
-               case OPT_CLOCK_SECONDS:
-               case OPT_COLOR:
-               case OPT_DEBUG:
-               case OPT_DEBUG_INFO:
-               case OPT_DEBUG_INFO_DIR:
-               case OPT_DEBUG_INFO_FULL_PATH:
-               case OPT_DEBUG_INFO_TARGET_PREFIX:
-               case OPT_END:
-               case OPT_FIELDS:
-               case OPT_INPUT_FORMAT:
-               case OPT_NAMES:
-               case OPT_NO_DELTA:
-               case OPT_OUTPUT_FORMAT:
-               case OPT_OUTPUT:
-               case OPT_RUN_ARGS:
-               case OPT_RUN_ARGS_0:
-               case OPT_STREAM_INTERSECTION:
-               case OPT_TIMERANGE:
-               case OPT_VERBOSE:
-                       /* Ignore in this pass */
-                       break;
-               default:
-                       BT_CLI_LOGE_APPEND_CAUSE("Unknown command-line option specified (option code %d).",
-                               argpar_item_opt->descr->id);
-                       goto error;
+               } else {
+                       abort();
                }
        }
 
@@ -3964,25 +3983,6 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[],
                goto error;
        }
 
-       /* Consume and keep leftover arguments */
-       for (i = 0; i < argpar_parse_ret.items->len; i++) {
-               struct bt_argpar_item *argpar_item =
-                       g_ptr_array_index(argpar_parse_ret.items, i);
-               struct bt_argpar_item_non_opt *argpar_item_non_opt;
-
-               if (argpar_item->type != BT_ARGPAR_ITEM_TYPE_NON_OPT) {
-                       continue;
-               }
-
-               argpar_item_non_opt = (struct bt_argpar_item_non_opt *) argpar_item;
-
-               if (bt_value_array_append_string_element(leftovers, argpar_item_non_opt->arg) !=
-                               BT_VALUE_ARRAY_APPEND_ELEMENT_STATUS_OK) {
-                       BT_CLI_LOGE_APPEND_CAUSE_OOM();
-                       goto error;
-               }
-       }
-
        /* Print CTF metadata or print LTTng live sessions */
        if (print_ctf_metadata) {
                const bt_value *bt_val_leftover;

diff --git a/tests/cli/test_convert_args b/tests/cli/test_convert_args
index 475f9052a2c664e22c1975118381ff47f988a74b..74c32252fe30daa7a6d6aa4fe475fc7f522723fd 100755 (executable)
--- a/tests/cli/test_convert_args
+++ b/tests/cli/test_convert_args
@@ -27,6 +27,7 @@ fi
 source "$UTILSSH"
 
 test_head_comment=0
+tmp_stderr=$(mktemp)
 
 test_bt_convert_run_args() {
        local what="$1"
@@ -54,9 +55,10 @@ test_bt_convert_run_args() {
 test_bt_convert_fails() {
        local what="$1"
        local convert_args="$2"
+       local expected_error_str="${3:-}"
 
        # execute convert command
-       "$BT_TESTS_BT2_BIN" convert --run-args $convert_args >/dev/null 2>&1
+       "$BT_TESTS_BT2_BIN" convert --run-args $convert_args >/dev/null 2>"${tmp_stderr}"
 
        local status=$?
 
@@ -71,6 +73,16 @@ test_bt_convert_fails() {
        else
                pass "FAILS: $what"
        fi
+
+       if [ -n "${expected_error_str}" ]; then
+               grep --quiet "$expected_error_str" "$tmp_stderr"
+               status=$?
+               ok "$status" "FAILS: $what, error message"
+               if [ "$status" -ne 0 ]; then
+                       diag "Expected error string '${expected_error_str}' not found in stderr:"
+                       diag "$(cat ${tmp_stderr})"
+               fi
+       fi
 }
 
 comment() {
@@ -90,7 +102,7 @@ if [ "$BT_OS_TYPE" = "mingw" ]; then
        output_path=$(cygpath -m "$output_path")
 fi
 
-plan_tests 71
+plan_tests 75
 
 test_bt_convert_run_args 'path leftover' "$path_to_trace" "--component auto-disc-source-ctf-fs:source.ctf.fs --params 'inputs=[\"$path_to_trace\"]' --component pretty:sink.text.pretty --component muxer:filter.utils.muxer --connect auto-disc-source-ctf-fs:muxer --connect muxer:pretty"
 test_bt_convert_run_args 'path leftovers' "$path_to_trace $path_to_trace2" "--component auto-disc-source-ctf-fs:source.ctf.fs --params 'inputs=[\"$path_to_trace\", \"${path_to_trace2}\"]' --component pretty:sink.text.pretty --component muxer:filter.utils.muxer --connect auto-disc-source-ctf-fs:muxer --connect muxer:pretty"
@@ -164,3 +176,7 @@ test_bt_convert_fails 'no source' '-o text'
 test_bt_convert_fails '-o ctf without --output' 'my-trace -o ctf'
 test_bt_convert_fails '-o ctf + --output with implicit sink.text.pretty' "my-trace -o ctf --output $output_path --no-delta"
 test_bt_convert_fails '--stream-intersection' "$path_to_trace --stream-intersection"
+test_bt_convert_fails '--params after leftover' "--component src.ctf.fs $path_to_trace --params=a=2" 'No current component (--component option) of which to set parameters'
+test_bt_convert_fails '--log-level after leftover' "--component src.ctf.fs $path_to_trace --log-level=W" 'No current component (--component option) to assign a log level to'
+
+rm -f "${tmp_stderr}"