From b17ed2ad7083d1b0bf45fe3e1bfc4e4ad787aaf3 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Thu, 1 Jun 2023 16:19:31 -0400 Subject: [PATCH] Provide an idiomatic c++ interface for action lists MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Replace for_each macros with the use of an iterator. It is done by using a random_access_container_wrapper util which is intended to wrap random_access containers implemented in C. Change-Id: I1b22725b7335f267c9b2d02fc65f9375baf37426 Signed-off-by: Jérémie Galarneau --- include/lttng/action/list-internal.hpp | 68 ++++++++-- src/bin/lttng-sessiond/action-executor.cpp | 4 +- .../notification-thread-events.cpp | 3 +- src/bin/lttng/commands/list_triggers.cpp | 3 +- src/common/Makefile.am | 1 + src/common/container-wrapper.hpp | 128 ++++++++++++++++++ .../tools/trigger/utils/Makefile.am | 3 +- .../trigger/utils/notification-client.cpp | 5 +- tests/unit/test_action.cpp | 14 +- 9 files changed, 195 insertions(+), 34 deletions(-) create mode 100644 src/common/container-wrapper.hpp diff --git a/include/lttng/action/list-internal.hpp b/include/lttng/action/list-internal.hpp index 53cab5897..607d59818 100644 --- a/include/lttng/action/list-internal.hpp +++ b/include/lttng/action/list-internal.hpp @@ -8,9 +8,12 @@ #ifndef LTTNG_ACTION_LIST_INTERNAL_H #define LTTNG_ACTION_LIST_INTERNAL_H +#include +#include +#include #include -#include +#include #include #include @@ -41,19 +44,54 @@ lttng_action_list_mi_serialize(const struct lttng_trigger *trigger, const struct mi_lttng_error_query_callbacks *error_query_callbacks, struct lttng_dynamic_array *action_path_indexes); -#define for_each_action_const(__action_element, __action_list) \ - assert(lttng_action_get_type(__action_list) == LTTNG_ACTION_TYPE_LIST); \ - \ - for (unsigned int __action_idx = 0; \ - (__action_element = lttng_action_list_get_at_index(__action_list, __action_idx)); \ - __action_idx++) - -#define for_each_action_mutable(__action_element, __action_list) \ - assert(lttng_action_get_type(__action_list) == LTTNG_ACTION_TYPE_LIST); \ - \ - for (unsigned int __action_idx = 0; \ - (__action_element = \ - lttng_action_list_borrow_mutable_at_index(__action_list, __action_idx)); \ - __action_idx++) +namespace lttng { +namespace ctl { +namespace details { +class action_list_operations { +public: + static lttng_action *get(const lttng_action *list, std::size_t index) noexcept + { + return lttng_action_list_borrow_mutable_at_index(list, index); + } + + static std::size_t size(const lttng_action *list) + { + unsigned int count; + const auto status = lttng_action_list_get_count(list, &count); + + if (status != LTTNG_ACTION_STATUS_OK) { + LTTNG_THROW_INVALID_ARGUMENT_ERROR( + "Failed to get action list element count"); + } + + return count; + } +}; + +class const_action_list_operations { +public: + static const lttng_action *get(const lttng_action *list, std::size_t index) noexcept + { + return lttng_action_list_get_at_index(list, index); + } + + static std::size_t size(const lttng_action *list) + { + return action_list_operations::size(list); + } +}; +} /* namespace details */ + +using action_list_view = utils::random_access_container_wrapper; + +using const_action_list_view = + utils::random_access_container_wrapper; + +} /* namespace ctl */ +} /* namespace lttng */ #endif /* LTTNG_ACTION_LIST_INTERNAL_H */ diff --git a/src/bin/lttng-sessiond/action-executor.cpp b/src/bin/lttng-sessiond/action-executor.cpp index a9476bd31..3fbb87f43 100644 --- a/src/bin/lttng-sessiond/action-executor.cpp +++ b/src/bin/lttng-sessiond/action-executor.cpp @@ -992,9 +992,7 @@ static int add_action_to_subitem_array(struct lttng_action *action, LTTNG_ASSERT(subitems); if (type == LTTNG_ACTION_TYPE_LIST) { - struct lttng_action *inner_action = NULL; - - for_each_action_mutable (inner_action, action) { + for (auto inner_action : lttng::ctl::action_list_view(action)) { LTTNG_ASSERT(inner_action); ret = add_action_to_subitem_array(inner_action, subitems); diff --git a/src/bin/lttng-sessiond/notification-thread-events.cpp b/src/bin/lttng-sessiond/notification-thread-events.cpp index ab0fa84ce..aaf24b3f0 100644 --- a/src/bin/lttng-sessiond/notification-thread-events.cpp +++ b/src/bin/lttng-sessiond/notification-thread-events.cpp @@ -2503,7 +2503,6 @@ static bool is_trigger_action_notify(const struct lttng_trigger *trigger) { bool is_notify = false; const struct lttng_action *action = lttng_trigger_get_const_action(trigger); - const struct lttng_action *inner_action; enum lttng_action_type action_type; LTTNG_ASSERT(action); @@ -2515,7 +2514,7 @@ static bool is_trigger_action_notify(const struct lttng_trigger *trigger) goto end; } - for_each_action_const (inner_action, action) { + for (auto inner_action : lttng::ctl::const_action_list_view(action)) { if (lttng_action_get_type(inner_action) == LTTNG_ACTION_TYPE_NOTIFY) { is_notify = true; goto end; diff --git a/src/bin/lttng/commands/list_triggers.cpp b/src/bin/lttng/commands/list_triggers.cpp index c9c6c265d..03e567c86 100644 --- a/src/bin/lttng/commands/list_triggers.cpp +++ b/src/bin/lttng/commands/list_triggers.cpp @@ -1024,11 +1024,10 @@ static void print_one_trigger(const struct lttng_trigger *trigger) action = lttng_trigger_get_const_action(trigger); action_type = lttng_action_get_type(action); if (action_type == LTTNG_ACTION_TYPE_LIST) { - const struct lttng_action *subaction; uint64_t action_path_index = 0; MSG(" actions:"); - for_each_action_const (subaction, action) { + for (auto subaction : lttng::ctl::const_action_list_view(action)) { _MSG(" "); print_one_action(trigger, subaction, &action_path_index, 1); action_path_index++; diff --git a/src/common/Makefile.am b/src/common/Makefile.am index 8f18033bc..344787347 100644 --- a/src/common/Makefile.am +++ b/src/common/Makefile.am @@ -63,6 +63,7 @@ libcommon_lgpl_la_SOURCES = \ conditions/event-rule-matches.cpp \ conditions/session-consumed-size.cpp \ conditions/session-rotation.cpp \ + container-wrapper.hpp \ credentials.cpp credentials.hpp \ defaults.cpp \ domain.cpp \ diff --git a/src/common/container-wrapper.hpp b/src/common/container-wrapper.hpp new file mode 100644 index 000000000..09097e935 --- /dev/null +++ b/src/common/container-wrapper.hpp @@ -0,0 +1,128 @@ +/* + * Copyright (C) 2023 Jérémie Galarneau + * + * SPDX-License-Identifier: LGPL-2.1-only + * + */ + +#ifndef LTTNG_CONTAINER_WRAPPER_H +#define LTTNG_CONTAINER_WRAPPER_H + +#include + +#include +#include + +namespace lttng { +namespace utils { + +/* + * random_access_container_wrapper is a helper to provide an idiomatic C++ interface + * from a C container API. ElementAccessorCallable and ElementCountAccessorCallable + * are two functors which must be provided to allow access to the underlying elements + * of the container and to its size. + */ +template +class random_access_container_wrapper { + class _iterator : public std::iterator { + public: + explicit _iterator(const random_access_container_wrapper& container, + std::size_t start_index = 0) : + _container(container), _index(start_index) + { + } + + _iterator& operator++() noexcept + { + ++_index; + return *this; + } + + _iterator& operator--() noexcept + { + --_index; + return *this; + } + + _iterator& operator++(int) noexcept + { + auto this_before_increment = *this; + + _index++; + return this_before_increment; + } + + _iterator& operator--(int) noexcept + { + _index--; + return *this; + } + + bool operator==(const _iterator& other) const noexcept + { + return _index == other._index; + } + + bool operator!=(const _iterator& other) const noexcept + { + return !(*this == other); + } + + typename std::conditional::value, + ElementType, + ElementType&>::type + operator*() const noexcept + { + return _container[_index]; + } + + private: + const random_access_container_wrapper& _container; + std::size_t _index; + }; + + using iterator = _iterator; + +public: + explicit random_access_container_wrapper(ContainerType container) : _container{ container } + { + } + + iterator begin() noexcept + { + return iterator(*this); + } + + iterator end() noexcept + { + return iterator(*this, ContainerOperations::size(_container)); + } + + std::size_t size() const noexcept + { + return ContainerOperations::size(_container); + } + + typename std::conditional::value, ElementType, ElementType&>::type + operator[](std::size_t index) + { + LTTNG_ASSERT(index < ContainerOperations::size(_container)); + return ContainerOperations::get(_container, index); + } + + typename std::conditional::value, + const ElementType, + const ElementType&>::type + operator[](std::size_t index) const + { + LTTNG_ASSERT(index < ContainerOperations::size(_container)); + return ContainerOperations::get(_container, index); + } + +private: + ContainerType _container; +}; +} /* namespace utils */ +} /* namespace lttng */ + +#endif /* LTTNG_CONTAINER_WRAPPER_H */ diff --git a/tests/regression/tools/trigger/utils/Makefile.am b/tests/regression/tools/trigger/utils/Makefile.am index 3522aaec3..d90a45ea5 100644 --- a/tests/regression/tools/trigger/utils/Makefile.am +++ b/tests/regression/tools/trigger/utils/Makefile.am @@ -2,13 +2,14 @@ AM_CPPFLAGS += -I$(srcdir) -I$(top_srcdir)/tests/utils LIBLTTNG_CTL=$(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la +LIBCOMMON_LGPL=$(top_builddir)/src/common/libcommon-lgpl.la noinst_PROGRAMS = \ notification-client \ register-some-triggers notification_client_SOURCES = notification-client.cpp -notification_client_LDADD = $(LIBLTTNG_CTL) \ +notification_client_LDADD = $(LIBLTTNG_CTL) $(LIBCOMMON_LGPL) \ $(top_builddir)/tests/utils/libtestutils.la register_some_triggers_SOURCES = register-some-triggers.cpp diff --git a/tests/regression/tools/trigger/utils/notification-client.cpp b/tests/regression/tools/trigger/utils/notification-client.cpp index 02dc06260..f73382158 100644 --- a/tests/regression/tools/trigger/utils/notification-client.cpp +++ b/tests/regression/tools/trigger/utils/notification-client.cpp @@ -38,13 +38,12 @@ static struct option long_options[] = { static bool action_list_contains_notify(const struct lttng_action *action_list) { - const struct lttng_action *sub_action; - - for_each_action_const (sub_action, action_list) { + for (auto sub_action : lttng::ctl::const_action_list_view(action_list)) { if (lttng_action_get_type(sub_action) == LTTNG_ACTION_TYPE_NOTIFY) { return true; } } + return false; } diff --git a/tests/unit/test_action.cpp b/tests/unit/test_action.cpp index 131a3af1d..7d0727eda 100644 --- a/tests/unit/test_action.cpp +++ b/tests/unit/test_action.cpp @@ -103,9 +103,8 @@ static void test_action_list(void) { int ret, action_idx; struct lttng_action *list_action = NULL, *list_action_from_buffer = NULL, - *mut_inner_action = NULL, *stop_session_action = NULL, - *notify_action = NULL, *start_session_action = NULL; - const struct lttng_action *const_inner_action; + *stop_session_action = NULL, *notify_action = NULL, + *start_session_action = NULL; struct lttng_payload payload; lttng_payload_init(&payload); @@ -139,9 +138,8 @@ static void test_action_list(void) "Serialized and de-serialized list action are equal"); action_idx = 0; - for_each_action_const (const_inner_action, list_action) { - enum lttng_action_type inner_action_type = - lttng_action_get_type(const_inner_action); + for (auto action : lttng::ctl::const_action_list_view(list_action)) { + enum lttng_action_type inner_action_type = lttng_action_get_type(action); switch (action_idx) { case 0: ok(inner_action_type == LTTNG_ACTION_TYPE_START_SESSION, @@ -160,8 +158,8 @@ static void test_action_list(void) } action_idx = 0; - for_each_action_mutable (mut_inner_action, list_action) { - enum lttng_action_type inner_action_type = lttng_action_get_type(mut_inner_action); + for (auto action : lttng::ctl::action_list_view(list_action)) { + enum lttng_action_type inner_action_type = lttng_action_get_type(action); switch (action_idx) { case 0: ok(inner_action_type == LTTNG_ACTION_TYPE_START_SESSION, -- 2.34.1