From f9b485aec70c4f55a202cf3d1e4bfd416a091835 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 10 Sep 2024 15:45:40 -0400 Subject: [PATCH] iterator.c, muxer: tweak clock compatibility validation error messages In general, make the prose part of the messages be a bit more in line with the enumerator names, which usually ends up a bit more specific than the current messages. In muxer's msg-iter.cpp: - Split the ExpectingOriginUnixGotNone, ExpectingOriginUnknownWithUuidGotNone and ExpectingOriginUnknownWithoutUuidGotNone, to provide slightly more precise error messages about what kind of clock class is expected. - Always include the stream class information, whenever there is a stream class (message iterator inactivity messages don't have an associated stream class). Factor this out in a `formatStreamCls` lambda. - Factor out the formatting of the clock class info, and print an additional `clock-class-origin-is-unix-epoch` and `clock-class-uuid` fields. This factorisation will be helpful when adding support for MIP 1, since the fields to format will change. We will need to update fewer places. In iterator.c: - Print the actual clock class info in a few more places. - Change phrases like "has non Unix epoch origin" for "has unknown origin", both in human-readable messages and conditions IDs. - Tweak the "Expecting a clock class, got none." messages to be a bit more specific, differentiating the three cases. - Print the expected UUID whenever we expect a clock class with a specific UUID. Instead of cherry-picking the information we print for each case, we could also print the entire reference clock class information (making it clear that we don't necessarily expect this particular clock class instance, perhaps with a `reference-` prefix). It would probably make the implementation simpler, but it would print a bit more unnecessary information. The user would have to infer from the error message what is the relevant information in there. I don't have a strong preference for either approach. Change-Id: I0ce2ab5b146a79baa5aa0afbd9d5de4f79f283a1 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/12784 Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/lib/graph/iterator.c | 38 ++++--- src/plugins/utils/muxer/msg-iter.cpp | 100 ++++++++++++------ .../clk-cls-compat-postconds-triggers.cpp | 2 +- .../test-clock-compatibility.cpp | 30 +++--- 4 files changed, 110 insertions(+), 60 deletions(-) diff --git a/src/lib/graph/iterator.c b/src/lib/graph/iterator.c index d7119ee0..1724af33 100644 --- a/src/lib/graph/iterator.c +++ b/src/lib/graph/iterator.c @@ -696,41 +696,55 @@ void assert_post_dev_clock_classes_are_compatible_one( case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_NO_CLOCK_CLASS_GOT_ONE: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "stream-class-has-no-clock-class", false, - "Expecting no clock class, got one."); + "Expecting no clock class, got one: %![cc-]+K", + actual_clock_cls); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNIX_GOT_NO_CLOCK_CLASS: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "stream-class-has-clock-class-with-unix-epoch-origin", false, - "Expecting a clock class, got none."); + "Expecting a clock class with Unix epoch origin, got none."); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNIX_GOT_UNKNOWN_ORIGIN: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "clock-class-has-unix-epoch-origin", false, - "Expecting a clock class with Unix epoch origin: %![cc-]+K", + "Expecting a clock class with Unix epoch origin, got one with " + "unknown origin: %![cc-]+K", actual_clock_cls); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNKNOWN_WITH_UUID_GOT_NO_CLOCK_CLASS: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "stream-class-has-clock-class-with-uuid", false, - "Expecting a clock class, got none."); + "Expecting a clock class with unknown origin and a specific UUID, " + "got none: expected-uuid=%!u", + bt_clock_class_get_uuid(ref_clock_cls)); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNKNOWN_WITH_UUID_GOT_UNIX_ORIGIN: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, - "clock-class-has-non-unix-epoch-origin", false, - "Expecting a clock class without Unix epoch origin: %![cc-]+K", - actual_clock_cls); + "clock-class-has-unknown-origin", false, + "Expecting a clock class with unknown origin and a specific UUID, " + "got one with Unix epoch origin: %![cc-]+K, expected-uuid=%!u", + actual_clock_cls, bt_clock_class_get_uuid(ref_clock_cls)); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNKNOWN_WITH_UUID_GOT_WITHOUT_UUID: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "clock-class-has-uuid", false, - "Expecting a clock class with UUID: %![cc-]+K", - actual_clock_cls); + "Expecting a clock class with unknown origin and a specific UUID, " + "got one without a UUID: %![cc-]+K, expected-uuid=%!u", + actual_clock_cls, bt_clock_class_get_uuid(ref_clock_cls)); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNKNOWN_WITH_UUID_GOT_OTHER_UUID: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "clock-class-has-expected-uuid", false, - "Expecting a clock class with UUID, got one with a different UUID: " - "%![cc-]+K, expected-uuid=%!u", + "Expecting a clock class with unknown origin and a specific UUID, " + "got one with a different UUID: %![cc-]+K, expected-uuid=%!u", actual_clock_cls, bt_clock_class_get_uuid(ref_clock_cls)); case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNKNOWN_WITHOUT_UUID_GOT_NO_CLOCK_CLASS: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "stream-class-has-clock-class", false, - "Expecting a clock class, got none."); + "Expecting a clock class, got none: %![expected-cc-]+K", + ref_clock_cls); + case BT_CLOCK_CORRELATION_VALIDATOR_ERROR_TYPE_EXPECTING_ORIGIN_UNKNOWN_WITHOUT_UUID_GOT_OTHER_CLOCK_CLASS: BT_ASSERT_POST_DEV(NEXT_METHOD_NAME, "clock-class-is-expected", false, diff --git a/src/plugins/utils/muxer/msg-iter.cpp b/src/plugins/utils/muxer/msg-iter.cpp index cc719898..3cdc1a54 100644 --- a/src/plugins/utils/muxer/msg-iter.cpp +++ b/src/plugins/utils/muxer/msg-iter.cpp @@ -252,62 +252,98 @@ void MsgIter::_validateMsgClkCls(const bt2::ConstMessage msg) const auto actualClkCls = error.actualClockCls(); const auto refClkCls = error.refClockCls(); + const auto formatClkClsOrigin = [](const bt2::ConstClockClass clockCls, + const char * const prefix) { + return fmt::format("{}clock-class-origin-is-unix-epoch={}", prefix, + clockCls.origin().isUnixEpoch()); + }; + const auto formatClkClsUuid = [](const bt2::ConstClockClass clockCls, + const char * const prefix) { + if (const auto uuid = clockCls.uuid()) { + return fmt::format("{}clock-class-uuid={}", prefix, *uuid); + } else { + return fmt::format("{}clock-class-uuid=(none)", prefix); + } + }; + const auto formatExpClkClsUuid = [&] { + return formatClkClsUuid(*refClkCls, "expected-"); + }; + const auto formatClkCls = [&](const bt2::ConstClockClass clockCls, + const char * const prefix) { + return fmt::format("{}clock-class-addr={}, {}clock-class-name={}, {}, {}", prefix, + fmt::ptr(clockCls.libObjPtr()), prefix, clockCls.name(), + formatClkClsOrigin(clockCls, prefix), + formatClkClsUuid(clockCls, prefix)); + }; + const auto formatActClkCls = [&] { + return formatClkCls(*actualClkCls, ""); + }; + const auto formatExpClkCls = [&] { + return formatClkCls(*refClkCls, "expected-"); + }; + const auto formatStreamCls = [&](const bool withTrailingComma) { + if (const auto streamCls = error.streamCls()) { + return fmt::format( + "stream-class-addr={}, stream-class-name=\"{}\", stream-class-id={}{}", + fmt::ptr(streamCls->libObjPtr()), streamCls->name(), streamCls->id(), + withTrailingComma ? ", " : ""); + } else { + return std::string {}; + } + }; switch (error.type()) { case Type::ExpectingNoClockClassGotOne: - BT_CPPLOGE_APPEND_CAUSE_AND_THROW( - bt2::Error, - "Expecting no clock class, but got one: clock-class-addr={}, clock-class-name={}", - fmt::ptr(actualClkCls->libObjPtr()), actualClkCls->name()); + BT_CPPLOGE_APPEND_CAUSE_AND_THROW(bt2::Error, "Expecting no clock class, got one: {}{}", + formatStreamCls(true), formatActClkCls()); case Type::ExpectingOriginUnixGotNoClockClass: - case Type::ExpectingOriginUnknownWithUuidGotNoClockClass: - case Type::ExpectingOriginUnknownWithoutUuidGotNoClockClass: BT_CPPLOGE_APPEND_CAUSE_AND_THROW( - bt2::Error, - "Expecting a clock class, but got none: stream-class-addr={}, " - "stream-class-name=\"{}\", stream-class-id={}", - fmt::ptr(error.streamCls()->libObjPtr()), error.streamCls()->name(), - error.streamCls()->id()); + bt2::Error, "Expecting a clock class with Unix epoch origin, got none: {}", + formatStreamCls(false)); case Type::ExpectingOriginUnixGotUnknownOrigin: BT_CPPLOGE_APPEND_CAUSE_AND_THROW( bt2::Error, - "Expecting a clock class having a Unix epoch origin, but got one not having a Unix " - "epoch origin: clock-class-addr={}, clock-class-name={}", - fmt::ptr(actualClkCls->libObjPtr()), actualClkCls->name()); + "Expecting a clock class with Unix epoch origin, got one with unknown " + "origin: {}{}", + formatStreamCls(true), formatActClkCls()); + + case Type::ExpectingOriginUnknownWithUuidGotNoClockClass: + BT_CPPLOGE_APPEND_CAUSE_AND_THROW( + bt2::Error, + "Expecting a clock class with unknown origin and a specific UUID, got none: {}", + formatStreamCls(true), formatExpClkClsUuid()); case Type::ExpectingOriginUnknownWithUuidGotUnixOrigin: BT_CPPLOGE_APPEND_CAUSE_AND_THROW( bt2::Error, - "Expecting a clock class not having a Unix epoch origin, " - "but got one having a Unix epoch origin: " - "clock-class-addr={}, clock-class-name={}", - fmt::ptr(actualClkCls->libObjPtr()), actualClkCls->name()); + "Expecting a clock class with unknown origin and a specific UUID, got one " + "with Unix epoch origin: {}{}, {}", + formatStreamCls(true), formatActClkCls(), formatExpClkClsUuid()); case Type::ExpectingOriginUnknownWithUuidGotWithoutUuid: BT_CPPLOGE_APPEND_CAUSE_AND_THROW( bt2::Error, - "Expecting a clock class with a UUID, but got one without a UUID: " - "clock-class-addr={}, clock-class-name={}", - fmt::ptr(actualClkCls->libObjPtr()), actualClkCls->name()); + "Expecting a clock class with unknown origin and a specific UUID, got one " + "without a UUID: {}{}, {}", + formatStreamCls(true), formatActClkCls(), formatExpClkClsUuid()); case Type::ExpectingOriginUnknownWithUuidGotOtherUuid: BT_CPPLOGE_APPEND_CAUSE_AND_THROW( bt2::Error, - "Expecting a clock class with a specific UUID, but got one with a different UUID: " - "clock-class-addr={}, clock-class-name={}, expected-uuid=\"{}\", uuid=\"{}\"", - fmt::ptr(actualClkCls->libObjPtr()), actualClkCls->name(), *refClkCls->uuid(), - *actualClkCls->uuid()); + "Expecting a clock class with unknown origin and a specific UUID, got one with " + "a different UUID: {}{}, {}", + formatStreamCls(true), formatActClkCls(), formatExpClkClsUuid()); + + case Type::ExpectingOriginUnknownWithoutUuidGotNoClockClass: + BT_CPPLOGE_APPEND_CAUSE_AND_THROW(bt2::Error, "Expecting a clock class, got none: {}{}", + formatStreamCls(true), formatExpClkCls()); case Type::ExpectingOriginUnknownWithoutUuidGotOtherClockClass: - BT_CPPLOGE_APPEND_CAUSE_AND_THROW( - bt2::Error, - "Unexpected clock class: " - "expected-clock-class-addr={}, expected-clock-class-name={}, " - "actual-clock-class-addr={}, actual-clock-class-name={}", - fmt::ptr(refClkCls->libObjPtr()), refClkCls->name(), - fmt::ptr(actualClkCls->libObjPtr()), actualClkCls->name()); + BT_CPPLOGE_APPEND_CAUSE_AND_THROW(bt2::Error, "Unexpected clock class: {}{}, {}", + formatStreamCls(true), formatActClkCls(), + formatExpClkCls()); } } } diff --git a/tests/lib/conds/clk-cls-compat-postconds-triggers.cpp b/tests/lib/conds/clk-cls-compat-postconds-triggers.cpp index 0c7675b4..34429871 100644 --- a/tests/lib/conds/clk-cls-compat-postconds-triggers.cpp +++ b/tests/lib/conds/clk-cls-compat-postconds-triggers.cpp @@ -190,7 +190,7 @@ void addClkClsCompatTriggers(CondTriggers& triggers) [](const bt2::SelfComponent self) { return self.createClockClass(); }, - "message-iterator-class-next-method:clock-class-has-non-unix-epoch-origin"); + "message-iterator-class-next-method:clock-class-has-unknown-origin"); addValidCases( [](const bt2::SelfComponent self) { diff --git a/tests/plugins/flt.utils.muxer/test-clock-compatibility.cpp b/tests/plugins/flt.utils.muxer/test-clock-compatibility.cpp index 29b847bb..02a72be1 100644 --- a/tests/plugins/flt.utils.muxer/test-clock-compatibility.cpp +++ b/tests/plugins/flt.utils.muxer/test-clock-compatibility.cpp @@ -333,13 +333,13 @@ const ErrorTestCase errorTestCases[] = { [](const bt2::SelfComponent self) { return self.createClockClass(); }, - "no clock class followed by clock class", "Expecting no clock class, but got one"}, + "no clock class followed by clock class", "Expecting no clock class, got one"}, {[](const bt2::SelfComponent self) { return self.createClockClass(); }, noClockClass, "clock class with Unix epoch origin followed by no clock class", - "Expecting a clock class, but got none"}, + "Expecting a clock class with Unix epoch origin, got none"}, {[](const bt2::SelfComponent self) { return self.createClockClass(); @@ -350,8 +350,8 @@ const ErrorTestCase errorTestCases[] = { clockCls->originIsUnixEpoch(false); return clockCls; }, - "clock class with Unix epoch origin followed by clock class with other origin", - "Expecting a clock class having a Unix epoch origin, but got one not having a Unix epoch origin"}, + "clock class with Unix epoch origin followed by clock class with unknown origin", + "Expecting a clock class with Unix epoch origin, got one with unknown origin"}, {[](const bt2::SelfComponent self) { const auto clockCls = self.createClockClass(); @@ -359,8 +359,8 @@ const ErrorTestCase errorTestCases[] = { clockCls->originIsUnixEpoch(false).uuid(uuidA); return clockCls; }, - noClockClass, "clock class with other origin and a UUID followed by no clock class", - "Expecting a clock class, but got none"}, + noClockClass, "clock class with unknown origin and a UUID followed by no clock class", + "Expecting a clock class with unknown origin and a specific UUID, got none"}, {[](const bt2::SelfComponent self) { const auto clockCls = self.createClockClass(); @@ -371,8 +371,8 @@ const ErrorTestCase errorTestCases[] = { [](const bt2::SelfComponent self) { return self.createClockClass(); }, - "clock class with other origin and a UUID followed by clock class with Unix epoch origin", - "Expecting a clock class not having a Unix epoch origin, but got one having a Unix epoch origin"}, + "clock class with unknown origin and a UUID followed by clock class with Unix epoch origin", + "Expecting a clock class with unknown origin and a specific UUID, got one with Unix epoch origin"}, {[](const bt2::SelfComponent self) { const auto clockCls = self.createClockClass(); @@ -386,8 +386,8 @@ const ErrorTestCase errorTestCases[] = { clockCls->originIsUnixEpoch(false); return clockCls; }, - "clock class with other origin and a UUID followed by clock class with other origin and no UUID", - "Expecting a clock class with a UUID, but got one without a UUID"}, + "clock class with unknown origin and a UUID followed by clock class with unknown origin and no UUID", + "Expecting a clock class with unknown origin and a specific UUID, got one without a UUID"}, {[](const bt2::SelfComponent self) { const auto clockCls = self.createClockClass(); @@ -401,8 +401,8 @@ const ErrorTestCase errorTestCases[] = { clockCls->originIsUnixEpoch(false).uuid(uuidB); return clockCls; }, - "clock class with other origin and a UUID followed by clock class with other origin and another UUID", - "Expecting a clock class with a specific UUID, but got one with a different UUID"}, + "clock class with unknown origin and a UUID followed by clock class with unknown origin and another UUID", + "Expecting a clock class with unknown origin and a specific UUID, got one with a different UUID"}, {[](const bt2::SelfComponent self) { const auto clockCls = self.createClockClass(); @@ -410,8 +410,8 @@ const ErrorTestCase errorTestCases[] = { clockCls->originIsUnixEpoch(false); return clockCls; }, - noClockClass, "clock class with other origin and no UUID followed by no clock class", - "Expecting a clock class, but got none"}, + noClockClass, "clock class with unknown origin and no UUID followed by no clock class", + "Expecting a clock class, got none"}, {[](const bt2::SelfComponent self) { const auto clockCls = self.createClockClass(); @@ -426,7 +426,7 @@ const ErrorTestCase errorTestCases[] = { clockCls->originIsUnixEpoch(false); return clockCls; }, - "clock class with other origin and no UUID followed by different clock class", + "clock class with unknown origin and no UUID followed by different clock class", "Unexpected clock class"}, }; -- 2.34.1