From b5cd7d6593ad0dba510172268777a521442387d7 Mon Sep 17 00:00:00 2001 From: Francis Deslauriers Date: Tue, 29 Oct 2019 17:45:09 -0400 Subject: [PATCH] cli: exit with status 2 when interrupted by SIGINT With this commit, users of the CLI issuing a ctrl+c to interrupt a running graph will not get an error cause stack printed to stderr. Instead, they will now simply get a non-zero exit status to signify that the execution of the command did not complete as expected. We preferred this approach, because we consider it unexpected that a user who willingly pressed ctrl+c to stop the command ends up getting an error cause stack printed to the CLI. As the execution did not complete normally, we also did not want the command to exit with status 0 in such cases. Exiting with status code 1 was also not appropriate as it is typically used to signify an error. In sum, the middle ground we found, is to exit with status code 2 and not print an error message. This commit adds tests to confirm that the right exit status is produced by the CLI when a graph is interrupted, is erroring, or is returning successfully. To do this, this commit adds a custom Python source component class that takes as parameter the name of the scenario to produce during its iterator's first `_next()` call. The iterator then does the needful to put the graph in the right condition. We then test if the CLI exits with the expected status. Signed-off-by: Francis Deslauriers Change-Id: I82a451b24240be6fb2256ce681685ba02b73600f Reviewed-on: https://review.lttng.org/c/babeltrace/+/2308 Reviewed-by: Simon Marchi Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/cli/babeltrace2.c | 169 ++++++++++-------- tests/Makefile.am | 4 +- tests/cli/test_exit_status | 93 ++++++++++ .../bt_plugin_test_cli_exit_status.py | 42 +++++ 4 files changed, 237 insertions(+), 71 deletions(-) create mode 100755 tests/cli/test_exit_status create mode 100644 tests/data/cli/exit_status/bt_plugin_test_cli_exit_status.py diff --git a/src/cli/babeltrace2.c b/src/cli/babeltrace2.c index a59bb71c..bb0411a6 100644 --- a/src/cli/babeltrace2.c +++ b/src/cli/babeltrace2.c @@ -45,6 +45,27 @@ #define ENV_BABELTRACE_WARN_COMMAND_NAME_DIRECTORY_CLASH "BABELTRACE_CLI_WARN_COMMAND_NAME_DIRECTORY_CLASH" #define NSEC_PER_SEC 1000000000LL +enum bt_cmd_status { + BT_CMD_STATUS_OK = 0, + BT_CMD_STATUS_ERROR = -1, + BT_CMD_STATUS_INTERRUPTED = -2, +}; + +static +const char *bt_cmd_status_string(enum bt_cmd_status cmd_status) +{ + switch (cmd_status) { + case BT_CMD_STATUS_OK: + return "OK"; + case BT_CMD_STATUS_ERROR: + return "ERROR"; + case BT_CMD_STATUS_INTERRUPTED: + return "INTERRUPTED"; + default: + bt_common_abort(); + } +} + /* Application's interrupter (owned by this) */ static bt_interrupter *the_interrupter; @@ -670,9 +691,10 @@ void print_plugin_info(const bt_plugin *plugin) } static -int cmd_query(struct bt_config *cfg) +enum bt_cmd_status cmd_query(struct bt_config *cfg) { int ret = 0; + enum bt_cmd_status cmd_status; const bt_component_class *comp_cls = NULL; const bt_value *results = NULL; const char *fail_reason = NULL; @@ -688,8 +710,7 @@ int cmd_query(struct bt_config *cfg) cfg->cmd_data.query.cfg_component->plugin_name->str, cfg->cmd_data.query.cfg_component->comp_cls_name->str, cfg->cmd_data.query.cfg_component->type); - ret = -1; - goto end; + goto error; } ret = query(cfg, comp_cls, cfg->cmd_data.query.object->str, @@ -700,6 +721,7 @@ int cmd_query(struct bt_config *cfg) } print_value(stdout, results, 0); + cmd_status = BT_CMD_STATUS_OK; goto end; failed: @@ -711,12 +733,13 @@ failed: cfg->cmd_data.query.cfg_component->comp_cls_name->str, cfg->cmd_data.query.cfg_component->type, cfg->cmd_data.query.object->str); - ret = -1; +error: + cmd_status = BT_CMD_STATUS_ERROR; end: bt_component_class_put_ref(comp_cls); bt_value_put_ref(results); - return ret; + return cmd_status; } static @@ -744,9 +767,9 @@ void print_component_class_help(const char *plugin_name, } static -int cmd_help(struct bt_config *cfg) +enum bt_cmd_status cmd_help(struct bt_config *cfg) { - int ret = 0; + enum bt_cmd_status cmd_status; const bt_plugin *plugin = NULL; const bt_component_class *needed_comp_cls = NULL; @@ -755,8 +778,7 @@ int cmd_help(struct bt_config *cfg) BT_CLI_LOGE_APPEND_CAUSE( "Cannot find plugin: plugin-name=\"%s\"", cfg->cmd_data.help.cfg_component->plugin_name->str); - ret = -1; - goto end; + goto error; } print_plugin_info(plugin); @@ -775,6 +797,7 @@ int cmd_help(struct bt_config *cfg) if (strlen(cfg->cmd_data.help.cfg_component->comp_cls_name->str) == 0) { /* Plugin help only */ + cmd_status = BT_CMD_STATUS_OK; goto end; } @@ -789,19 +812,23 @@ int cmd_help(struct bt_config *cfg) cfg->cmd_data.help.cfg_component->plugin_name->str, cfg->cmd_data.help.cfg_component->comp_cls_name->str, cfg->cmd_data.help.cfg_component->type); - ret = -1; - goto end; + goto error; } printf("\n"); print_component_class_help( cfg->cmd_data.help.cfg_component->plugin_name->str, needed_comp_cls); + cmd_status = BT_CMD_STATUS_OK; + goto end; + +error: + cmd_status = BT_CMD_STATUS_ERROR; end: bt_component_class_put_ref(needed_comp_cls); bt_plugin_put_ref(plugin); - return ret; + return cmd_status; } typedef void *(* plugin_borrow_comp_cls_by_index_func_t)(const bt_plugin *, @@ -858,9 +885,8 @@ end: } static -int cmd_list_plugins(struct bt_config *cfg) +enum bt_cmd_status cmd_list_plugins(struct bt_config *cfg) { - int ret = 0; int plugins_count, component_classes_count = 0, i; printf("From the following plugin paths:\n\n"); @@ -915,13 +941,14 @@ int cmd_list_plugins(struct bt_config *cfg) } end: - return ret; + return BT_CMD_STATUS_OK; } static -int cmd_print_lttng_live_sessions(struct bt_config *cfg) +enum bt_cmd_status cmd_print_lttng_live_sessions(struct bt_config *cfg) { int ret = 0; + enum bt_cmd_status cmd_status; const bt_component_class *comp_cls = NULL; const bt_value *results = NULL; bt_value *params = NULL; @@ -977,7 +1004,6 @@ int cmd_print_lttng_live_sessions(struct bt_config *cfg) fopen(cfg->cmd_data.print_lttng_live_sessions.output_path->str, "w"); if (!out_stream) { - ret = -1; BT_LOGE_ERRNO("Cannot open file for writing", ": path=\"%s\"", cfg->cmd_data.print_lttng_live_sessions.output_path->str); @@ -985,7 +1011,7 @@ int cmd_print_lttng_live_sessions(struct bt_config *cfg) "Babeltrace CLI", "Cannot open file for writing: path=\"%s\"", cfg->cmd_data.print_lttng_live_sessions.output_path->str); - goto end; + goto error; } } @@ -1032,6 +1058,7 @@ int cmd_print_lttng_live_sessions(struct bt_config *cfg) fprintf(out_stream, "%" PRIu64 " client(s) connected)\n", clients); } + cmd_status = BT_CMD_STATUS_OK; goto end; failed: @@ -1039,7 +1066,7 @@ failed: fail_reason); error: - ret = -1; + cmd_status = BT_CMD_STATUS_ERROR; end: bt_value_put_ref(results); @@ -1056,13 +1083,14 @@ end: } } - return ret; + return cmd_status; } static -int cmd_print_ctf_metadata(struct bt_config *cfg) +enum bt_cmd_status cmd_print_ctf_metadata(struct bt_config *cfg) { int ret = 0; + enum bt_cmd_status cmd_status; const bt_component_class *comp_cls = NULL; const bt_value *results = NULL; bt_value *params = NULL; @@ -1084,21 +1112,18 @@ int cmd_print_ctf_metadata(struct bt_config *cfg) "comp-cls-name=\"%s\", comp-cls-type=%d", plugin_name, comp_cls_name, BT_COMPONENT_CLASS_TYPE_SOURCE); - ret = -1; - goto end; + goto error; } params = bt_value_map_create(); if (!params) { - ret = -1; - goto end; + goto error; } ret = bt_value_map_insert_string_entry(params, "path", cfg->cmd_data.print_ctf_metadata.path->str); if (ret) { - ret = -1; - goto end; + goto error; } ret = query(cfg, comp_cls, "metadata-info", @@ -1112,8 +1137,7 @@ int cmd_print_ctf_metadata(struct bt_config *cfg) if (!metadata_text_value) { BT_CLI_LOGE_APPEND_CAUSE( "Cannot find `text` string value in the resulting metadata info object."); - ret = -1; - goto end; + goto error; } metadata_text = bt_value_string_get(metadata_text_value); @@ -1130,8 +1154,7 @@ int cmd_print_ctf_metadata(struct bt_config *cfg) "Babeltrace CLI", "Cannot open file for writing: path=\"%s\"", cfg->cmd_data.print_ctf_metadata.output_path->str); - ret = -1; - goto end; + goto error; } } @@ -1140,16 +1163,17 @@ int cmd_print_ctf_metadata(struct bt_config *cfg) BT_CLI_LOGE_APPEND_CAUSE( "Cannot write whole metadata text to output stream: " "ret=%d", ret); - goto end; + goto error; } - ret = 0; + cmd_status = BT_CMD_STATUS_OK; goto end; failed: - ret = -1; BT_CLI_LOGE_APPEND_CAUSE( "Failed to query `metadata-info` object: %s", fail_reason); +error: + cmd_status = BT_CMD_STATUS_ERROR; end: bt_value_put_ref(results); @@ -1166,7 +1190,7 @@ end: } } - return ret; + return cmd_status; } struct port_id { @@ -2477,9 +2501,9 @@ end: } static -int cmd_run(struct bt_config *cfg) +enum bt_cmd_status cmd_run(struct bt_config *cfg) { - int ret = 0; + enum bt_cmd_status cmd_status; struct cmd_run_ctx ctx = { 0 }; /* Initialize the command's context and the graph object */ @@ -2536,12 +2560,15 @@ int cmd_run(struct bt_config *cfg) switch (run_status) { case BT_GRAPH_RUN_STATUS_OK: + cmd_status = BT_CMD_STATUS_OK; goto end; case BT_GRAPH_RUN_STATUS_AGAIN: if (bt_interrupter_is_set(the_interrupter)) { - BT_CLI_LOGW_APPEND_CAUSE( - "Graph was interrupted by user."); - goto error; + /* + * The graph was interrupted by a SIGINT. + */ + cmd_status = BT_CMD_STATUS_INTERRUPTED; + goto end; } if (cfg->cmd_data.run.retry_duration_us > 0) { @@ -2551,20 +2578,16 @@ int cmd_run(struct bt_config *cfg) if (usleep(cfg->cmd_data.run.retry_duration_us)) { if (bt_interrupter_is_set(the_interrupter)) { - BT_CLI_LOGW_APPEND_CAUSE( - "Graph was interrupted by user."); - goto error; + cmd_status = BT_CMD_STATUS_INTERRUPTED; + goto end; } } } break; default: if (bt_interrupter_is_set(the_interrupter)) { - BT_CLI_LOGW_APPEND_CAUSE( - "Graph was interrupted by user and failed: " - "status=%s", - bt_common_func_status_string(run_status)); - goto error; + cmd_status = BT_CMD_STATUS_INTERRUPTED; + goto end; } BT_CLI_LOGE_APPEND_CAUSE( @@ -2572,17 +2595,12 @@ int cmd_run(struct bt_config *cfg) goto error; } } - - goto end; - error: - if (ret == 0) { - ret = -1; - } + cmd_status = BT_CMD_STATUS_ERROR; end: cmd_run_ctx_destroy(&ctx); - return ret; + return cmd_status; } static @@ -2735,8 +2753,8 @@ end: int main(int argc, const char **argv) { - int ret; - int retcode; + int ret, retcode; + enum bt_cmd_status cmd_status; struct bt_config *cfg = NULL; init_log_level(); @@ -2791,42 +2809,53 @@ int main(int argc, const char **argv) switch (cfg->command) { case BT_CONFIG_COMMAND_RUN: - ret = cmd_run(cfg); + cmd_status = cmd_run(cfg); break; case BT_CONFIG_COMMAND_LIST_PLUGINS: - ret = cmd_list_plugins(cfg); + cmd_status = cmd_list_plugins(cfg); break; case BT_CONFIG_COMMAND_HELP: - ret = cmd_help(cfg); + cmd_status = cmd_help(cfg); break; case BT_CONFIG_COMMAND_QUERY: - ret = cmd_query(cfg); + cmd_status = cmd_query(cfg); break; case BT_CONFIG_COMMAND_PRINT_CTF_METADATA: - ret = cmd_print_ctf_metadata(cfg); + cmd_status = cmd_print_ctf_metadata(cfg); break; case BT_CONFIG_COMMAND_PRINT_LTTNG_LIVE_SESSIONS: - ret = cmd_print_lttng_live_sessions(cfg); + cmd_status = cmd_print_lttng_live_sessions(cfg); break; default: BT_LOGF("Invalid/unknown command: cmd=%d", cfg->command); bt_common_abort(); } - BT_LOGI("Command completed: cmd=%d, command-name=\"%s\", ret=%d", - cfg->command, cfg->command_name, ret); + BT_LOGI("Command completed: cmd=%d, command-name=\"%s\", command-status=\"%s\"", + cfg->command, cfg->command_name, bt_cmd_status_string(cmd_status)); warn_command_name_and_directory_clash(cfg); - retcode = ret ? 1 : 0; + + switch (cmd_status) { + case BT_CMD_STATUS_OK: + retcode = 0; + break; + case BT_CMD_STATUS_ERROR: + retcode = 1; + print_error_causes(); + break; + case BT_CMD_STATUS_INTERRUPTED: + retcode = 2; + break; + default: + BT_LOGF("Invalid command status: cmd-status=%d", cmd_status); + bt_common_abort(); + } end: BT_OBJECT_PUT_REF_AND_RESET(cfg); fini_loaded_plugins(); bt_interrupter_put_ref(the_interrupter); - if (retcode != 0) { - print_error_causes(); - } - /* * Clear current thread's error in case there is one to avoid a * memory leak. diff --git a/tests/Makefile.am b/tests/Makefile.am index ac7eb887..079e7922 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -42,6 +42,7 @@ dist_check_SCRIPTS = \ cli/convert/test_auto_source_discovery_log_level \ cli/convert/test_convert_args \ cli/params/test_params \ + cli/test_exit_status \ cli/test_help \ cli/test_intersection \ cli/test_output_ctf_metadata \ @@ -124,7 +125,8 @@ TESTS_CLI += \ cli/convert/test_auto_source_discovery_grouping \ cli/convert/test_auto_source_discovery_log_level \ cli/convert/test_auto_source_discovery_params \ - cli/params/test_params + cli/params/test_params \ + cli/test_exit_status TESTS_PLUGINS += plugins/flt.utils.trimmer/test_trimming \ plugins/flt.utils.muxer/succeed/test_succeed diff --git a/tests/cli/test_exit_status b/tests/cli/test_exit_status new file mode 100755 index 00000000..3e6f5029 --- /dev/null +++ b/tests/cli/test_exit_status @@ -0,0 +1,93 @@ +#!/bin/bash +# +# Copyright (C) 2019 EfficiOS Inc. +# +# 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. + +SH_TAP=1 + +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 +source "$UTILSSH" + +data_dir="$BT_TESTS_DATADIR/cli/exit_status" +source_name="src.test_exit_status.StatusSrc" + +test_interrupted_graph() { + local cli_args=("--plugin-path=$data_dir" "-c" "$source_name" "-p" "case=\"INTERRUPTED\"") + local actual_stdout=$(mktemp -t test_cli_exit_status_stdout_actual.XXXXXX) + local actual_stderr=$(mktemp -t test_cli_exit_status_stderr_actual.XXXXXX) + + bt_cli "$actual_stdout" "$actual_stderr" "${cli_args[@]}" + + is $? 2 "Interrupted graph exits with status 2" + + bt_diff /dev/null "$actual_stdout" + ok $? "Interrupted graph gives no stdout" + + bt_diff /dev/null "$actual_stderr" + ok $? "Interrupted graph gives no stderr" + + rm -f "${actual_stdout}" + rm -f "${actual_stderr}" +} + +test_error_graph() { + local cli_args=("--plugin-path=$data_dir" "-c" "$source_name" "-p" "case=\"ERROR\"") + local actual_stdout=$(mktemp -t test_cli_exit_status_stdout_actual.XXXXXX) + local actual_stderr=$(mktemp -t test_cli_exit_status_stderr_actual.XXXXXX) + + bt_cli "$actual_stdout" "$actual_stderr" "${cli_args[@]}" + + is $? 1 "Erroring graph exits with status 1" + + bt_diff /dev/null "$actual_stdout" + ok $? "Erroring graph gives expected stdout" + + like "$(cat "${actual_stderr}")" "TypeError: Raising type error" \ + "Erroring graph gives expected error message" + + rm -f "${actual_stdout}" + rm -f "${actual_stderr}" +} + +test_stop_graph() { + local cli_args=("--plugin-path=$data_dir" "-c" "$source_name" "-p" "case=\"STOP\"") + local actual_stdout=$(mktemp -t test_cli_exit_status_stdout_actual.XXXXXX) + local actual_stderr=$(mktemp -t test_cli_exit_status_stderr_actual.XXXXXX) + + bt_cli "$actual_stdout" "$actual_stderr" "${cli_args[@]}" + + is $? 0 "Successful graph exits with status 0" + + bt_diff /dev/null "$actual_stdout" + ok $? "Successful graph gives no stdout" + + bt_diff /dev/null "$actual_stderr" + ok $? "Successful graph gives no stderr" + + rm -f "${actual_stdout}" + rm -f "${actual_stderr}" +} + +plan_tests 9 + +test_interrupted_graph +test_error_graph +test_stop_graph diff --git a/tests/data/cli/exit_status/bt_plugin_test_cli_exit_status.py b/tests/data/cli/exit_status/bt_plugin_test_cli_exit_status.py new file mode 100644 index 00000000..5367fc9c --- /dev/null +++ b/tests/data/cli/exit_status/bt_plugin_test_cli_exit_status.py @@ -0,0 +1,42 @@ +import bt2 +import signal +import os +import time + +bt2.register_plugin(__name__, "test_exit_status") + + +class StatusIter(bt2._UserMessageIterator): + def __init__(self, config, output_port): + self.case = output_port.user_data['case'] + + def __next__(self): + if self.case == "STOP": + raise bt2.Stop() + if self.case == "INTERRUPTED": + os.kill(os.getpid(), signal.SIGINT) + + # Wait until the graph is in the interrupted state. + timeout_s = 10 + for _ in range(timeout_s * 10): + if self._is_interrupted: + raise bt2.TryAgain() + + time.sleep(0.1) + + raise Exception( + '{} was not interrupted after {} seconds'.format( + self.__class__.__name__, timeout_s + ) + ) + + elif self.case == "ERROR": + raise TypeError("Raising type error") + else: + raise ValueError("Invalid parameter") + + +@bt2.plugin_component_class +class StatusSrc(bt2._UserSourceComponent, message_iterator_class=StatusIter): + def __init__(self, config, params, obj): + self._add_output_port("out", {'case': params['case']}) -- 2.34.1