From b9984811575702dea69108d180f5e90098709d13 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Tue, 30 Apr 2024 15:35:32 -0400 Subject: [PATCH 01/16] Clean-up: consumer.hpp: coding style indentation fix MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Caught this that slipped by when running ./format-cpp Signed-off-by: Jérémie Galarneau Change-Id: Ie16dc145d0776acba2ddcee31f2180b840112921 --- src/common/consumer/consumer.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/consumer/consumer.hpp b/src/common/consumer/consumer.hpp index 310969172..851b3e2b8 100644 --- a/src/common/consumer/consumer.hpp +++ b/src/common/consumer/consumer.hpp @@ -922,7 +922,7 @@ void lttng_consumer_set_command_sock_path(struct lttng_consumer_local_data *ctx, * on error. */ int lttng_consumer_send_error(struct lttng_consumer_local_data *ctx, - enum lttcomm_return_code error_code); + enum lttcomm_return_code error_code); /* * Called from signal handler to ensure a clean exit. -- 2.34.1 From 6dee08cf205eb0c5f9edc37eeb5d10ad61abd0e1 Mon Sep 17 00:00:00 2001 From: Michael Jeanson Date: Tue, 30 Apr 2024 15:17:52 -0400 Subject: [PATCH 02/16] fix: relayd: unaligned access in trace_chunk_registry_ht_key_hash MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit In 328c2fe7297c941aa9cbcfa4ce944fca1bd7300f, the type of 'lttng_uuid' was changed from a C array of 16 'uint8_t' to a C++ std::array of the same type and length. In 'trace_chunk_registry_ht_key_hash()' we access these 16 bytes as 2 'uint64_t', to do so we used to cast the array to '(uint64_t *)' and then access index 0 and 1. When it was converted to C++, an error was introduced where instead we reinterpret_cast to 'const uint64_t *' the index 0 and 1 of the array which results in a 'uint64_t' pointer to the first and second bytes of the array. These values overlap but since they are used as keys for an hash table it still works. However, on platforms that don't allow unaligned access, the second pointer being only offset by one byte results in a 'Bus error'. Reintroduce the old behavior by applying the index 0 and 1 to the pointer resulting from the reinterpret_cast. Change-Id: I2bc287edbe6864a2a870f9de1f3b4dd8f8a90ace Signed-off-by: Michael Jeanson Signed-off-by: Jérémie Galarneau --- src/bin/lttng-relayd/sessiond-trace-chunks.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bin/lttng-relayd/sessiond-trace-chunks.cpp b/src/bin/lttng-relayd/sessiond-trace-chunks.cpp index 9d793c9bc..2326878fb 100644 --- a/src/bin/lttng-relayd/sessiond-trace-chunks.cpp +++ b/src/bin/lttng-relayd/sessiond-trace-chunks.cpp @@ -78,8 +78,8 @@ struct trace_chunk_registry_ht_element { static unsigned long trace_chunk_registry_ht_key_hash(const struct trace_chunk_registry_ht_key *key) { - const uint64_t uuid_h1 = *reinterpret_cast(&key->sessiond_uuid[0]); - const uint64_t uuid_h2 = *reinterpret_cast(&key->sessiond_uuid[1]); + const uint64_t uuid_h1 = reinterpret_cast(key->sessiond_uuid.data())[0]; + const uint64_t uuid_h2 = reinterpret_cast(key->sessiond_uuid.data())[1]; return hash_key_u64(&uuid_h1, lttng_ht_seed) ^ hash_key_u64(&uuid_h2, lttng_ht_seed); } -- 2.34.1 From 9617033a3a3229daabe17e38fbab802ec2e6e223 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 30 Apr 2024 15:22:15 -0400 Subject: [PATCH 03/16] tests: convert pretty_xml.c to C++ MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Change-Id: I40e0aa849193e789bf6634068a92b55484ab17af Signed-off-by: Simon Marchi Signed-off-by: Jérémie Galarneau --- tests/utils/xml-utils/Makefile.am | 2 +- tests/utils/xml-utils/{pretty_xml.c => pretty_xml.cpp} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename tests/utils/xml-utils/{pretty_xml.c => pretty_xml.cpp} (100%) diff --git a/tests/utils/xml-utils/Makefile.am b/tests/utils/xml-utils/Makefile.am index 7997d94e1..eec519d31 100644 --- a/tests/utils/xml-utils/Makefile.am +++ b/tests/utils/xml-utils/Makefile.am @@ -9,7 +9,7 @@ extract_xml_SOURCES = extract_xml.cpp extract_xml_CPPFLAGS = $(libxml2_CFLAGS) $(AM_CPPFLAGS) extract_xml_LDADD = $(libxml2_LIBS) -pretty_xml_SOURCES = pretty_xml.c +pretty_xml_SOURCES = pretty_xml.cpp pretty_xml_CPPFLAGS = $(libxml2_CFLAGS) $(AM_CPPFLAGS) pretty_xml_LDADD = $(libxml2_LIBS) diff --git a/tests/utils/xml-utils/pretty_xml.c b/tests/utils/xml-utils/pretty_xml.cpp similarity index 100% rename from tests/utils/xml-utils/pretty_xml.c rename to tests/utils/xml-utils/pretty_xml.cpp -- 2.34.1 From 051356a81761338485706e779d046d7fb8dbfe1d Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 30 Apr 2024 15:19:32 -0400 Subject: [PATCH 04/16] tests: remove uses of `xmlKeepBlanksDefault()` MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When compiling against libxml 2.12.6, I get: CXX extract_xml-extract_xml.o /home/simark/src/lttng-tools/tests/utils/xml-utils/extract_xml.cpp: In function ‘int main(int, char**)’: /home/simark/src/lttng-tools/tests/utils/xml-utils/extract_xml.cpp:256:29: error: ‘int xmlKeepBlanksDefault(int)’ is deprecated [-Werror=deprecated-declarations] 256 | xmlKeepBlanksDefault(0); | ~~~~~~~~~~~~~~~~~~~~^~~ In file included from /home/simark/src/lttng-tools/tests/utils/xml-utils/extract_xml.cpp:29: /usr/include/libxml2/libxml/parser.h:957:17: note: declared here 957 | xmlKeepBlanksDefault (int val); | ^~~~~~~~~~~~~~~~~~~~ The documentation[1] suggests moving to "the modern options API with XML_PARSE_NOBLANKS", do that. Add a new `xml_parser_ctx_uptr` RAII object to manage the lifetime of the `xmlParserCtxt` object. [1] https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-parser.html#xmlKeepBlanksDefault Change-Id: I7f9e70d2245fa333e01296aa2b4e3efa4e7fbefa Signed-off-by: Simon Marchi Signed-off-by: Jérémie Galarneau --- tests/utils/xml-utils/Makefile.am | 2 ++ tests/utils/xml-utils/common.hpp | 20 +++++++++++++++++++ tests/utils/xml-utils/extract_xml.cpp | 12 ++++++++++-- tests/utils/xml-utils/pretty_xml.cpp | 28 +++++++++++++++++---------- 4 files changed, 50 insertions(+), 12 deletions(-) create mode 100644 tests/utils/xml-utils/common.hpp diff --git a/tests/utils/xml-utils/Makefile.am b/tests/utils/xml-utils/Makefile.am index eec519d31..5890eef27 100644 --- a/tests/utils/xml-utils/Makefile.am +++ b/tests/utils/xml-utils/Makefile.am @@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only +EXTRA_DIST = common.hpp + noinst_PROGRAMS = validate_xml extract_xml pretty_xml validate_xml_SOURCES = validate_xml.cpp validate_xml_CPPFLAGS = $(libxml2_CFLAGS) $(AM_CPPFLAGS) diff --git a/tests/utils/xml-utils/common.hpp b/tests/utils/xml-utils/common.hpp new file mode 100644 index 000000000..5acb1b1be --- /dev/null +++ b/tests/utils/xml-utils/common.hpp @@ -0,0 +1,20 @@ +/* + * Copyright (C) 2024 EfficiOS Inc. + * + * SPDX-License-Identifier: LGPL-2.1-only + * + */ + +#ifndef TESTS_UTILS_XML_UTILS_COMMON_HPP +#define TESTS_UTILS_XML_UTILS_COMMON_HPP + +#include "common/make-unique-wrapper.hpp" + +#include +#include + +using xml_parser_ctx_uptr = std::unique_ptr< + xmlParserCtxt, + lttng::memory::create_deleter_class::deleter>; + +#endif /* TESTS_UTILS_XML_UTILS_COMMON_HPP */ diff --git a/tests/utils/xml-utils/extract_xml.cpp b/tests/utils/xml-utils/extract_xml.cpp index 3995dde48..ad319d3c4 100644 --- a/tests/utils/xml-utils/extract_xml.cpp +++ b/tests/utils/xml-utils/extract_xml.cpp @@ -24,6 +24,8 @@ * node;b; * node;c; */ +#include "common.hpp" + #include #include @@ -176,8 +178,15 @@ static int extract_xpath(const char *xml_path, const xmlChar *xpath) LTTNG_ASSERT(xml_path); LTTNG_ASSERT(xpath); + xml_parser_ctx_uptr parserCtx{ xmlNewParserCtxt() }; + + if (!parserCtx) { + fprintf(stderr, "ERR: could not allocate an XML parser context\n"); + return -1; + } + /* Parse the xml file */ - doc = xmlParseFile(xml_path); + doc = xmlCtxtReadFile(parserCtx.get(), xml_path, nullptr, XML_PARSE_NOBLANKS); if (!doc) { fprintf(stderr, "ERR parsing: xml file invalid \"%s\"\n", xml_path); return -1; @@ -253,7 +262,6 @@ int main(int argc, char **argv) /* Init libxml */ xmlInitParser(); - xmlKeepBlanksDefault(0); if (access(argv[optind], F_OK)) { fprintf(stderr, "ERR:%s\n", "Xml path not valid"); return -1; diff --git a/tests/utils/xml-utils/pretty_xml.cpp b/tests/utils/xml-utils/pretty_xml.cpp index 3f296f023..7eb4710ca 100644 --- a/tests/utils/xml-utils/pretty_xml.cpp +++ b/tests/utils/xml-utils/pretty_xml.cpp @@ -10,26 +10,34 @@ * This allows a more human friendly format for xml testing when problems occur. */ +#include "common.hpp" + #include +#include -int main(void) +int main() { xmlDocPtr doc = NULL; /* Init libxml. */ xmlInitParser(); - xmlKeepBlanksDefault(0); - /* Parse the XML document from stdin. */ - doc = xmlParseFile("-"); - if (!doc) { - fprintf(stderr, "ERR parsing: xml input invalid"); - return -1; - } + { + xml_parser_ctx_uptr parserCtx{ xmlNewParserCtxt() }; + + /* Parse the XML document from stdin. */ + doc = xmlCtxtReadFd( + parserCtx.get(), STDIN_FILENO, nullptr, nullptr, XML_PARSE_NOBLANKS); + if (!doc) { + fprintf(stderr, "ERR parsing: xml input invalid"); + return -1; + } - xmlDocFormatDump(stdout, doc, 1); + xmlDocFormatDump(stdout, doc, 1); + + xmlFreeDoc(doc); + } - xmlFreeDoc(doc); /* Shutdown libxml. */ xmlCleanupParser(); -- 2.34.1 From 0e9cf259c6e7f66b48c07365dca15903cd352164 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 24 Apr 2024 16:50:32 -0400 Subject: [PATCH 05/16] .clang-format: remove duplicated BreakConstructorInitializers MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Signed-off-by: Jérémie Galarneau Change-Id: I1cae8752d4c1b6ead1b718f5bc8414bca2580588 --- .clang-format | 1 - 1 file changed, 1 deletion(-) diff --git a/.clang-format b/.clang-format index 28ea69873..8fcee64f8 100644 --- a/.clang-format +++ b/.clang-format @@ -35,7 +35,6 @@ BraceWrapping: SplitEmptyFunction: true BreakBeforeBinaryOperators: None BreakBeforeTernaryOperators: false -BreakConstructorInitializers: AfterColon BreakStringLiterals: false ColumnLimit: 100 ConstructorInitializerAllOnOneLineOrOnePerLine: true -- 2.34.1 From f59edc7cf8bfcf1450ce2d9e649f70c1e6796da0 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 1 May 2024 11:21:04 -0400 Subject: [PATCH 06/16] Clean-up: modernize pretty_xml.cpp MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Since pretty_xml was converted to C++, clang-tidy had a number of grievances such as the use of NULL (instead of nullptr). Since the file is very small, it is modernized to address those issues: - Wrap libxml2 resources in RAII wrappers - Use C++ IO APIs in lieu of fprintf Signed-off-by: Jérémie Galarneau Change-Id: Ie90e3e05130f7916f411e0de36e8aac72a0f790c --- tests/utils/xml-utils/common.hpp | 31 ++++++++++++++++++++++- tests/utils/xml-utils/extract_xml.cpp | 4 ++- tests/utils/xml-utils/pretty_xml.cpp | 36 +++++++++++---------------- 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/tests/utils/xml-utils/common.hpp b/tests/utils/xml-utils/common.hpp index 5acb1b1be..01a9b5207 100644 --- a/tests/utils/xml-utils/common.hpp +++ b/tests/utils/xml-utils/common.hpp @@ -13,8 +13,37 @@ #include #include -using xml_parser_ctx_uptr = std::unique_ptr< +namespace lttng { +namespace libxml { + +using parser_ctx_uptr = std::unique_ptr< xmlParserCtxt, lttng::memory::create_deleter_class::deleter>; +using doc_uptr = + std::unique_ptr::deleter>; + +/* + * Manage the global parser context of libxml2. + * There should only be one instance of this class per process. + */ +class global_parser_context { +public: + global_parser_context() + { + xmlInitParser(); + } + + ~global_parser_context() + { + xmlCleanupParser(); + } + /* Deactivate copy and assignment. */ + global_parser_context(const global_parser_context&) = delete; + global_parser_context(global_parser_context&&) = delete; + global_parser_context& operator=(const global_parser_context&) = delete; + global_parser_context& operator=(global_parser_context&&) = delete; +}; +} /* namespace libxml */ +} /* namespace lttng */ #endif /* TESTS_UTILS_XML_UTILS_COMMON_HPP */ diff --git a/tests/utils/xml-utils/extract_xml.cpp b/tests/utils/xml-utils/extract_xml.cpp index ad319d3c4..280f2ed02 100644 --- a/tests/utils/xml-utils/extract_xml.cpp +++ b/tests/utils/xml-utils/extract_xml.cpp @@ -38,6 +38,8 @@ #include #include +namespace ll = lttng::libxml; + #if defined(LIBXML_XPATH_ENABLED) static int opt_verbose; @@ -178,7 +180,7 @@ static int extract_xpath(const char *xml_path, const xmlChar *xpath) LTTNG_ASSERT(xml_path); LTTNG_ASSERT(xpath); - xml_parser_ctx_uptr parserCtx{ xmlNewParserCtxt() }; + ll::parser_ctx_uptr parserCtx{ xmlNewParserCtxt() }; if (!parserCtx) { fprintf(stderr, "ERR: could not allocate an XML parser context\n"); diff --git a/tests/utils/xml-utils/pretty_xml.cpp b/tests/utils/xml-utils/pretty_xml.cpp index 7eb4710ca..8a6e967a7 100644 --- a/tests/utils/xml-utils/pretty_xml.cpp +++ b/tests/utils/xml-utils/pretty_xml.cpp @@ -12,34 +12,28 @@ #include "common.hpp" +#include + +#include #include #include +namespace ll = lttng::libxml; + int main() { - xmlDocPtr doc = NULL; - - /* Init libxml. */ - xmlInitParser(); - - { - xml_parser_ctx_uptr parserCtx{ xmlNewParserCtxt() }; - - /* Parse the XML document from stdin. */ - doc = xmlCtxtReadFd( - parserCtx.get(), STDIN_FILENO, nullptr, nullptr, XML_PARSE_NOBLANKS); - if (!doc) { - fprintf(stderr, "ERR parsing: xml input invalid"); - return -1; - } - - xmlDocFormatDump(stdout, doc, 1); - - xmlFreeDoc(doc); + const ll::global_parser_context global_parser_context; + const ll::parser_ctx_uptr parserCtx{ xmlNewParserCtxt() }; + + /* Parse the XML document from stdin. */ + const ll::doc_uptr doc{ xmlCtxtReadFd( + parserCtx.get(), STDIN_FILENO, nullptr, nullptr, XML_PARSE_NOBLANKS) }; + if (!doc) { + std::cerr << "Error: invalid XML input on stdin\n"; + return -1; } - /* Shutdown libxml. */ - xmlCleanupParser(); + xmlDocFormatDump(stdout, doc.get(), 1); return 0; } -- 2.34.1 From c61231cc5ea3380a358a4406e740cf1f2bca6019 Mon Sep 17 00:00:00 2001 From: Jonathan Rajotte Date: Wed, 24 Feb 2021 14:24:09 -0500 Subject: [PATCH 07/16] Clean-up: ust-app: use const condition and event rule MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit At this point only const object are required. Signed-off-by: Jonathan Rajotte Signed-off-by: Jérémie Galarneau Change-Id: I9c297fdd6e8026db4e0dc56bd9238fec05fbae3b --- src/bin/lttng-sessiond/ust-app.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/bin/lttng-sessiond/ust-app.cpp b/src/bin/lttng-sessiond/ust-app.cpp index 12250edfd..d993b7b44 100644 --- a/src/bin/lttng-sessiond/ust-app.cpp +++ b/src/bin/lttng-sessiond/ust-app.cpp @@ -6025,8 +6025,8 @@ static void ust_app_synchronize_event_notifier_rules(struct ust_app *app) } for (i = 0; i < count; i++) { - struct lttng_condition *condition; - struct lttng_event_rule *event_rule; + const struct lttng_condition *condition; + const struct lttng_event_rule *event_rule; struct lttng_trigger *trigger; const struct ust_app_event_notifier_rule *looked_up_event_notifier_rule; enum lttng_condition_status condition_status; @@ -6036,7 +6036,7 @@ static void ust_app_synchronize_event_notifier_rules(struct ust_app *app) LTTNG_ASSERT(trigger); token = lttng_trigger_get_tracer_token(trigger); - condition = lttng_trigger_get_condition(trigger); + condition = lttng_trigger_get_const_condition(trigger); if (lttng_condition_get_type(condition) != LTTNG_CONDITION_TYPE_EVENT_RULE_MATCHES) { @@ -6044,8 +6044,8 @@ static void ust_app_synchronize_event_notifier_rules(struct ust_app *app) continue; } - condition_status = lttng_condition_event_rule_matches_borrow_rule_mutable( - condition, &event_rule); + condition_status = + lttng_condition_event_rule_matches_get_rule(condition, &event_rule); LTTNG_ASSERT(condition_status == LTTNG_CONDITION_STATUS_OK); if (lttng_event_rule_get_domain_type(event_rule) == LTTNG_DOMAIN_KERNEL) { -- 2.34.1 From a6df2497a5c3d9b46b049d6fa0b2fd8a1965cf8a Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Tue, 23 Apr 2024 16:46:47 -0400 Subject: [PATCH 08/16] Import CStringView from the Babeltrace tree MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The class is imported as of 1dcaf4b74 in the babeltrace tree. It is renamed and reformated to fit the LTTng-tools conventions. No changes in behaviour are intended. Signed-off-by: Jérémie Galarneau Change-Id: I8acc3833c52c4ac23a91af87157aade0b4bbb7c1 --- src/common/Makefile.am | 2 + src/common/string-utils/c-string-view.hpp | 262 ++++++++++++++++++++++ src/common/type-traits.hpp | 38 ++++ 3 files changed, 302 insertions(+) create mode 100644 src/common/string-utils/c-string-view.hpp create mode 100644 src/common/type-traits.hpp diff --git a/src/common/Makefile.am b/src/common/Makefile.am index 34fd83452..8170e6e5a 100644 --- a/src/common/Makefile.am +++ b/src/common/Makefile.am @@ -115,6 +115,7 @@ libcommon_lgpl_la_SOURCES = \ time.cpp \ tracker.cpp tracker.hpp \ trigger.cpp \ + type-traits.hpp \ unix.cpp unix.hpp \ uri.cpp uri.hpp \ userspace-probe.cpp \ @@ -432,6 +433,7 @@ endif # libstring-utils noinst_LTLIBRARIES += libstring-utils.la libstring_utils_la_SOURCES = \ + string-utils/c-string-view.hpp \ string-utils/format.hpp \ string-utils/string-utils.cpp \ string-utils/string-utils.hpp diff --git a/src/common/string-utils/c-string-view.hpp b/src/common/string-utils/c-string-view.hpp new file mode 100644 index 000000000..6b175ea33 --- /dev/null +++ b/src/common/string-utils/c-string-view.hpp @@ -0,0 +1,262 @@ +/* + * Copyright (c) 2023 Philippe Proulx + * + * SPDX-License-Identifier: MIT + */ + +#ifndef LTTNG_C_STRING_VIEW_HPP +#define LTTNG_C_STRING_VIEW_HPP + +#include +#include + +#include +#include +#include +#include + +namespace lttng { + +/* + * A view on a constant null-terminated C string. + */ +class c_string_view final { +public: + /* + * Builds an empty view (data() returns `nullptr`). + * + * Intentionally not explicit. + */ + constexpr c_string_view() noexcept = default; + + /* + * Builds a view of the C string `str` (may be `nullptr`). + * + * Intentionally not explicit. + */ + /* NOLINTBEGIN(google-explicit-constructor) */ + constexpr c_string_view(const char *const str) noexcept : _str{ str } + { + } + /* NOLINTEND(google-explicit-constructor) */ + + /* + * Builds a view of the string `str`. + */ + /* NOLINTBEGIN(google-explicit-constructor) */ + c_string_view(const std::string& str) noexcept : _str{ str.c_str() } + { + } + /* NOLINTEND */ + + /* + * Makes this view view the C string `str` (may be `nullptr`). + */ + c_string_view& operator=(const char *const str) noexcept + { + _str = str; + return *this; + } + + /* + * Viewed null-terminated C string (may be `nullptr`). + */ + const char *data() const noexcept + { + return _str; + } + + /* + * Alias of data(). + */ + operator const char *() const noexcept /* NOLINT(google-explicit-constructor) */ + { + return this->data(); + } + + /* + * Evaluate as boolean (false means an empty string). + */ + operator bool() const noexcept /* NOLINT(google-explicit-constructor) */ + { + return *this->data(); + } + + /* + * Alias of data(). + */ + const char *operator*() const noexcept + { + return this->data(); + } + + /* + * Alias of data(). + * + * data() must not return `nullptr`. + */ + const char *begin() const noexcept + { + return this->data(); + } + + /* + * Pointer to the null character of the viewed C string. + * + * data() must not return `nullptr`. + */ + const char *end() const noexcept + { + return _str + this->len(); + } + + /* + * Length of the viewed C string, excluding the null character. + * + * data() must not return `nullptr`. + */ + std::size_t len() const noexcept + { + return std::strlen(_str); + } + + /* + * Returns an `std::string` instance containing a copy of the viewed + * C string. + * + * data() must not return `nullptr`. + */ + std::string str() const + { + return std::string{ _str }; + } + + /* + * Alias of str(). + */ + operator std::string() const /* NOLINT(google-explicit-constructor) */ + { + return this->str(); + } + + /* + * Returns the character at index `i`. + * + * `i` must be less than what len() returns. + * + * data() must not return `nullptr`. + */ + char operator[](const std::size_t i) const noexcept + { + return _str[i]; + } + + bool startsWith(const lttng::c_string_view prefix) const noexcept + { + return std::strncmp(_str, (const char *) prefix, prefix.len()) == 0; + } + +private: + const char *_str = nullptr; +}; + +inline const char *format_as(const c_string_view& str) +{ + return str ? *str : "(null)"; +} + +namespace internal { + +template +const char *as_const_char_ptr(StrT&& val) noexcept +{ + return val.data(); +} + +inline const char *as_const_char_ptr(const char *const val) noexcept +{ + return val; +} + +template +using comparable_with_c_string_view = lttng::traits:: + is_one_of::type, c_string_view, std::string, const char *>; + +} /* namespace internal */ + +/* + * Returns true if `lhs` is equal to `rhs`. + * + * `LhsT` and `RhsT` may be any of: + * + * • `const char *` + * • `std::string` + * • `c_string_view` + * + * Both `lhs` and `rhs` must not have an underlying `nullptr` raw data. + */ +template < + typename LhsT, + typename RhsT, + typename = + typename std::enable_if::value>::type, + typename = + typename std::enable_if::value>::type> +bool operator==(LhsT&& lhs, RhsT&& rhs) noexcept +{ + const auto raw_lhs = internal::as_const_char_ptr(lhs); + const auto raw_rhs = internal::as_const_char_ptr(rhs); + + return std::strcmp(raw_lhs, raw_rhs) == 0; +} + +/* + * Returns true if `lhs` is not equal to `rhs`. + * + * `LhsT` and `RhsT` may be any of: + * + * • `const char *` + * • `std::string` + * • `c_string_view` + * + * Both `lhs` and `rhs` must not have an underlying `nullptr` raw data. + */ +template < + typename LhsT, + typename RhsT, + typename = + typename std::enable_if::value>::type, + typename = + typename std::enable_if::value>::type> +bool operator!=(LhsT&& lhs, RhsT&& rhs) noexcept +{ + return !(std::forward(lhs) == std::forward(rhs)); +} + +} /* namespace lttng */ + +/* + * Appends `rhs` to `lhs`. + */ +inline void operator+=(std::string& lhs, lttng::c_string_view rhs) +{ + lhs += rhs.data(); +} + +namespace std { +template <> +struct hash { + std::size_t operator()(const lttng::c_string_view& str) const + { + auto hash_value = std::hash{}('\0'); + + for (auto character : str) { + hash_value ^= std::hash{}(character); + } + + return hash_value; + } +}; +} /* namespace std */ + +#endif /* LTTNG_C_STRING_VIEW_HPP */ diff --git a/src/common/type-traits.hpp b/src/common/type-traits.hpp new file mode 100644 index 000000000..3801bf13f --- /dev/null +++ b/src/common/type-traits.hpp @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2023 Philippe Proulx + * + * SPDX-License-Identifier: MIT + */ + +#ifndef LTTNG_TYPE_TRAITS_HPP +#define LTTNG_TYPE_TRAITS_HPP + +#include + +namespace lttng { +namespace traits { + +/* + * Provides the member constant `value` equal to: + * + * `T` is in the list of types `Ts`: + * `true` + * + * Otherwise: + * `false` + */ +template +struct is_one_of : std::false_type { +}; + +template +struct is_one_of : std::true_type { +}; + +template +struct is_one_of : is_one_of { +}; +} /* namespace traits */ +} /* namespace lttng */ + +#endif /* LTTNG_TYPE_TRAITS_HPP */ -- 2.34.1 From fc67b8bfaeeff5f8355fb336df75651de1963ccf Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Mon, 13 May 2024 14:41:41 -0400 Subject: [PATCH 09/16] .gitignore: ignore local vscode workspace settings file MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Signed-off-by: Jérémie Galarneau Change-Id: I518d85077566ab341acb4644d27132ade79b5749 --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index b0884eb16..66e76696f 100644 --- a/.gitignore +++ b/.gitignore @@ -47,6 +47,7 @@ TAGS /.clangd/ compile_commands.json *_flymake* +/.vscode/settings.json # m4 macros not automatically generated /m4/libtool.m4 -- 2.34.1 From 8a8bca410a240a7177deb09201878bad7a41d46b Mon Sep 17 00:00:00 2001 From: Kienan Stewart Date: Fri, 10 May 2024 13:15:38 -0400 Subject: [PATCH 10/16] tests: Correct text when enabling a small overwrite kernel channel MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Change-Id: I937aa6334c4f3ab0788ef1898ac9b323e82ea44c Signed-off-by: Kienan Stewart Signed-off-by: Jérémie Galarneau --- tests/utils/utils.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/utils.sh b/tests/utils/utils.sh index 0e8ec54dd..55c821d60 100644 --- a/tests/utils/utils.sh +++ b/tests/utils/utils.sh @@ -1337,7 +1337,7 @@ function enable_lttng_mmap_overwrite_small_kernel_channel() _run_lttng_cmd "$OUTPUT_DEST" "$ERROR_OUTPUT_DEST" \ enable-channel -s $sess_name $channel_name -k --output mmap --overwrite --subbuf-size=$(getconf PAGE_SIZE) --num-subbuf=2 - ok $? "Enable small discard channel $channel_name for session $sess_name" + ok $? "Enable small overwrite channel $channel_name for session $sess_name" } function enable_lttng_mmap_overwrite_ust_channel() -- 2.34.1 From 849019151cf07f376a4a68ce1669e357f4775657 Mon Sep 17 00:00:00 2001 From: Kienan Stewart Date: Fri, 10 May 2024 14:41:11 -0400 Subject: [PATCH 11/16] tests: Correct use of taskset in snapshot tests MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== While investigating potential changes to how buffers are flushed during snapshots, I discovered that the changes made the small overwrite test flaky. After investigation, the issue appears to be a problem with the test that was exposed by the other changes. Cause ===== `taskset` handles its arguments more strictly than many other CLI programs, and the argument order used is not valid. As the return of the taskset invocation in the test isn't validated, there was little indication of the issue save a warning that is easily lost in the text. The following example demonstrates the situation: ``` $ bash -x test.sh + taskset -p 910843 pid 910843's current affinity mask: f + echo 0 0 + ./tests/utils/testapp/gen-ust-events/gen-ust-events Process 910845 has 4 cpus in its affinity set CPU 0 is set? 1 CPU 1 is set? 1 CPU 2 is set? 1 CPU 3 is set? 1 + taskset -c 0 -p 910843 taskset: failed to execute -p: No such file or directory + echo 127 127 + ./tests/utils/testapp/gen-ust-events/gen-ust-events Process 910849 has 4 cpus in its affinity set CPU 0 is set? 1 CPU 1 is set? 1 CPU 2 is set? 1 CPU 3 is set? 1 + taskset -pc 0 910843 pid 910843's current affinity list: 0-3 pid 910843's new affinity list: 0 + echo 0 0 + ./tests/utils/testapp/gen-ust-events/gen-ust-events Process 910853 has 1 cpus in its affinity set CPU 0 is set? 1 ``` Solution ======== Correct the invocation of taskset and add a check on its return code when used to set the cpu affinity of the current process. Known drawbacks =============== None. Change-Id: Ia629121624532746431875b2031dd65df207666d Signed-off-by: Kienan Stewart Signed-off-by: Jérémie Galarneau --- tests/regression/tools/snapshots/ust_test | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/regression/tools/snapshots/ust_test b/tests/regression/tools/snapshots/ust_test index 5188c2b5b..e49549804 100755 --- a/tests/regression/tools/snapshots/ust_test +++ b/tests/regression/tools/snapshots/ust_test @@ -16,7 +16,7 @@ TESTAPP_NAME="gen-ust-events" TESTAPP_BIN="$TESTAPP_PATH/$TESTAPP_NAME/$TESTAPP_NAME" APPS_PID=() -NUM_TESTS=104 +NUM_TESTS=106 TRACE_PATH=$(mktemp -d -t tmp.test_snapshots_ust_trace_path.XXXXXX) @@ -209,7 +209,8 @@ function test_ust_local_snapshot_small_discard_buffers () OLDCPUSET=$(taskset -p $$) diag "Test local UST snapshots with small discard buffers" - taskset -c "$(get_any_available_cpu)" -p $$ 1>/dev/null 2>&1 + taskset -cp "$(get_any_available_cpu)" $$ 1>/dev/null 2>&1 + ok $? "Set current process CPU affinity" create_lttng_session_no_output "$SESSION_NAME" enable_mmap_small_discard_ust_channel "$SESSION_NAME" $CHANNEL_NAME enable_ust_lttng_event_ok "$SESSION_NAME" $EVENT_NAME $CHANNEL_NAME @@ -254,7 +255,8 @@ function test_ust_local_snapshot_small_overwrite_buffers () OLDCPUSET=$(taskset -p $$) diag "Test local UST snapshots with small overwrite buffers" - taskset -p "$(get_any_available_cpu)" $$ 1>/dev/null 2>&1 + taskset -cp "$(get_any_available_cpu)" $$ 1>/dev/null 2>&1 + ok $? "Set current process CPU affinity" create_lttng_session_no_output "$SESSION_NAME" enable_mmap_small_overwrite_ust_channel "$SESSION_NAME" $CHANNEL_NAME enable_ust_lttng_event_ok "$SESSION_NAME" $EVENT_NAME $CHANNEL_NAME -- 2.34.1 From cfcb1e562fa9517544e1b2b02e73311390721f73 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Tue, 14 May 2024 13:58:55 +0000 Subject: [PATCH 12/16] .clang-tidy: remove modernize-concat-nested-namespaces MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The concatenated namespace syntax (e.g. namespace my::ns::woah{}) was introduced in C++17 while this project is built as C++11. Leaving this rule it in the .clang-tidy only adds noise (and salt to the wound). Change-Id: Ib9550aa602fbcccc7500b14603dfd698fd7b69de Signed-off-by: Jérémie Galarneau --- .clang-tidy | 1 - 1 file changed, 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index 5b6f3705b..d2a18789e 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -70,7 +70,6 @@ Checks: '-*, misc-unused-using-decls, misc-use-anonymous-namespace, modernize-avoid-bind, - modernize-concat-nested-namespaces, modernize-loop-convert, modernize-make-shared, modernize-make-unique, -- 2.34.1 From c0a36edc2ce1f1ce6e9f014e111b0003cd6af122 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 22 May 2024 16:15:39 -0400 Subject: [PATCH 13/16] docs: lttng-sessiond(8): bitness mix-up in env variable name MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The section about the --consumerd64-path parameter erroneously refers to the 32-bit consumer daemon binary environment variable (LTTNG_CONSUMERD32_BIN). It is modified to mention LTTNG_CONSUMERD64_BIN. Signed-off-by: Jérémie Galarneau Change-Id: I338d204382b8bfca952abf604fe82277b7b08226 --- doc/man/lttng-sessiond.8.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/man/lttng-sessiond.8.txt b/doc/man/lttng-sessiond.8.txt index 2bfce1c37..57b05ecc0 100644 --- a/doc/man/lttng-sessiond.8.txt +++ b/doc/man/lttng-sessiond.8.txt @@ -232,7 +232,7 @@ See also the `LTTNG_CONSUMERD64_LIBDIR` environment variable. option:--consumerd64-path='PATH':: Set the 64-bit consumer daemon binary path to 'PATH'. + -See also the `LTTNG_CONSUMERD32_BIN` environment variable. +See also the `LTTNG_CONSUMERD64_BIN` environment variable. option:--kconsumerd-cmd-sock='PATH':: Set the command Unix socket path of the Linux kernel consumer daemon -- 2.34.1 From ac559d09a23994f9f52014b0efba3a3c566dcfea Mon Sep 17 00:00:00 2001 From: Kienan Stewart Date: Fri, 17 May 2024 13:15:25 -0400 Subject: [PATCH 14/16] lttng: Add PWARN macro MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Change-Id: Id6d15e094b8fe7ef779022e44d8414214af3444a Signed-off-by: Kienan Stewart Signed-off-by: Jérémie Galarneau --- src/common/error.hpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/common/error.hpp b/src/common/error.hpp index 23d840c2d..66f38bf2e 100644 --- a/src/common/error.hpp +++ b/src/common/error.hpp @@ -226,6 +226,7 @@ static inline void __lttng_print_check_abort(enum lttng_error_level type) } while (0); #define _PERROR(fmt, args...) _ERRMSG("PERROR", PRINT_ERR, fmt, ##args) +#define _PWARN(fmt, args...) _ERRMSG("PWARN", PRINT_WARN, fmt, ##args) #if !defined(__GLIBC__) || \ ((_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !defined(_GNU_SOURCE)) @@ -239,6 +240,13 @@ static inline void __lttng_print_check_abort(enum lttng_error_level type) strerror_r(errno, _perror_buf, sizeof(_perror_buf)); \ _PERROR(call ": %s", ##args, _perror_buf); \ } while (0); + +#define PWARN(call, args...) \ + do { \ + char _perror_buf[200]; \ + strerror_r(errno, _perror_buf, sizeof(_perror_buf)); \ + _PWARN(call ": %s", ##args, _perror_buf); \ + } while (0); #else /* * Version using GNU strerror_r, for linux with appropriate defines. @@ -250,6 +258,13 @@ static inline void __lttng_print_check_abort(enum lttng_error_level type) _perror_buf = strerror_r(errno, _perror_tmp, sizeof(_perror_tmp)); \ _PERROR(call ": %s", ##args, _perror_buf); \ } while (0); +#define PWARN(call, args...) \ + do { \ + char *_perror_buf; \ + char _perror_tmp[200]; \ + _perror_buf = strerror_r(errno, _perror_tmp, sizeof(_perror_tmp)); \ + _PWARN(call ": %s", ##args, _perror_buf); \ + } while (0); #endif const char *error_get_str(int32_t code); -- 2.34.1 From dcffe9462d11f9de5b441b801da5b2b7ae42c79a Mon Sep 17 00:00:00 2001 From: Kienan Stewart Date: Mon, 15 Apr 2024 15:15:28 -0400 Subject: [PATCH 15/16] lttng: Indicate file path and error reason when open_config fails MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== In a test environment where the configuration file had been chowned to no longer be writeable by the current user, `lttng` commands would work but return a non-zero exit code and print `Error: Unable to create config file`. This doesn't give the user much information to work with to address the issue. Solution ======== The `PWARN` macro is used to print the file_path and reason when the call to `fopen` fails. This is done since the `file_path` percolated up to where the existing error message is may be null for multiple reasons (eg. allocation failure). Known drawbacks =============== The output of `PWARN` is inconsistent with the output used for most of the `lttng` client. Eg. ``` $ lttng create Spawning a session daemon Session auto-20240415-151450 created. Traces will be output to /home/lttng-traces/auto-20240415-151450 PWARN - 15:14:50.420628389 [1184515/1184515]: fopen '/home/.lttngrc': Permission denied (in open_config() at conf.cpp:57) Error: Unable to create config file ``` Change-Id: I5a9253fb02f3a712e7c5c2567311920dc33d36c7 Signed-off-by: Kienan Stewart Signed-off-by: Jérémie Galarneau --- src/bin/lttng/commands/create.cpp | 3 ++- src/bin/lttng/conf.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bin/lttng/commands/create.cpp b/src/bin/lttng/commands/create.cpp index dce13866f..e1756c3d3 100644 --- a/src/bin/lttng/commands/create.cpp +++ b/src/bin/lttng/commands/create.cpp @@ -479,7 +479,8 @@ static int create_session(const char *session_name) /* Init lttng session config */ ret = config_init(created_session_name); if (ret < 0) { - ret = CMD_ERROR; + MSG("Unable to initialize configuration for created session: future commands will require the target session name explicitly"); + ret = CMD_WARNING; goto error; } diff --git a/src/bin/lttng/conf.cpp b/src/bin/lttng/conf.cpp index 65be35a15..81aa65438 100644 --- a/src/bin/lttng/conf.cpp +++ b/src/bin/lttng/conf.cpp @@ -54,6 +54,7 @@ static FILE *open_config(const char *path, const char *mode) fp = fopen(file_path, mode); if (fp == nullptr) { + PWARN("Failed to open configuration file '%s'", file_path); goto error; } @@ -74,7 +75,6 @@ static int create_config_file(const char *path) fp = open_config(path, "w+"); if (fp == nullptr) { - ERR("Unable to create config file"); ret = -1; goto error; } -- 2.34.1 From fbedc3dceae917e81ae37d378bec0b6bf1304901 Mon Sep 17 00:00:00 2001 From: Kienan Stewart Date: Wed, 22 May 2024 10:45:08 -0400 Subject: [PATCH 16/16] tests: Make test_per_application_leaks more robust MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== On some systems (e.g. sles15sp4, RHEL 7), test_per_application_leaks would fail spuriously. Cause ===== When walking the the FDs in `/proc/XX/fds`, the symbolic links exist but link to unlinked files. E.g. ``` lrwx------ 1 root root 64 May 22 14:49 /proc/83578/fd/58 -> '/dev/shm/shm-ust-consumer-83578 (deleted)' ``` Solution ======== The test has been modified to loop, waiting for the post-destroy shm count to drop back to the post-start count. In the case of a failure, the test will hang forever but doesn't fail spuriously. Known drawbacks =============== None. Change-Id: Id3c8a9f6db83fe888e79b8f06cb8308b4d90da87 Signed-off-by: Kienan Stewart Signed-off-by: Jérémie Galarneau --- .../tools/live/test_per_application_leaks.py | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/tests/regression/tools/live/test_per_application_leaks.py b/tests/regression/tools/live/test_per_application_leaks.py index 1d12049ed..60267810c 100755 --- a/tests/regression/tools/live/test_per_application_leaks.py +++ b/tests/regression/tools/live/test_per_application_leaks.py @@ -14,6 +14,7 @@ import os import pathlib import subprocess import sys +import time test_utils_import_path = pathlib.Path(__file__).absolute().parents[3] / "utils" sys.path.append(str(test_utils_import_path)) @@ -22,7 +23,7 @@ import lttngtest def get_consumerd_pid(tap, parent, match_string): - pid = 0 + pid = None try: process = subprocess.Popen( ["pgrep", "-P", str(parent), "-f", match_string], @@ -30,13 +31,14 @@ def get_consumerd_pid(tap, parent, match_string): ) process.wait() output = str(process.stdout.read(), encoding="UTF-8").splitlines() - if len(output) != 1: + if len(output) > 1: raise Exception( "Unexpected number of output lines (got {}): {}".format( len(output), output ) ) - pid = int(output[0]) + elif len(output) == 1: + pid = int(output[0]) except Exception as e: tap.diagnostic( "Failed to find child process of '{}' matching '{}': '{}'".format( @@ -48,19 +50,23 @@ def get_consumerd_pid(tap, parent, match_string): def count_process_dev_shm_fds(pid): count = 0 - if pid == 0: + if pid is None: return count dir = os.path.join("/proc", str(pid), "fd") for root, dirs, files in os.walk(dir): for f in files: filename = pathlib.Path(os.path.join(root, f)) try: + # The symlink in /proc/PID may exist, but point to an unlinked + # file - shm_unlink is called but either the kernel hasn't yet + # finished the clean-up or the consumer hasn't called close() + # on the FD yet. if filename.is_symlink() and str(filename.resolve()).startswith( "/dev/shm/shm-ust-consumer" ): count += 1 except FileNotFoundError: - # As we're walking /proc/XX/fd/, fds may be added or removed + # As /proc/XX/fd/ is being walked, fds may be added or removed continue return count @@ -112,7 +118,18 @@ def test_fd_leak(tap, test_env, buffer_sharing_policy, kill_relayd=True): session.stop() session.destroy() - count_post_destroy = count_dev_shm_fds(tap, test_env) + # As there is not method to know exactly when the final close of the + # shm happens (it is timing dependant from an external point of view), + # this test iterates waiting for the post-destroy count to reach the + # post-start count. In a failure, this will loop infinitely. + tap.diagnostic( + "Waiting for post-destroy shm count to drop back to post-start level" + ) + while True: + count_post_destroy = count_dev_shm_fds(tap, test_env) + if count_post_destroy == count_post_start: + break + time.sleep(0.1) tap.diagnostic( "FD counts post-start: {}, post-destroy: {}".format( -- 2.34.1