Fix review comments

Signed-off-by: Stephen Sun <[email protected]>
sonic-net · Oct 21, 2024 · 0f24502 · 0f24502
1 parent c2b1269
commit 0f24502
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 30 deletions.
diff --git a/syncd/CommandLineOptions.cpp b/syncd/CommandLineOptions.cpp
@@ -66,6 +66,7 @@ std::string CommandLineOptions::getCommandLineString() const
     ss << " ContextConfig=" << m_contextConfig;
     ss << " BreakConfig=" << m_breakConfig;
     ss << " WatchdogWarnTimeSpan=" << m_watchdogWarnTimeSpan;
+    ss << " SupportingBulkCounters=" << m_supportingBulkCounterGroups;
 
 #ifdef SAITHRIFT
 
@@ -74,8 +75,6 @@ std::string CommandLineOptions::getCommandLineString() const
 
 #endif // SAITHRIFT
 
-    ss << " supportingBulkCounters" << m_supportingBulkCounterGroups;
-
     return ss.str();
 }
 

diff --git a/syncd/CommandLineOptionsParser.cpp b/syncd/CommandLineOptionsParser.cpp
@@ -42,11 +42,11 @@ std::shared_ptr<CommandLineOptions> CommandLineOptionsParser::parseCommandLine(
             { "contextContig",           required_argument, 0, 'x' },
             { "breakConfig",             required_argument, 0, 'b' },
             { "watchdogWarnTimeSpan",    optional_argument, 0, 'w' },
+            { "SupportingBulkCounters",  required_argument, 0, 'B' },
 #ifdef SAITHRIFT
             { "rpcserver",               no_argument,       0, 'r' },
             { "portmap",                 required_argument, 0, 'm' },
 #endif // SAITHRIFT
-            { "supportingBulkCounters",  required_argument, 0, 'B' },
             { "help",                    no_argument,       0, 'h' },
             { 0,                         0,                 0,  0  }
         };
@@ -161,7 +161,7 @@ void CommandLineOptionsParser::printUsage()
     SWSS_LOG_ENTER();
 
 #ifdef SAITHRIFT
-    std::cout << "Usage: syncd [-d] [-p profile] [-t type] [-u] [-S] [-U] [-C] [-s] [-z mode] [-l] [-g idx] [-x contextConfig] [-b breakConfig] [-r] [-m portmap] [-B supportingBulkCounters] [-h]" << std::endl;
+    std::cout << "Usage: syncd [-d] [-p profile] [-t type] [-u] [-S] [-U] [-C] [-s] [-z mode] [-l] [-g idx] [-x contextConfig] [-b breakConfig] [-B supportingBulkCounters] [-r] [-m portmap] [-h]" << std::endl;
 #else
     std::cout << "Usage: syncd [-d] [-p profile] [-t type] [-u] [-S] [-U] [-C] [-s] [-z mode] [-l] [-g idx] [-x contextConfig] [-b breakConfig] [-B supportingBulkCounters] [-h]" << std::endl;
 #endif // SAITHRIFT
@@ -194,6 +194,8 @@ void CommandLineOptionsParser::printUsage()
     std::cout << "        Comparison logic 'break before make' configuration file" << std::endl;
     std::cout << "    -w --watchdogWarnTimeSpan" << std::endl;
     std::cout << "        Watchdog time span (in microseconds) to watch for execution" << std::endl;
+    std::cout << "    -B --supportingBulkCounters" << std::endl;
+    std::cout << "        Counter groups those support bulk polling" << std::endl;
 
 #ifdef SAITHRIFT
 
@@ -204,9 +206,6 @@ void CommandLineOptionsParser::printUsage()
 
 #endif // SAITHRIFT
 
-    std::cout << "    -B --supportingBulkCounters" << std::endl;
-    std::cout << "        Counter groups that support bulk polling" << std::endl;
-
     std::cout << "    -h --help" << std::endl;
     std::cout << "        Print out this message" << std::endl;
 }
diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp
@@ -30,6 +30,14 @@ static const std::string ATTR_TYPE_QUEUE = "Queue Attribute";
 static const std::string ATTR_TYPE_PG = "Priority Group Attribute";
 static const std::string ATTR_TYPE_MACSEC_SA = "MACSEC SA Attribute";
 static const std::string ATTR_TYPE_ACL_COUNTER = "ACL Counter Attribute";
+const std::map<std::string, std::string> FlexCounter::m_plugIn2CounterType = {
+    {QUEUE_PLUGIN_FIELD, COUNTER_TYPE_QUEUE},
+    {PG_PLUGIN_FIELD, COUNTER_TYPE_PG},
+    {PORT_PLUGIN_FIELD, COUNTER_TYPE_PORT},
+    {RIF_PLUGIN_FIELD, COUNTER_TYPE_RIF},
+    {BUFFER_POOL_PLUGIN_FIELD, COUNTER_TYPE_BUFFER_POOL},
+    {TUNNEL_PLUGIN_FIELD, COUNTER_TYPE_TUNNEL},
+    {FLOW_COUNTER_PLUGIN_FIELD, COUNTER_TYPE_FLOW}};
 
 BaseCounterContext::BaseCounterContext(const std::string &name):
 m_name(name)
@@ -56,7 +64,8 @@ void BaseCounterContext::addPlugins(
     }
 }
 
-void BaseCounterContext::setNoDoubleCheckBulkCapability(bool noDoubleCheckBulkCapability)
+void BaseCounterContext::setNoDoubleCheckBulkCapability(
+    _In_ bool noDoubleCheckBulkCapability)
 {
     SWSS_LOG_ENTER();
     no_double_check_bulk_capability = noDoubleCheckBulkCapability;
@@ -1145,23 +1154,16 @@ void FlexCounter::addCounterPlugin(
         }
         else
         {
-            std::map<std::string, std::string> plugIn2CounterType = {
-                {QUEUE_PLUGIN_FIELD, COUNTER_TYPE_QUEUE},
-                {PG_PLUGIN_FIELD, COUNTER_TYPE_PG},
-                {PORT_PLUGIN_FIELD, COUNTER_TYPE_PORT},
-                {RIF_PLUGIN_FIELD, COUNTER_TYPE_RIF},
-                {BUFFER_POOL_PLUGIN_FIELD, COUNTER_TYPE_BUFFER_POOL},
-                {TUNNEL_PLUGIN_FIELD, COUNTER_TYPE_TUNNEL},
-                {FLOW_COUNTER_PLUGIN_FIELD, COUNTER_TYPE_FLOW}};
-
-            auto counterTypeRef = plugIn2CounterType.find(field);
-            if (counterTypeRef != plugIn2CounterType.end())
+            auto counterTypeRef = m_plugIn2CounterType.find(field);
+
+            if (counterTypeRef != m_plugIn2CounterType.end())
             {
                 getCounterContext(counterTypeRef->second)->addPlugins(shaStrings);
 
                 if (m_noDoubleCheckBulkCapability)
                 {
                     getCounterContext(counterTypeRef->second)->setNoDoubleCheckBulkCapability(true);
+
                     SWSS_LOG_NOTICE("Do not double check bulk capability counter context %s %s", m_instanceId.c_str(), counterTypeRef->second.c_str());
                 }
             }

diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h
@@ -24,7 +24,8 @@ namespace syncd
         void addPlugins(
             _In_ const std::vector<std::string>& shaStrings);
 
-        void setNoDoubleCheckBulkCapability(bool);
+        void setNoDoubleCheckBulkCapability(
+            _In_ bool);
 
         bool hasPlugin() const {return !m_plugins.empty();}
 
@@ -46,8 +47,6 @@ namespace syncd
                 _In_ swss::DBConnector& counters_db,
                 _In_ const std::vector<std::string>& argv) = 0;
 
-
-
         virtual bool hasObject() const = 0;
 
     protected:
@@ -71,7 +70,7 @@ namespace syncd
                     _In_ const std::string& instanceId,
                     _In_ std::shared_ptr<sairedis::SaiInterface> vendorSai,
                     _In_ const std::string& dbCounters,
-                    _In_ const bool noDoubleCheckBulkCapability);
+                    _In_ const bool noDoubleCheckBulkCapability=false);
 
             virtual ~FlexCounter();
 
