From aa15c5af6e137cce47266f03d77efaf933b76f42 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 22 Feb 2024 21:23:58 +0200 Subject: [PATCH] [#3245] addressed review --- src/hooks/dhcp/perfmon/alarm.h | 8 +-- src/hooks/dhcp/perfmon/monitored_duration.cc | 6 +- src/hooks/dhcp/perfmon/monitored_duration.h | 56 +++++++-------- .../dhcp/perfmon/tests/alarm_unittests.cc | 30 ++++---- .../tests/monitored_duration_unittests.cc | 68 +++++++++---------- src/lib/dhcp/pkt_filter_lpf.cc | 2 +- 6 files changed, 85 insertions(+), 85 deletions(-) diff --git a/src/hooks/dhcp/perfmon/alarm.h b/src/hooks/dhcp/perfmon/alarm.h index f0080ae2ad..5797a84e21 100644 --- a/src/hooks/dhcp/perfmon/alarm.h +++ b/src/hooks/dhcp/perfmon/alarm.h @@ -30,21 +30,21 @@ public: /// /// @param family protocol family AF_INET or AF_INET6 /// @param query_type message type of the query packet - /// @param response_type_ message type of the response packet + /// @param response_type message type of the response packet /// @param start_event_label label of the start event /// @param end_event_label label of the end event /// @param SubnetID subnet_id id of the selected subnet /// @param low_water threshold below which the average duration must fall to clear the alarm /// @brief high_water threshold above which the average duration must rise to trigger the alarm. /// @brief enabled true sets state to CLEAR, otherwise DISABLED, defaults to true. - Alarm(uint16_t family, uint8_t query_type_, uint8_t response_type_, + Alarm(uint16_t family, uint8_t query_type, uint8_t response_type, const std::string& start_event_label, const std::string& end_event_label, - dhcp::SubnetID subnet_id_, + dhcp::SubnetID subnet_id, const Duration& low_water, const Duration& high_water, bool enabled = true); /// @brief Constructor /// - /// param key composite key that identifies the alarm + /// @param key composite key that identifies the alarm /// @param low_water threshold below which the average duration must fall to clear the alarm /// @brief high_water threshold above which the average duration must rise to trigger the alarm. /// @brief enabled true sets state to CLEAR, otherwise DISABLED, defaults to true. diff --git a/src/hooks/dhcp/perfmon/monitored_duration.cc b/src/hooks/dhcp/perfmon/monitored_duration.cc index abc4343872..d6ddd172fb 100644 --- a/src/hooks/dhcp/perfmon/monitored_duration.cc +++ b/src/hooks/dhcp/perfmon/monitored_duration.cc @@ -21,9 +21,9 @@ namespace perfmon { // DurationDataInterval methods DurationDataInterval::DurationDataInterval(const Timestamp& start_time /* = PktEvent::now()*/) - : start_time_(start_time), occurrences_(0), - min_duration_(pos_infin), max_duration_(neg_infin), - total_duration_(microseconds(0)) { + : start_time_(start_time), occurrences_(0), + min_duration_(pos_infin), max_duration_(neg_infin), + total_duration_(microseconds(0)) { } void diff --git a/src/hooks/dhcp/perfmon/monitored_duration.h b/src/hooks/dhcp/perfmon/monitored_duration.h index 9709c864b5..535a7882fe 100644 --- a/src/hooks/dhcp/perfmon/monitored_duration.h +++ b/src/hooks/dhcp/perfmon/monitored_duration.h @@ -63,7 +63,7 @@ public: /// @brief Get the number of occurrences that have contributed to the /// interval. /// - /// @return uint64_t containing the number of occurrences. + /// @return the number of occurrences. uint64_t getOccurrences() const { return (occurrences_); }; @@ -119,20 +119,20 @@ typedef boost::shared_ptr DurationDataIntervalPtr; /// -# Response Packet Type /// -# Start Event /// -# End Event -/// -# Subnet ID can be GLOBAL_SUBNET_ID for aggregate durations +/// -# Subnet ID can be GLOBAL_SUBNET_ID for aggregate durations class DurationKey { public: /// @brief Constructor /// /// @param family protocol family AF_INET or AF_INET6 /// @param query_type message type of the query packet - /// @param response_type_ message type of the response packet + /// @param response_type message type of the response packet /// @param start_event_label label of the start event /// @param end_event_label label of the end event - /// @param SubnetID subnet_id id of the selected subnet - DurationKey(uint16_t family, uint8_t query_type_, uint8_t response_type_, + /// @param subnet_id id of the selected subnet + DurationKey(uint16_t family, uint8_t query_type, uint8_t response_type, const std::string& start_event_label, const std::string& end_event_label, - dhcp::SubnetID subnet_id_); + dhcp::SubnetID subnet_id); /// @brief Destructor virtual ~DurationKey() = default; @@ -141,33 +141,33 @@ public: /// /// @return uint16_t containing the family (AF_INET or AF_INET6) uint16_t getFamily() { - return(family_); + return (family_); } /// @brief Get the query packet type. /// - /// @return uint8_t containing the query packet type. + /// @return the query packet type. uint8_t getQueryType() const { return (query_type_); } /// @brief Get the response packet type. /// - /// @return uint8_t containing the response packet type. + /// @return the response packet type. uint8_t getResponseType() const { return (response_type_); }; /// @brief Get the start event label. /// - /// @return String containing the start event label. + /// @return the start event label. std::string getStartEventLabel() const { return (start_event_label_); } /// @brief Get the end event label. /// - /// @return String containing the end event label. + /// @return the end event label. std::string getEndEventLabel() const { return (end_event_label_); } @@ -197,7 +197,7 @@ public: /// /// @endcode /// - /// @return String containing the composite label. + /// @return the composite label. std::string getLabel() const; /// @brief Validates that a query and response message type pair is sane. @@ -205,19 +205,19 @@ public: /// @param family Protocol family of the key (AF_INET or AF_INET6) /// The format of the string: /// @param query_type message type of the query packet - /// @param response_type_ message type of the response packet + /// @param response_type message type of the response packet /// /// @throw BadValue is the pairing does not make sense. static void validateMessagePair(uint16_t family, uint8_t query_type, uint8_t response_type); protected: - /// @brief Protocol family AF_INET or AF_INET6 + /// @brief Protocol family AF_INET or AF_INET6. uint16_t family_; - /// @brief Query message type (e.g. DHCPDISCOVER, DHCP6_SOLICIT) + /// @brief Query message type (e.g. DHCPDISCOVER, DHCP6_SOLICIT). uint8_t query_type_; - /// @brief Response message type. (e.g. DHCPOFFER, DHCP6_ADVERTISE) + /// @brief Response message type (e.g. DHCPOFFER, DHCP6_ADVERTISE). uint8_t response_type_; /// @brief Label of the start event which begins the duration. @@ -239,39 +239,39 @@ public: /// /// @param family protocol family AF_INET or AF_INET6 /// @param query_type message type of the query packet - /// @param response_type_ message type of the response packet + /// @param response_type message type of the response packet /// @param start_event_label label of the start event /// @param end_event_label label of the end event - /// @param SubnetID subnet_id id of the selected subnet - /// @param Duration interval_duration_; - MonitoredDuration(uint16_t family, uint8_t query_type_, uint8_t response_type_, + /// @param subnet_id id of the selected subnet + /// @param interval_duration the interval duration + MonitoredDuration(uint16_t family, uint8_t query_type, uint8_t response_type, const std::string& start_event_label, const std::string& end_event_label, - dhcp::SubnetID subnet_id_, const Duration& interval_duration_); + dhcp::SubnetID subnet_id, const Duration& interval_duration); /// @brief Constructor /// - /// param key composite key that identifies the alarm - /// @param Duration interval_duration_; - MonitoredDuration(const DurationKey& key, const Duration& interval_duration_); + /// @param key composite key that identifies the alarm + /// @param interval_duration the interval duration + MonitoredDuration(const DurationKey& key, const Duration& interval_duration); /// @brief Destructor virtual ~MonitoredDuration() = default; - /// @brief Get the interval duration + /// @brief Get the interval duration. /// /// @return Duration containing the interval duration. Duration getIntervalDuration() const { - return(interval_duration_); + return (interval_duration_); } - /// @brief Get the previous interval + /// @brief Get the previous interval. /// /// @return Pointer to the previous interval if it exists or an empty pointer. DurationDataIntervalPtr getPreviousInterval() const { return (previous_interval_); } - /// @brief Get the current interval + /// @brief Get the current interval. /// /// @return Pointer to the current interval if it exists or an empty pointer. DurationDataIntervalPtr getCurrentInterval() const { diff --git a/src/hooks/dhcp/perfmon/tests/alarm_unittests.cc b/src/hooks/dhcp/perfmon/tests/alarm_unittests.cc index 759b3bdc73..ffae6d1261 100644 --- a/src/hooks/dhcp/perfmon/tests/alarm_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/alarm_unittests.cc @@ -30,9 +30,9 @@ TEST(Alarm, validConstructors) { Duration low_water(milliseconds(50)); Duration high_water(milliseconds(250)); ASSERT_NO_THROW_LOG(alarm.reset(new Alarm(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, - low_water, high_water))); + "process_started", "process_completed", + SUBNET_ID_GLOBAL, + low_water, high_water))); ASSERT_TRUE(alarm); EXPECT_EQ(alarm->getFamily(), AF_INET); EXPECT_EQ(alarm->getQueryType(), DHCPDISCOVER); @@ -52,7 +52,7 @@ TEST(Alarm, validConstructors) { // Create valid v6 key and use that to create an alarm. Verify contents and label. DurationKeyPtr key; ASSERT_NO_THROW_LOG(key.reset(new DurationKey(AF_INET6, DHCPV6_SOLICIT, DHCPV6_ADVERTISE, - "mt_queued", "process_started", 77))); + "mt_queued", "process_started", 77))); ASSERT_NO_THROW_LOG(alarm.reset(new Alarm(*key, low_water, high_water, false))); ASSERT_TRUE(alarm); @@ -77,15 +77,15 @@ TEST(Alarm, invalidConstructors) { Duration low_water(milliseconds(50)); Duration high_water(milliseconds(250)); ASSERT_THROW_MSG(alarm.reset(new Alarm(AF_INET, DHCPDISCOVER, DHCPDISCOVER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, low_water, high_water)), + "process_started", "process_completed", + SUBNET_ID_GLOBAL, low_water, high_water)), BadValue, "Response type: DHCPDISCOVER not valid for query type: DHCPDISCOVER"); // Low water too high, should throw. ASSERT_THROW_MSG(alarm.reset(new Alarm(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, high_water, low_water)), + "process_started", "process_completed", + SUBNET_ID_GLOBAL, high_water, low_water)), BadValue, "low water: 00:00:00.250000, must be less than high water:" " 00:00:00.050000"); @@ -93,7 +93,7 @@ TEST(Alarm, invalidConstructors) { // Create valid v6 key. DurationKeyPtr key; ASSERT_NO_THROW_LOG(key.reset(new DurationKey(AF_INET6, DHCPV6_SOLICIT, DHCPV6_ADVERTISE, - "mt_queued", "process_started", 77))); + "mt_queued", "process_started", 77))); // Low water too high, should throw. ASSERT_THROW_MSG(alarm.reset(new Alarm(*key, high_water, low_water)), @@ -108,9 +108,9 @@ TEST(Alarm, lowWaterHighWaterSetters) { Duration high_water(milliseconds(250)); AlarmPtr alarm; ASSERT_NO_THROW_LOG(alarm.reset(new Alarm(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, - low_water, high_water))); + "process_started", "process_completed", + SUBNET_ID_GLOBAL, + low_water, high_water))); // Should be able to set thresholds to new, valid values. low_water += milliseconds(50); @@ -135,8 +135,8 @@ TEST(Alarm, clearAndDisable) { auto start_time = PktEvent::now(); AlarmPtr alarm; ASSERT_NO_THROW_LOG(alarm.reset(new Alarm(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, milliseconds(100), milliseconds(200)))); + "process_started", "process_completed", + SUBNET_ID_GLOBAL, milliseconds(100), milliseconds(200)))); // Initial state should be CLEAR, stos_time_ should be close to now, no report time. EXPECT_EQ(alarm->getState(), Alarm::CLEAR); @@ -175,7 +175,7 @@ TEST(Alarm, clearAndDisable) { // ``` // INPUT | OUTPUT // Test sample relationship Input Report Int.| -// to the thresholds State Elapsed | Report State Stos Last Report +// to the thresholds State Elapsed | Report State Stos Last Report // -------------------------------------------------|---------------------------------- // sample < low_water C false | false C - - // sample < low_water C true | false C - - diff --git a/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc b/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc index cce92f068d..c85bd3631a 100644 --- a/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc @@ -86,8 +86,8 @@ TEST(DurationKey, basics) { // Create valid v4 key, verify contents and label. ASSERT_NO_THROW_LOG(key.reset(new DurationKey(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL))); + "process_started", "process_completed", + SUBNET_ID_GLOBAL))); ASSERT_TRUE(key); EXPECT_EQ(key->getFamily(), AF_INET); EXPECT_EQ(key->getQueryType(), DHCPDISCOVER); @@ -127,28 +127,28 @@ TEST(DurationKey, validateMessagePairs4) { // List of scenarios to test, one per v4 message type. std::list scenarios { - { DHCP_NOTYPE, {DHCP_NOTYPE, DHCPOFFER, DHCPACK, DHCPNAK}}, - { DHCPDISCOVER, {DHCP_NOTYPE, DHCPOFFER, DHCPNAK}}, - { DHCPOFFER, {}}, - { DHCPREQUEST, {DHCP_NOTYPE, DHCPACK, DHCPNAK}}, - { DHCPDECLINE, {}}, - { DHCPACK, {}}, - { DHCPNAK, {}}, - { DHCPRELEASE, {}}, - { DHCPINFORM, {DHCP_NOTYPE, DHCPACK}}, -// { DHCPFORCERENEW, {}}, commented out in dhcp4.h - { DHCPLEASEQUERY, {}}, - { DHCPLEASEUNASSIGNED, {}}, - { DHCPLEASEUNKNOWN, {}}, - { DHCPLEASEACTIVE, {}}, - { DHCPBULKLEASEQUERY, {}}, - { DHCPLEASEQUERYDONE, {}}, -// { DHCPACTIVELEASEQUERY, {}}, commented out in dhcp4.h - { DHCPLEASEQUERYSTATUS, {}}, - { DHCPTLS, {}}, + {DHCP_NOTYPE, {DHCP_NOTYPE, DHCPOFFER, DHCPACK, DHCPNAK}}, + {DHCPDISCOVER, {DHCP_NOTYPE, DHCPOFFER, DHCPNAK}}, + {DHCPOFFER, {}}, + {DHCPREQUEST, {DHCP_NOTYPE, DHCPACK, DHCPNAK}}, + {DHCPDECLINE, {}}, + {DHCPACK, {}}, + {DHCPNAK, {}}, + {DHCPRELEASE, {}}, + {DHCPINFORM, {DHCP_NOTYPE, DHCPACK}}, +// {DHCPFORCERENEW, {}}, commented out in dhcp4.h + {DHCPLEASEQUERY, {}}, + {DHCPLEASEUNASSIGNED, {}}, + {DHCPLEASEUNKNOWN, {}}, + {DHCPLEASEACTIVE, {}}, + {DHCPBULKLEASEQUERY, {}}, + {DHCPLEASEQUERYDONE, {}}, +// {DHCPACTIVELEASEQUERY, {}}, commented out in dhcp4.h + {DHCPLEASEQUERYSTATUS, {}}, + {DHCPTLS, {}}, }; - // Iterate over the scenarios. Attempt to pair each scenario query type with every v6 message + // Iterate over the scenarios. Attempt to pair each scenario query type with every v4 message // type as a response type. If the response type is in the scenario's valid list, the pair // should validate, otherwise it should throw. for (auto const& scenario : scenarios) { @@ -165,7 +165,7 @@ TEST(DurationKey, validateMessagePairs4) { } } -// Verify v4 message pair validation works. +// Verify v6 message pair validation works. TEST(DurationKey, validateMessagePairs6) { // Defines a test scenario. struct Scenario { @@ -279,24 +279,24 @@ TEST(MonitoredDuration, invalidConstructors) { // Make sure we catch an invalid message pairing. Duration interval_duration = seconds(60); ASSERT_THROW_MSG(mond.reset(new MonitoredDuration(AF_INET, DHCPDISCOVER, DHCPDISCOVER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, interval_duration)), + "process_started", "process_completed", + SUBNET_ID_GLOBAL, interval_duration)), BadValue, "Response type: DHCPDISCOVER not valid for query type: DHCPDISCOVER"); - // Interval duration cannot be than zero. + // Interval duration cannot be zero. interval_duration = DurationDataInterval::ZERO_DURATION(); ASSERT_THROW_MSG(mond.reset(new MonitoredDuration(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, interval_duration)), + "process_started", "process_completed", + SUBNET_ID_GLOBAL, interval_duration)), BadValue, "MonitoredDuration - interval_duration 00:00:00," " is invalid, it must be greater than 0"); // Interval duration cannot be negative. ASSERT_THROW_MSG(mond.reset(new MonitoredDuration(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, seconds(-5))), + "process_started", "process_completed", + SUBNET_ID_GLOBAL, seconds(-5))), BadValue, "MonitoredDuration - interval_duration -00:00:05," " is invalid, it must be greater than 0"); @@ -304,9 +304,9 @@ TEST(MonitoredDuration, invalidConstructors) { // Create valid v6 key. DurationKeyPtr key; ASSERT_NO_THROW_LOG(key.reset(new DurationKey(AF_INET6, DHCPV6_SOLICIT, DHCPV6_ADVERTISE, - "mt_queued", "process_started", 77))); + "mt_queued", "process_started", 77))); - // Interval duration cannot be than zero. + // Interval duration cannot be zero. ASSERT_THROW_MSG(mond.reset(new MonitoredDuration(*key, interval_duration)), BadValue, "MonitoredDuration - interval_duration 00:00:00," @@ -326,8 +326,8 @@ TEST(MonitoredDuration, addSampleAndClear) { // Create valid v4 duration with interval duration of 50ms. ASSERT_NO_THROW_LOG(mond.reset(new MonitoredDuration(AF_INET, DHCPDISCOVER, DHCPOFFER, - "process_started", "process_completed", - SUBNET_ID_GLOBAL, interval_duration))); + "process_started", "process_completed", + SUBNET_ID_GLOBAL, interval_duration))); ASSERT_TRUE(mond); // Initially there are no intervals. diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index f0f19e946a..37ce7c2d14 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -192,7 +192,7 @@ PktFilterLPF::openSocket(Iface& iface, int enable = 1; if (setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP, &enable, sizeof(enable))) { const char* errmsg = strerror(errno); - isc_throw(SocketConfigError, "Can't enable SO_TIMESTAMP for " << addr.toText() + isc_throw(SocketConfigError, "Can not enable SO_TIMESTAMP for " << addr.toText() << ", error: " << errmsg); } #endif