From 8b4cd08a4cc2e13235c20689d74790ed3ccc6a51 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 29 Jul 2019 14:42:23 -0400 Subject: [PATCH] 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 Reviewed-on: https://review.lttng.org/c/babeltrace/+/1806 Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/cli/babeltrace2-cfg-cli-args.c | 410 ++++++++++++++--------------- tests/cli/test_convert_args | 20 +- 2 files changed, 223 insertions(+), 207 deletions(-) diff --git a/src/cli/babeltrace2-cfg-cli-args.c b/src/cli/babeltrace2-cfg-cli-args.c index 13602d45..36b781d3 100644 --- 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 475f9052..74c32252 100755 --- 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}" -- 2.34.1