From 9497e2f771b05ba0cf6ca690f24f73cadc8c6e1d Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 19 Apr 2023 15:15:50 -0400 Subject: [PATCH] lttng: start: ensure a cmd_error_code is returned by the command MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The start_tracing functions mix cmd_error_code and lttng_error_code values in "raw" integers which is unexpected by the top-level client. For instance, the client returns '80' when attempting to start a session that is already active. Signed-off-by: Jérémie Galarneau Change-Id: I12c15e8aa3eb960e1e47d5166307995af8c46989 --- src/bin/lttng/commands/start.cpp | 174 ++++++++++++++++++------------- src/bin/lttng/utils.hpp | 34 ++++-- 2 files changed, 123 insertions(+), 85 deletions(-) diff --git a/src/bin/lttng/commands/start.cpp b/src/bin/lttng/commands/start.cpp index 1d787ce65..87df0f72f 100644 --- a/src/bin/lttng/commands/start.cpp +++ b/src/bin/lttng/commands/start.cpp @@ -79,75 +79,101 @@ end: * * Start tracing for all trace of the session. */ -int start_tracing(const char *session_name) +cmd_error_code start_tracing(const char *session_name) { - int ret; - if (session_name == nullptr) { - ret = CMD_ERROR; - goto error; + return CMD_ERROR; } - DBG("Starting tracing for session %s", session_name); + DBG("Starting tracing for session `%s`", session_name); - ret = lttng_start_tracing(session_name); + const int ret = lttng_start_tracing(session_name); if (ret < 0) { - switch (-ret) { - case LTTNG_ERR_TRACE_ALREADY_STARTED: - WARN("Tracing already started for session %s", session_name); - break; - default: - ERR("%s", lttng_strerror(ret)); - break; - } - goto error; + LTTNG_THROW_CTL(fmt::format("Failed to start session `{}`", session_name), + static_cast(-ret)); } - ret = CMD_SUCCESS; - - MSG("Tracing started for session %s", session_name); + MSG("Tracing started for session `%s`", session_name); if (lttng_opt_mi) { - ret = mi_print_session(session_name, 1); - if (ret) { - ret = CMD_ERROR; - goto error; + if (mi_print_session(session_name, 1)) { + return CMD_ERROR; } } -error: - return ret; + return CMD_SUCCESS; } -int start_tracing(const struct session_spec& spec) +cmd_error_code start_tracing(const session_spec& spec) noexcept { - int ret = CMD_SUCCESS; bool had_warning = false; + bool had_error = false; + bool listing_failed = false; + + const auto sessions = [&listing_failed, &spec]() -> session_list { + try { + return list_sessions(spec); + } catch (const lttng::ctl::error& ctl_exception) { + ERR_FMT("Failed to list sessions ({})", + lttng_strerror(-ctl_exception.code())); + listing_failed = true; + return {}; + } + }(); + + if (!listing_failed && sessions.size() == 0 && spec.type == session_spec::type::NAME) { + ERR_FMT("Session `{}` not found", spec.value); + return CMD_ERROR; + } - try { - for (const auto& session : list_sessions(spec)) { - const auto sub_ret = start_tracing(session.name); + if (listing_failed) { + return CMD_FATAL; + } + + for (const auto& session : sessions) { + cmd_error_code sub_ret; - switch (sub_ret) { - case CMD_WARNING: - had_warning = true; - /* fall-through. */ - case CMD_SUCCESS: - continue; + try { + sub_ret = start_tracing(session.name); + } catch (const lttng::ctl::error& ctl_exception) { + switch (ctl_exception.code()) { + case LTTNG_ERR_TRACE_ALREADY_STARTED: + WARN_FMT("Tracing already started for session `{}`", session.name); + sub_ret = CMD_SUCCESS; + break; + case LTTNG_ERR_NO_SESSION: + if (spec.type != session_spec::type::NAME) { + /* Session destroyed during command, ignore and carry-on. */ + sub_ret = CMD_SUCCESS; + break; + } else { + sub_ret = CMD_ERROR; + break; + } + case LTTNG_ERR_NO_SESSIOND: + /* Don't keep going on a fatal error. */ + return CMD_FATAL; default: - ret = sub_ret; + /* Generic error. */ + sub_ret = CMD_ERROR; + ERR_FMT("Failed to start session `{}` ({})", + session.name, + lttng_strerror(-ctl_exception.code())); break; } } - } catch (const std::exception& e) { - ERR_FMT("{}", e.what()); - return CMD_FATAL; - } - if (ret == CMD_SUCCESS && had_warning) { - ret = CMD_WARNING; + /* Keep going, but report the most serious state. */ + had_warning |= sub_ret == CMD_WARNING; + had_error |= sub_ret == CMD_ERROR; } - return ret; + if (had_error) { + return CMD_ERROR; + } else if (had_warning) { + return CMD_WARNING; + } else { + return CMD_SUCCESS; + } } } /* namespace */ @@ -158,10 +184,12 @@ int start_tracing(const struct session_spec& spec) */ int cmd_start(int argc, const char **argv) { - int opt, ret = CMD_SUCCESS, command_ret = CMD_SUCCESS, success = 1; + int opt; + cmd_error_code command_ret = CMD_SUCCESS; + bool success = true; static poptContext pc; const char *leftover = nullptr; - struct session_spec session_spec = { + session_spec session_spec = { .type = session_spec::NAME, .value = nullptr, }; @@ -172,8 +200,13 @@ int cmd_start(int argc, const char **argv) while ((opt = poptGetNextOpt(pc)) != -1) { switch (opt) { case OPT_HELP: + { + int ret; + SHOW_HELP(); + command_ret = static_cast(ret); goto end; + } case OPT_LIST_OPTIONS: list_cmd_options(stdout, long_options); goto end; @@ -184,7 +217,7 @@ int cmd_start(int argc, const char **argv) session_spec.type = session_spec::ALL; break; default: - ret = CMD_UNDEFINED; + command_ret = CMD_UNDEFINED; goto end; } } @@ -194,7 +227,7 @@ int cmd_start(int argc, const char **argv) leftover = poptGetArg(pc); if (leftover) { ERR("Unknown argument: %s", leftover); - ret = CMD_ERROR; + command_ret = CMD_ERROR; goto end; } @@ -202,21 +235,19 @@ int cmd_start(int argc, const char **argv) if (lttng_opt_mi) { writer = mi_lttng_writer_create(fileno(stdout), lttng_opt_mi); if (!writer) { - ret = -LTTNG_ERR_NOMEM; + command_ret = CMD_ERROR; goto end; } /* Open command element */ - ret = mi_lttng_writer_command_open(writer, mi_lttng_element_command_start); - if (ret) { - ret = CMD_ERROR; + if (mi_lttng_writer_command_open(writer, mi_lttng_element_command_start)) { + command_ret = CMD_ERROR; goto end; } /* Open output element */ - ret = mi_lttng_writer_open_element(writer, mi_lttng_element_command_output); - if (ret) { - ret = CMD_ERROR; + if (mi_lttng_writer_open_element(writer, mi_lttng_element_command_output)) { + command_ret = CMD_ERROR; goto end; } @@ -224,39 +255,35 @@ int cmd_start(int argc, const char **argv) * Open sessions element * For validation purpose */ - ret = mi_lttng_writer_open_element(writer, config_element_sessions); - if (ret) { - ret = CMD_ERROR; + if (mi_lttng_writer_open_element(writer, config_element_sessions)) { + command_ret = CMD_ERROR; goto end; } } command_ret = start_tracing(session_spec); - if (command_ret) { - success = 0; + if (command_ret != CMD_SUCCESS) { + success = false; } /* Mi closing */ if (lttng_opt_mi) { /* Close sessions and output element */ - ret = mi_lttng_close_multi_element(writer, 2); - if (ret) { - ret = CMD_ERROR; + if (mi_lttng_close_multi_element(writer, 2)) { + command_ret = CMD_ERROR; goto end; } /* Success ? */ - ret = mi_lttng_writer_write_element_bool( - writer, mi_lttng_element_command_success, success); - if (ret) { - ret = CMD_ERROR; + if (mi_lttng_writer_write_element_bool( + writer, mi_lttng_element_command_success, success)) { + command_ret = CMD_ERROR; goto end; } /* Command element close */ - ret = mi_lttng_writer_command_close(writer); - if (ret) { - ret = CMD_ERROR; + if (mi_lttng_writer_command_close(writer)) { + command_ret = CMD_ERROR; goto end; } } @@ -264,12 +291,9 @@ int cmd_start(int argc, const char **argv) end: /* Mi clean-up */ if (writer && mi_lttng_writer_destroy(writer)) { - /* Preserve original error code */ - ret = ret ? ret : -LTTNG_ERR_MI_IO_FAIL; + command_ret = CMD_ERROR; } - /* Overwrite ret if an error occurred with start_tracing */ - ret = command_ret ? command_ret : ret; poptFreeContext(pc); - return ret; + return command_ret; } diff --git a/src/bin/lttng/utils.hpp b/src/bin/lttng/utils.hpp index c3b442056..4fa15c38d 100644 --- a/src/bin/lttng/utils.hpp +++ b/src/bin/lttng/utils.hpp @@ -40,56 +40,60 @@ struct session_spec { * We don't use a std::vector here because it would make a copy of the C array. */ class session_list { - class iterator : public std::iterator { + template + class iterator_template : public std::iterator { public: - explicit iterator(session_list& list, std::size_t k) : _list(list), _index(k) + explicit iterator_template(ContainerType& list, std::size_t k) : _list(list), _index(k) { } - iterator& operator++() noexcept + iterator_template& operator++() noexcept { ++_index; return *this; } - iterator& operator--() noexcept + iterator_template& operator--() noexcept { --_index; return *this; } - iterator& operator++(int) noexcept + iterator_template& operator++(int) noexcept { _index++; return *this; } - iterator& operator--(int) noexcept + iterator_template& operator--(int) noexcept { _index--; return *this; } - bool operator==(iterator other) const noexcept + bool operator==(iterator_template other) const noexcept { return _index == other._index; } - bool operator!=(iterator other) const noexcept + bool operator!=(iterator_template other) const noexcept { return !(*this == other); } - lttng_session& operator*() const noexcept + DereferenceReturnType& operator*() const noexcept { return _list[_index]; } private: - session_list& _list; + ContainerType& _list; std::size_t _index; }; + using iterator = iterator_template; + using const_iterator = iterator_template; + public: session_list() : _sessions_count(0), _sessions(nullptr) { @@ -117,6 +121,16 @@ public: return iterator(*this, _sessions_count); } + const_iterator begin() const noexcept + { + return const_iterator(*this, 0); + } + + const_iterator end() const noexcept + { + return const_iterator(*this, _sessions_count); + } + std::size_t size() const noexcept { return _sessions_count; -- 2.34.1