[vslib]: Add MACsec state to state base #722
Conversation
Signed-off-by: Ze Gan <ganze718@gmail.com>
vslib/inc/SwitchStateBase.h
Outdated
| @@ -249,6 +250,12 @@ namespace saivs | |||
| _In_ const std::string &serializedObjectId, | |||
| _In_ const sai_attribute_t* attr); | |||
|
|
|||
| virtual sai_status_t get_internal( | |||
There was a problem hiding this comment.
please remove, no need for get internal function
vslib/src/SwitchStateBase.cpp
Outdated
| if (objectType == SAI_OBJECT_TYPE_MACSEC_SA) | ||
| { | ||
| return getMACsecSAAttr( | ||
| serializedObjectId, | ||
| attr_count, | ||
| attr_list); | ||
| } | ||
|
|
||
| if (objectType == SAI_OBJECT_TYPE_MACSEC) | ||
| { | ||
| return getMACsecAttr( | ||
| serializedObjectId, | ||
| attr_count, | ||
| attr_list); | ||
| } | ||
|
|
||
| return get_internal(objectType, serializedObjectId, attr_count, attr_list); | ||
| } |
There was a problem hiding this comment.
please remove get_inernal function, there are no special action needed for GET, those attributes SAI_MACSEC_ATTR_SCI_IN_INGRESS_MACSEC_ACL you are referring, are READ_ONLY and should be refressed in "status = refresh_read_only(meta, oid);" function internals
There was a problem hiding this comment.
SAI_MACSEC_SA_ATTR_XPN should be set during CREATE, since it's create and set attribute, so get will work fine for this attribute, same for SAI_MACSEC_SA_ATTR_MINIMUM_XPN, those attributes have default values, but they are not set, when object is created, probably you should set them on custom create function
There was a problem hiding this comment.
those 2 functions getMACsecSAAttr getMACsecAttr can be removed
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
|
This pull request introduces 1 alert when merging 26acb46 into 6dfad66 - view on LGTM.com new alerts:
|
|
can you check LGTM warnings ? |
Signed-off-by: Ze Gan <ganze718@gmail.com>
|
This pull request introduces 1 alert when merging ea49bd9 into 6dfad66 - view on LGTM.com new alerts:
|
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
Pterosaur
force-pushed
the
vslib_macsec
branch
from
ee12cfe to
582a5ac
Compare
|
This pull request introduces 1 alert when merging 582a5ac into d814d2c - view on LGTM.com new alerts:
|
| @@ -818,7 +818,7 @@ size_t MACsecManager::get_macsec_sa_count( | |||
|
|
|||
| size_t sa_count = 0; | |||
|
|
|||
| for (macsec_an_t an = 0; an <= MAX_MACSEC_SA_NUMBER; an++) | |||
| for (macsec_an_t an = 0; an <= MAX_MACSEC_SA_NUMBER; an++) // lgtm [cpp/constant-comparison] | |||
There was a problem hiding this comment.
can't figure out why this is alert ?
There was a problem hiding this comment.
Yes, I cannot understand why it's an alert. Maybe it's a false positive, I guess.
vslib/src/SwitchStateBaseMACsec.cpp
Outdated
| sai_status_t SwitchStateBase::getMACsecSAAttr( | ||
| _In_ const std::string &serializedObjectId, | ||
| _In_ uint32_t attrCount, | ||
| _Out_ sai_attribute_t *attrList) | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| CHECK_STATUS(get_internal(SAI_OBJECT_TYPE_MACSEC_SA, serializedObjectId, attrCount, attrList)); | ||
| CHECK_STATUS(get(SAI_OBJECT_TYPE_MACSEC_SA, serializedObjectId, attrCount, attrList)); |
There was a problem hiding this comment.
this and below loop seems wrong to me, get() api will already populate all attributeList, and iy seems like you in 765 doing again some population? if those attributes must be refreshed, then they need to be done in refresh functions,
basically there should be no custom GET api implementations
There was a problem hiding this comment.
Actually, this function will never be used currently. We haven't had a finally conclusion about how to implement fetching the packet number of MACsec SA. Guohan's suggestion is we can skip this feature temporarily.
But I will remove this function if it's not acceptable to you.
There was a problem hiding this comment.
i think you can leave it if you want, ideally we would not like to have dead code in source, but if you gonna leave it, add TODO comment, with statement that i made, that get should have not other custom attribute changes
There was a problem hiding this comment.
I removed it and backup this version in my private repo. Please check it.
Signed-off-by: Ze Gan <ganze718@gmail.com>
|
This pull request introduces 1 alert when merging 74e67c8 into d814d2c - view on LGTM.com new alerts:
|
|
what is this new alerts? |
I didn't find any alert in the LGTM link. I'm not sure whether it's a false report of LGTM. |
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan ganze718@gmail.com