LAB5 - swss work for lab by abelamit · Pull Request #1 · abelamit/sonic-swss

abelamit · 2025-07-30T10:41:48Z

What I did

Why I did it

How I verified it

Details if related

nazariig · 2025-08-10T15:51:49Z

orchagent/txerrorcheckorch.cpp

+                fieldValue = std::to_string(TX_ERROR_CHECK_THRESHOLD_DEFAULT);
+            }
+
+            mcUpdateThreshold(std::stoul(fieldValue));


@abelamit suggest replacing with sonic-swss-common/common/converter.h

nazariig · 2025-08-10T15:52:12Z

orchagent/txerrorcheckorch.cpp

+                fieldValue = std::to_string(TX_ERROR_CHECK_POLL_TIMEOUT_SEC_DEFAULT);
+            }
+
+            mcUpdateTimePeriod(std::stoul(fieldValue));


@abelamit suggest replacing with sonic-swss-common/common/converter.h

nazariig · 2025-08-10T15:53:38Z

orchagent/txerrorcheckorch.cpp

+        {
+            if (op == DEL_COMMAND)
+            {
+                fieldValue = std::to_string(TX_ERROR_CHECK_THRESHOLD_DEFAULT);


@abelamit FYI. Usually we do no do like this, only in a very rare cases. Different vendors may want to have different defaults

I understand. So how will you define default values? Since we need to upload our system with some default before user is configuring. Basically it is not mandatory to support this in DB_CONFIG

nazariig · 2025-08-10T15:58:35Z

orchagent/txerrorcheckorch.h

+
+#define TX_ERROR_PORT_STATE_FIELD "tx_error_port_state"
+#define TX_ERROR_PORT_STATE_ERROR "error"
+#define TX_ERROR_PORT_STATE_OK "ok"


@abelamit it is better to avoid having define in the main header (.h) which you include everywhere. A dedicated stuff which is used mostly under the hood better to move to the compilation unit (.cpp). This will result in a scope limitation and better encapsulation

nazariig · 2025-08-10T16:04:06Z

orchagent/txerrorcheckorch.h

@@ -0,0 +1,50 @@
+#ifndef TXERRORCHECKORCH_H
+#define TXERRORCHECKORCH_H


@abelamit this is a good style but # pragma once is preferred

nazariig · 2025-08-10T16:15:12Z

orchagent/txerrorcheckorch.h

+class TxErrorCheckOrch: public Orch
+{
+public:
+    TxErrorCheckOrch(swss::DBConnector *db, std::string tableName);


@abelamit having const std::string &tableName would be better. Also, suggest trying some tool for a static code analysis. That might be helpful to learn a new stuff

Fixed. I will check out the static analysis tool

nazariig · 2025-08-10T16:16:00Z

orchagent/txerrorcheckorch.cpp

+
+    swss::Table portStateTable(m_stateDb.get(), STATE_PORT_TABLE_NAME);
+    portStateTable.hset(port_name, TX_ERROR_PORT_STATE_FIELD, (isTxErrorState ? TX_ERROR_PORT_STATE_ERROR : TX_ERROR_PORT_STATE_OK));
+}


@abelamit do not forget about new line at the end of each file

nazariig · 2025-08-10T16:22:38Z

orchagent/txerrorcheckorch.cpp

+            continue;
+        }
+
+        uint32_t outErrorsCount = static_cast<uint32_t>(std::stoul(outErrors));


@abelamit suggest replacing with sonic-swss-common/common/converter.h

nazariig · 2025-08-10T16:26:29Z

orchagent/txerrorcheckorch.cpp

+
+        std::stringstream ss;
+        ss << "oid:0x" << std::hex << portOid; // Set output format to hexadecimal and insert the value
+        std::string portOidStr = ss.str(); // Extract the string


@abelamit consider using sai_serialize_object_id from sonic-sairedis/meta/sai_serialize.h

Very valid point! I was looking for this function without success. Thanks!
Fixed

nazariig · 2025-08-10T16:28:17Z

orchagent/txerrorcheckorch.cpp

+
+TxErrorCheckOrch::TxErrorCheckOrch(swss::DBConnector *db, std::string tableName):
+    Orch(db, std::vector<string>{tableName}),
+    m_countersDb(new swss::DBConnector("COUNTERS_DB", 0)),


@abelamit a good practice is also to use make_shared and make_unique

abelamit force-pushed the master_amabel_swss_lab5 branch 2 times, most recently from 5e13a5b to 6345b1b Compare July 30, 2025 12:01

nazariig suggested changes Aug 10, 2025

View reviewed changes

Resolved conflict

3d575f4

abelamit force-pushed the master_amabel_swss_lab5 branch from 6345b1b to 3d575f4 Compare August 11, 2025 07:15

		@@ -0,0 +1,50 @@
		#ifndef TXERRORCHECKORCH_H
		#define TXERRORCHECKORCH_H

Conversation

abelamit commented Jul 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants