From 1ead9076477cc6be07b0c48ce02fa40e49ddc2f0 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 31 Jul 2019 17:40:56 -0400 Subject: [PATCH] cli: apply parameters (`--params` option) to leftovers It is currently not possible to apply --params after a leftover. This patch proposes a way to allow it and to deal with the ambiguity that it poses. In the simple case, we have: babeltrace2 my-traces --params=foo=2 All source components auto-discovered from the `my-traces` leftover will receive the `foo=2` parameter. If we have more than one leftover, but _no_ cross-leftover grouping, then it is also intuitive: babeltrace2 my-traces-1 --params=foo=2 my-traces-2 --params=bar=3 ... all source components discovered from `my-traces-1` will receive `foo=2` and all source components discovered from `my-traces-2` will receive `bar=3`. It becomes less obvious when components are given inputs coming from multiple leftovers (because of the auto-discovery grouping feature): which parameters do they receive? For example, if the following line: babeltrace2 my-traces-1 --params=foo=2,bar=3 my-traces-2 --params=foo=4 leads to these components getting instantiated, with these inputs: * Source component X with inputs `my-traces-1/x` and `my-traces-2/x`. * Source component Y with input `my-traces-1/y` In this case, each component receives the parameters of all leftovers that contributed to its inputs, in the same order as they are provided on the command line. The resulting `run` command line for the example above could therefore look like: ... --component x:src.my.comp --params=foo=2,bar=3 --params=foo=4 \ --component y:src.my.comp --params=foo=2,bar=3 resulting in these parameters being passed to the components: * Source component X receives parameters `foo=4,bar=3` * Source component Y receives parameters `foo=2,bar=3` Implementation details ---------------------- The auto discovery mechanism now returns, for each result, which input from the passed `inputs` array contributed to that result. This allows us, for the component that we create from a given result, to get the parameters from the leftovers that have contributed to it. Change-Id: Ic048e4e137c2e1f93b6da13a62629343500cb75a Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/1809 Tested-by: jenkins Reviewed-by: Philippe Proulx --- src/cli/babeltrace2-cfg-cli-args.c | 128 +++++++++++++++--- src/cli/babeltrace2-cfg-src-auto-disc.c | 75 ++++++++-- src/cli/babeltrace2-cfg-src-auto-disc.h | 6 + .../convert/test_auto_source_discovery_params | 87 ++++++++++++ tests/cli/convert/test_convert_args | 3 +- .../bt_plugin_test.py | 65 +++++++++ .../auto-source-discovery-params/dir-a/aaa | 0 .../auto-source-discovery-params/dir-ab/aaa | 0 .../auto-source-discovery-params/dir-ab/bbb | 0 .../auto-source-discovery-params/dir-b/bbb | 0 10 files changed, 330 insertions(+), 34 deletions(-) create mode 100755 tests/cli/convert/test_auto_source_discovery_params create mode 100644 tests/data/cli/convert/auto-source-discovery-params/bt_plugin_test.py create mode 100644 tests/data/cli/convert/auto-source-discovery-params/dir-a/aaa create mode 100644 tests/data/cli/convert/auto-source-discovery-params/dir-ab/aaa create mode 100644 tests/data/cli/convert/auto-source-discovery-params/dir-ab/bbb create mode 100644 tests/data/cli/convert/auto-source-discovery-params/dir-b/bbb diff --git a/src/cli/babeltrace2-cfg-cli-args.c b/src/cli/babeltrace2-cfg-cli-args.c index 36b781d3..a332bf72 100644 --- a/src/cli/babeltrace2-cfg-cli-args.c +++ b/src/cli/babeltrace2-cfg-cli-args.c @@ -3177,11 +3177,24 @@ end: /* * Create `struct implicit_component_args` structures for each of the source * components we identified. Add them to `component_args`. + * + * `leftover_params` is an array where each element is an array of strings + * containing all the arguments to `--params` that apply to the leftover at the + * same index. For example, if, for a leftover, the following `--params` + * options applied: + * + * --params=a=2 --params=b=3,c=4 + * + * its entry in `leftover_params` would contain + * + * ["a=2", "b=3,c=4"] */ static -void create_implicit_component_args_from_auto_discovered_sources( - const struct auto_source_discovery *auto_disc, GPtrArray *component_args) +int create_implicit_component_args_from_auto_discovered_sources( + const struct auto_source_discovery *auto_disc, + const bt_value *leftover_params, + GPtrArray *component_args) { gchar *cc_name = NULL; struct implicit_component_args *comp = NULL; @@ -3193,34 +3206,82 @@ void create_implicit_component_args_from_auto_discovered_sources( for (i = 0; i < len; i++) { struct auto_source_discovery_result *res = g_ptr_array_index(auto_disc->results, i); + uint64_t orig_indices_i, orig_indices_count; g_free(cc_name); cc_name = g_strdup_printf("source.%s.%s", res->plugin_name, res->source_cc_name); if (!cc_name) { BT_CLI_LOGE_APPEND_CAUSE_OOM(); - goto end; + goto error; } comp = create_implicit_component_args(cc_name); if (!comp) { - goto end; + goto error; + } + + /* + * Append parameters of all the leftovers that contributed to + * this component instance coming into existence. + */ + orig_indices_count = bt_value_array_get_size(res->original_input_indices); + for (orig_indices_i = 0; orig_indices_i < orig_indices_count; orig_indices_i++) { + const bt_value *orig_idx_value = + bt_value_array_borrow_element_by_index( + res->original_input_indices, orig_indices_i); + uint64_t orig_idx = bt_value_integer_unsigned_get(orig_idx_value); + const bt_value *params_array = + bt_value_array_borrow_element_by_index_const( + leftover_params, orig_idx); + uint64_t params_i, params_count; + + params_count = bt_value_array_get_size(params_array); + for (params_i = 0; params_i < params_count; params_i++) { + const bt_value *params_value = + bt_value_array_borrow_element_by_index_const( + params_array, params_i); + const char *params = bt_value_string_get(params_value); + bt_value_array_append_element_status append_status; + + append_status = bt_value_array_append_string_element( + comp->extra_params, "--params"); + if (append_status != BT_VALUE_ARRAY_APPEND_ELEMENT_STATUS_OK) { + BT_CLI_LOGE_APPEND_CAUSE("Failed to append array element."); + goto error; + } + + append_status = bt_value_array_append_string_element( + comp->extra_params, params); + if (append_status != BT_VALUE_ARRAY_APPEND_ELEMENT_STATUS_OK) { + BT_CLI_LOGE_APPEND_CAUSE("Failed to append array element."); + goto error; + } + } } status = append_parameter_to_args(comp->extra_params, "inputs", res->inputs); if (status != 0) { - goto end; + goto error; } g_ptr_array_add(component_args, comp); comp = NULL; } + status = 0; + goto end; + +error: + status = -1; + end: g_free(cc_name); if (comp) { destroy_implicit_component_args(comp); } + + return status; } /* @@ -3269,6 +3330,7 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[], GList *filter_names = NULL; GList *sink_names = NULL; bt_value *leftovers = NULL; + bt_value *leftover_params = NULL; struct implicit_component_args implicit_ctf_output_args = { 0 }; struct implicit_component_args implicit_lttng_live_args = { 0 }; struct implicit_component_args implicit_dummy_args = { 0 }; @@ -3377,6 +3439,12 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[], goto error; } + leftover_params = bt_value_array_create(); + if (!leftover_params) { + BT_CLI_LOGE_APPEND_CAUSE_OOM(); + goto error; + } + if (auto_source_discovery_init(&auto_disc) != 0) { goto error; } @@ -3530,21 +3598,32 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[], 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; - } + if (current_item_type == CONVERT_CURRENT_ITEM_TYPE_COMPONENT) { + /* + * The current item is a component (--component option), + * pass it directly to the run args. + */ + 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, - "--params")) { - 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; + } + } else if (current_item_type == CONVERT_CURRENT_ITEM_TYPE_LEFTOVER) { + /* The current item is a leftover, record it in `leftover_params`. */ + bt_value *array; + uint64_t idx = bt_value_array_get_size(leftover_params) - 1; - if (bt_value_array_append_string_element(run_args, arg)) { - BT_CLI_LOGE_APPEND_CAUSE_OOM(); + array = bt_value_array_borrow_element_by_index(leftover_params, idx); + bt_value_array_append_string_element(array, arg); + } else { + BT_CLI_LOGE_APPEND_CAUSE( + "No current component (--component option) or non-option argument of which to set parameters:\n %s", + arg); goto error; } break; @@ -3661,6 +3740,12 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[], BT_CLI_LOGE_APPEND_CAUSE_OOM(); goto error; } + + append_status = bt_value_array_append_empty_array_element(leftover_params); + if (append_status != BT_VALUE_ARRAY_APPEND_ELEMENT_STATUS_OK) { + BT_CLI_LOGE_APPEND_CAUSE_OOM(); + goto error; + } } else { abort(); } @@ -4135,8 +4220,11 @@ struct bt_config *bt_config_convert_from_args(int argc, const char *argv[], goto error; } - create_implicit_component_args_from_auto_discovered_sources( - &auto_disc, discovered_source_args); + status = create_implicit_component_args_from_auto_discovered_sources( + &auto_disc, leftover_params, discovered_source_args); + if (status != 0) { + goto error; + } } } diff --git a/src/cli/babeltrace2-cfg-src-auto-disc.c b/src/cli/babeltrace2-cfg-src-auto-disc.c index 1ffb1005..50141cf6 100644 --- a/src/cli/babeltrace2-cfg-src-auto-disc.c +++ b/src/cli/babeltrace2-cfg-src-auto-disc.c @@ -70,6 +70,12 @@ struct auto_source_discovery_result *auto_source_discovery_result_create( goto error; } + res->original_input_indices = bt_value_array_create(); + if (!res->original_input_indices) { + BT_CLI_LOGE_APPEND_CAUSE("Failed to allocate an array value."); + goto error; + } + goto end; error: auto_source_discovery_result_destroy(res); @@ -112,6 +118,14 @@ end: return status; } +static +const bt_value *borrow_array_value_last_element_const(const bt_value *array) +{ + uint64_t last_index = bt_value_array_get_size(array) - 1; + + return bt_value_array_borrow_element_by_index_const(array, last_index); +} + /* * Assign `input` to source component class `source_cc_name` of plugin * `plugin_name`, in the group with key `group`. @@ -122,13 +136,15 @@ int auto_source_discovery_add(struct auto_source_discovery *auto_disc, const char *plugin_name, const char *source_cc_name, const char *group, - const char *input) + const char *input, + uint64_t original_input_index) { int status; bt_value_array_append_element_status append_status; guint len; guint i; struct auto_source_discovery_result *res = NULL; + bool append_index; len = auto_disc->results->len; i = len; @@ -170,6 +186,35 @@ int auto_source_discovery_add(struct auto_source_discovery *auto_disc, goto error; } + /* + * Append `original_input_index` to `original_input_indices` if not + * there already. We process the `inputs` array in order, so if it is + * present, it has to be the last element. + */ + if (bt_value_array_is_empty(res->original_input_indices)) { + append_index = true; + } else { + const bt_value *last_index_value; + uint64_t last_index; + + last_index_value = + borrow_array_value_last_element_const(res->original_input_indices); + last_index = bt_value_integer_unsigned_get(last_index_value); + + BT_ASSERT(last_index <= original_input_index); + + append_index = (last_index != original_input_index); + } + + if (append_index) { + append_status = bt_value_array_append_unsigned_integer_element( + res->original_input_indices, original_input_index); + + if (append_status != BT_VALUE_ARRAY_APPEND_ELEMENT_STATUS_OK) { + BT_CLI_LOGE_APPEND_CAUSE("Failed to append an unsigned integer value."); + goto error; + } + } status = 0; goto end; @@ -242,7 +287,9 @@ end: */ static int support_info_query_all_sources(const char *input, - const char *input_type, size_t plugin_count, + const char *input_type, + uint64_t original_input_index, + size_t plugin_count, const char *plugin_restrict, const char *component_class_restrict, enum bt_logging_level log_level, @@ -439,7 +486,8 @@ int support_info_query_all_sources(const char *input, BT_LOGI("Input %s is awarded to component class source.%s.%s with weight %f", input, plugin_name, source_name, winner.weigth); - status = auto_source_discovery_add(auto_disc, plugin_name, source_name, group, input); + status = auto_source_discovery_add(auto_disc, plugin_name, + source_name, group, input, original_input_index); if (status != 0) { goto error; } @@ -471,18 +519,20 @@ end: static int auto_discover_source_for_input_as_string(const char *input, + uint64_t original_input_index, size_t plugin_count, const char *plugin_restrict, const char *component_class_restrict, enum bt_logging_level log_level, struct auto_source_discovery *auto_disc) { return support_info_query_all_sources(input, "string", - plugin_count, plugin_restrict, component_class_restrict, - log_level, auto_disc); + original_input_index, plugin_count, plugin_restrict, + component_class_restrict, log_level, auto_disc); } static int auto_discover_source_for_input_as_dir_or_file_rec(GString *input, + uint64_t original_input_index, size_t plugin_count, const char *plugin_restrict, const char *component_class_restrict, enum bt_logging_level log_level, @@ -494,7 +544,7 @@ int auto_discover_source_for_input_as_dir_or_file_rec(GString *input, if (g_file_test(input->str, G_FILE_TEST_IS_REGULAR)) { /* It's a file. */ status = support_info_query_all_sources(input->str, - "file", plugin_count, + "file", original_input_index, plugin_count, plugin_restrict, component_class_restrict, log_level, auto_disc); } else if (g_file_test(input->str, G_FILE_TEST_IS_DIR)) { GDir *dir; @@ -504,7 +554,7 @@ int auto_discover_source_for_input_as_dir_or_file_rec(GString *input, /* It's a directory. */ status = support_info_query_all_sources(input->str, - "directory", plugin_count, + "directory", original_input_index, plugin_count, plugin_restrict, component_class_restrict, log_level, auto_disc); @@ -545,7 +595,7 @@ int auto_discover_source_for_input_as_dir_or_file_rec(GString *input, g_string_append(input, dirent); status = auto_discover_source_for_input_as_dir_or_file_rec( - input, plugin_count, + input, original_input_index, plugin_count, plugin_restrict, component_class_restrict, log_level, auto_disc); @@ -595,6 +645,7 @@ end: static int auto_discover_source_for_input_as_dir_or_file(const char *input, + uint64_t original_input_index, size_t plugin_count, const char *plugin_restrict, const char *component_class_restrict, enum bt_logging_level log_level, @@ -610,7 +661,7 @@ int auto_discover_source_for_input_as_dir_or_file(const char *input, } status = auto_discover_source_for_input_as_dir_or_file_rec( - mutable_input, plugin_count, plugin_restrict, + mutable_input, original_input_index, plugin_count, plugin_restrict, component_class_restrict, log_level, auto_disc); g_string_free(mutable_input, TRUE); @@ -645,7 +696,7 @@ int auto_discover_source_components( input_value = bt_value_array_borrow_element_by_index_const(inputs, i_inputs); input = bt_value_string_get(input_value); - status = auto_discover_source_for_input_as_string(input, + status = auto_discover_source_for_input_as_string(input, i_inputs, plugin_count, plugin_restrict, component_class_restrict, log_level, auto_disc); if (status < 0) { @@ -657,8 +708,8 @@ int auto_discover_source_components( } status = auto_discover_source_for_input_as_dir_or_file(input, - plugin_count, plugin_restrict, component_class_restrict, - log_level, auto_disc); + i_inputs, plugin_count, plugin_restrict, + component_class_restrict, log_level, auto_disc); if (status < 0) { /* Fatal error. */ goto end; diff --git a/src/cli/babeltrace2-cfg-src-auto-disc.h b/src/cli/babeltrace2-cfg-src-auto-disc.h index 0d85c05e..d6192cd9 100644 --- a/src/cli/babeltrace2-cfg-src-auto-disc.h +++ b/src/cli/babeltrace2-cfg-src-auto-disc.h @@ -51,6 +51,12 @@ struct auto_source_discovery_result { /* Array of input strings. */ bt_value *inputs; + + /* + * Array of integers: indices of the original inputs that contributed + * to this result. + */ + bt_value *original_input_indices; }; int auto_source_discovery_init(struct auto_source_discovery *auto_disc); diff --git a/tests/cli/convert/test_auto_source_discovery_params b/tests/cli/convert/test_auto_source_discovery_params new file mode 100755 index 00000000..3a8837e6 --- /dev/null +++ b/tests/cli/convert/test_auto_source_discovery_params @@ -0,0 +1,87 @@ +#!/bin/bash +# +# Copyright (C) 2019 Simon Marchi +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License, version 2 only, as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# this program; if not, write to the Free Software Foundation, Inc., 51 +# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +# Test how parameters are applied to sources auto-discovered by the convert +# command. + +if [ "x${BT_TESTS_SRCDIR:-}" != "x" ]; then + UTILSSH="$BT_TESTS_SRCDIR/utils/utils.sh" +else + UTILSSH="$(dirname "$0")/../../utils/utils.sh" +fi + +# shellcheck source=../../utils/utils.sh +SH_TAP=1 source "$UTILSSH" + +NUM_TESTS=4 + +plan_tests $NUM_TESTS + +data_dir="${BT_TESTS_DATADIR}/cli/convert/auto-source-discovery-params" +plugin_dir="${data_dir}" +dir_a="${data_dir}/dir-a" +dir_b="${data_dir}/dir-b" +dir_ab="${data_dir}/dir-ab" + +expected_file=$(mktemp -t expected.XXXXXX) +stderr_expected=/dev/null + +# Apply params to two components from one leftover. +cat > "$expected_file" < "$expected_file" < "$expected_file" < "$expected_file" <