@@ -176,5 +175,7 @@ namespace syncd
             std::map<std::string, std::shared_ptr<BaseCounterContext>> m_counterContext;
 
             bool m_noDoubleCheckBulkCapability;
+
+            static const std::map<std::string, std::string> m_plugIn2CounterType;
     };
 }
diff --git a/unittest/syncd/TestCommandLineOptions.cpp b/unittest/syncd/TestCommandLineOptions.cpp
@@ -7,7 +7,7 @@
 using namespace syncd;
 
 const std::string expected_usage =
-R"(Usage: syncd [-d] [-p profile] [-t type] [-u] [-S] [-U] [-C] [-s] [-z mode] [-l] [-g idx] [-x contextConfig] [-b breakConfig] [-h]
+R"(Usage: syncd [-d] [-p profile] [-t type] [-u] [-S] [-U] [-C] [-s] [-z mode] [-l] [-g idx] [-x contextConfig] [-b breakConfig] [-B supportingBulkCounters] [-h]
     -d --diag
         Enable diagnostic shell
     -p --profile profile
@@ -36,6 +36,8 @@ R"(Usage: syncd [-d] [-p profile] [-t type] [-u] [-S] [-U] [-C] [-s] [-z mode] [
         Comparison logic 'break before make' configuration file
     -w --watchdogWarnTimeSpan
         Watchdog time span (in microseconds) to watch for execution
+    -B --supportingBulkCounters
+        Counter groups those support bulk polling
     -h --help
         Print out this message
 )";
@@ -49,7 +51,7 @@ TEST(CommandLineOptions, getCommandLineString)
     EXPECT_EQ(str, " EnableDiagShell=NO EnableTempView=NO DisableExitSleep=NO EnableUnittests=NO"
             " EnableConsistencyCheck=NO EnableSyncMode=NO RedisCommunicationMode=redis_async"
             " EnableSaiBulkSuport=NO StartType=cold ProfileMapFile= GlobalContext=0 ContextConfig= BreakConfig="
-            " WatchdogWarnTimeSpan=30000000");
+            " WatchdogWarnTimeSpan=30000000 SupportingBulkCounters=");
 }
 
 TEST(CommandLineOptions, startTypeStringToStartType)

diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp
@@ -51,7 +51,7 @@ void testAddRemoveCounter(
 {
     SWSS_LOG_ENTER();
 
-    FlexCounter fc("test", sai, "COUNTERS_DB", false);
+    FlexCounter fc("test", sai, "COUNTERS_DB");
 
     test_syncd::mockVidManagerObjectTypeQuery(object_type);
 
@@ -392,7 +392,7 @@ TEST(FlexCounter, noSupportedCounters)
         return SAI_STATUS_FAILURE;
     };
 
-    FlexCounter fc("test", sai, "COUNTERS_DB", false);
+    FlexCounter fc("test", sai, "COUNTERS_DB");
     std::vector<swss::FieldValueTuple> values;
     values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_UCAST_PKTS");
 
@@ -407,7 +407,7 @@ void testAddRemovePlugin(const std::string& pluginFieldName)
 {
     SWSS_LOG_ENTER();
 
-    FlexCounter fc("test", sai, "COUNTERS_DB", false);
+    FlexCounter fc("test", sai, "COUNTERS_DB");
 
     std::vector<swss::FieldValueTuple> values;
     values.emplace_back(pluginFieldName, "dummy_sha_strings");
@@ -435,7 +435,7 @@ TEST(FlexCounter, addRemoveCounterPlugin)
 
 TEST(FlexCounter, addRemoveCounterForPort)
 {
-    FlexCounter fc("test", sai, "COUNTERS_DB", false);
+    FlexCounter fc("test", sai, "COUNTERS_DB");
 
     sai_object_id_t counterVid{0x1000000000000};
     sai_object_id_t counterRid{0x1000000000000};
@@ -741,7 +741,7 @@ TEST(FlexCounter, counterIdChange)
         }
     };
 
-    FlexCounter fc("test", sai, "COUNTERS_DB", false);
+    FlexCounter fc("test", sai, "COUNTERS_DB");
 
     test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT);