LAB5 - swss work for lab#1
Conversation
abelamit
force-pushed
the
master_amabel_swss_lab5
branch
2 times, most recently
from
5e13a5b to
6345b1b
Compare
| fieldValue = std::to_string(TX_ERROR_CHECK_THRESHOLD_DEFAULT); | ||
| } | ||
|
|
||
| mcUpdateThreshold(std::stoul(fieldValue)); |
There was a problem hiding this comment.
@abelamit suggest replacing with sonic-swss-common/common/converter.h
| fieldValue = std::to_string(TX_ERROR_CHECK_POLL_TIMEOUT_SEC_DEFAULT); | ||
| } | ||
|
|
||
| mcUpdateTimePeriod(std::stoul(fieldValue)); |
There was a problem hiding this comment.
@abelamit suggest replacing with sonic-swss-common/common/converter.h
| { | ||
| if (op == DEL_COMMAND) | ||
| { | ||
| fieldValue = std::to_string(TX_ERROR_CHECK_THRESHOLD_DEFAULT); |
There was a problem hiding this comment.
@abelamit FYI. Usually we do no do like this, only in a very rare cases. Different vendors may want to have different defaults
There was a problem hiding this comment.
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
|
|
||
| #define TX_ERROR_PORT_STATE_FIELD "tx_error_port_state" | ||
| #define TX_ERROR_PORT_STATE_ERROR "error" | ||
| #define TX_ERROR_PORT_STATE_OK "ok" |
There was a problem hiding this comment.
@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
| @@ -0,0 +1,50 @@ | |||
| #ifndef TXERRORCHECKORCH_H | |||
| #define TXERRORCHECKORCH_H | |||
There was a problem hiding this comment.
@abelamit this is a good style but # pragma once is preferred
| class TxErrorCheckOrch: public Orch | ||
| { | ||
| public: | ||
| TxErrorCheckOrch(swss::DBConnector *db, std::string tableName); |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
Fixed. I will check out the static analysis tool
|
|
||
| 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)); | ||
| } No newline at end of file |
There was a problem hiding this comment.
@abelamit do not forget about new line at the end of each file
| continue; | ||
| } | ||
|
|
||
| uint32_t outErrorsCount = static_cast<uint32_t>(std::stoul(outErrors)); |
There was a problem hiding this comment.
@abelamit suggest replacing with sonic-swss-common/common/converter.h
|
|
||
| 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 |
There was a problem hiding this comment.
@abelamit consider using sai_serialize_object_id from sonic-sairedis/meta/sai_serialize.h
There was a problem hiding this comment.
Very valid point! I was looking for this function without success. Thanks!
Fixed
|
|
||
| TxErrorCheckOrch::TxErrorCheckOrch(swss::DBConnector *db, std::string tableName): | ||
| Orch(db, std::vector<string>{tableName}), | ||
| m_countersDb(new swss::DBConnector("COUNTERS_DB", 0)), |
There was a problem hiding this comment.
@abelamit a good practice is also to use make_shared and make_unique
abelamit
force-pushed
the
master_amabel_swss_lab5
branch
from
6345b1b to
3d575f4
Compare
2 participants
What I did
Why I did it
How I verified it
Details if related