Add FlexCounter for MACsec SA #684
Conversation
|
retest this please |
1 similar comment
|
retest this please |
Signed-off-by: Ze Gan <ganze718@gmail.com>
Pterosaur
force-pushed
the
macsec_flex_counter
branch
from
7ca38ba
to
8f265f7
Compare
|
Hi @kcudnik , I remember your suggestion about : |
if you have empty constructor you can ether use "= default;" inside header file, or if you initialize some members in default constructor in initializer list then you can make internal as: // empty intentionally |
syncd/FlexCounter.cpp
Outdated
| FlexCounter::MACsecSAAttrIds::MACsecSAAttrIds( | ||
| _In_ sai_object_id_t macsecSA, | ||
| _In_ const std::vector<sai_macsec_sa_attr_t> &macsecSAIds): | ||
| m_macsecSAId(macsecSA), m_macsecSAAttrIds(macsecSAIds) |
There was a problem hiding this comment.
initialize each member in separate line
There was a problem hiding this comment.
Done, please review it.
syncd/FlexCounter.cpp
Outdated
|
|
||
| if (status != SAI_STATUS_SUCCESS) | ||
| { | ||
| SWSS_LOG_WARN("Failed to get attr of MACsec SA 0x%" PRIx64 ": %d", macsecSAVid, status); |
There was a problem hiding this comment.
use sai_serialize_object_id() and sai_serialize_status()
There was a problem hiding this comment.
Done, please review it.
syncd/FlexCounter.cpp
Outdated
|
|
||
| for (size_t i = 0; i != macsecSAAttrIds.size(); i++) | ||
| { | ||
| const std::string &attrName = sai_serialize_enum(macsecSAAttrIds[i], &sai_metadata_enum_sai_macsec_sa_attr_t); |
There was a problem hiding this comment.
you don't need to use serialize here, since "meta" object contains member meta->attridname which is exactly that after serialize, you can use that, then also you can drop iterator and use for auto loop
There was a problem hiding this comment.
Done, please review it.
| { | ||
| const auto &macsecSAVid = kv.first; | ||
| const auto &macsecSARid = kv.second->m_macsecSAId; | ||
| const auto &macsecSAAttrIds = kv.second->m_macsecSAAttrIds; |
There was a problem hiding this comment.
this is interesting, so actual id's are here as a numbers ? not as stats name strings ? i dont know how other counters are passed, but i was under impressions that those id's were passed from OA as actual stats strings and not as id number, but i maybe mistaken
| values.emplace_back(attrName, sai_serialize_attr_value(*meta, macsecSAAttr[i])); | ||
| } | ||
| // Write counters to DB | ||
| std::string macsecSAVidStr = sai_serialize_object_id(macsecSAVid); |
There was a problem hiding this comment.
just a note, every file uses concept of prefixing "strXXX" instead of "xxxStr", this file is exception since i didn't created this, but in other files please follow strXXX naming
syncd/FlexCounter.cpp
Outdated
| @@ -2122,6 +2224,19 @@ void FlexCounter::addCounter( | |||
|
|
|||
| setSwitchDebugCounterList(vid, rid, switchCounterIds); | |||
| } | |||
| else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_ATTR_ID_LIST) | |||
| { | |||
| std::vector<sai_macsec_sa_attr_t> MACsecSAIds; | |||
There was a problem hiding this comment.
don't prefix local variables with capital letters
There was a problem hiding this comment.
Done, please review it.
syncd/FlexCounter.cpp
Outdated
| for (const auto &str : idStrings) | ||
| { | ||
| int32_t attr; | ||
| sai_deserialize_enum(str, &sai_metadata_enum_sai_macsec_sa_attr_t, attr); |
There was a problem hiding this comment.
ok, i actually look at rest, so every other attribute have dedicated sai_deserialize_xxx_attr function for this specific purpose, so i think you can also add this dedicated function to saiserialize.cpp as you had previously, just to follow convention, sorry for this, my mistake
There was a problem hiding this comment.
Done, please review it.
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
Add FlexCounter for MACsec SA
This PR depends on schema modification: sonic-net/sonic-swss-common#403
Signed-off-by: Ze Gan ganze718@gmail.com