support to check SAI capability of ACL META_DATA fields#3274
support to check SAI capability of ACL META_DATA fields#3274siqbal1986 wants to merge 10 commits intosonic-net:masterfrom
Conversation
SAI_SWITCH_ATTR_ACL_USER_META_DATA_RANGE
SAI_ACL_ENTRY_ATTR_ACTION_SET_ACL_META_DATA
SAI_ACL_ENTRY_ATTR_FIELD_ACL_USER_META
| // SWSS_LOG_WARN("Could not query ACL_USER_META_DATA_RANGE %d", status); | ||
| // } | ||
| // else | ||
| if (status == SAI_STATUS_SUCCESS) |
There was a problem hiding this comment.
The code commented out is required. Is there any other way to resolve the code coverage issue?
There was a problem hiding this comment.
I have not found a way to resolve this by other means. This is initialization code. The Vs environment does not populate these. There are VS tests in the 2nd PR which cover these values.
There was a problem hiding this comment.
If it can't be covered by UT in anyways, can you try to bypass the check as it's a test issue? It doesn't make sense to remove feature code for UT coverage.
There was a problem hiding this comment.
agree, please refer to other PRs with capability query support added
| } | ||
| // The following lines would be replaced in the next PR where these values would be used to initialze the metadataMgr. | ||
| (void)metadataMin; | ||
| (void)metadataMax; |
There was a problem hiding this comment.
Can we remove these two variables if they are not used?
There was a problem hiding this comment.
They are used in the 2nd PR. which #3307.
There was a problem hiding this comment.
Then can we add these variables in the 2nd PR?
There was a problem hiding this comment.
the 2nd PR is a child of this PR. In an effort to make the review easier, I created this PR first. the 2nd PR branch is a child of this branch. once this goes in, it would use these variables.
|
|
||
| if (platform == VS_PLATFORM_SUBSTRING) | ||
| { | ||
| // For testing on VS the following values will be used. |
There was a problem hiding this comment.
Please dont use magic numbers
|
|
||
| status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_SWITCH, SAI_SWITCH_ATTR_ACL_USER_META_DATA_RANGE, &capability); | ||
| // Disabling these blocks to stop coverage analysis failure. The subsequent PR will enable these. | ||
| // if (status != SAI_STATUS_SUCCESS) |
There was a problem hiding this comment.
No commented code per guidelines
| // SWSS_LOG_WARN("Could not query ACL_USER_META_DATA_RANGE %d", status); | ||
| // } | ||
| // else | ||
| if (status == SAI_STATUS_SUCCESS) |
There was a problem hiding this comment.
agree, please refer to other PRs with capability query support added
| attrs[0].id = SAI_SWITCH_ATTR_ACL_USER_META_DATA_RANGE; | ||
| sai_status_t status = sai_switch_api->get_switch_attribute(gSwitchId, 2, attrs); | ||
| // Disabling these blocks to stop coverage analysis failure. The subsequent PR will enable these. | ||
| // if (status != SAI_STATUS_SUCCESS) |
There was a problem hiding this comment.
Same here. no commented code. I think this PR is still in draft state
| SWSS_LOG_NOTICE("ACL_ENTRY_ATTR_ACTION_SET_ACL_META_DATA capability %d", capability.set_implemented); | ||
| } | ||
| // The following lines would be replaced in the next PR where these values would be used to initialze the metadataMgr. | ||
| (void)metadataMin; |
There was a problem hiding this comment.
Remove unused params. This can also fail the coverage.
|
closing this PR. Please review #3307 as the changes are merged there. |
3 participants
What I did
Added support to check if the platform supports the following SAI attributes.
This changes also gets the platform supported range for matadata value.
SAI_SWITCH_ATTR_ACL_USER_META_DATA_RANGE
SAI_ACL_ENTRY_ATTR_ACTION_SET_ACL_META_DATA
SAI_ACL_ENTRY_ATTR_FIELD_ACL_USER_META
Why I did it
For ACL outer DSCP change feature, these attributes are required.
How I verified it
Basic verification done in VS enviornment
Aug 23 23:13:37.191745 0066f13dd278 WARNING #orchagent: :- init: Could not get range for ACL_USER_META_DATA_RANGE -15
Aug 23 23:13:37.191756 0066f13dd278 NOTICE #orchagent: :- init: ACL_USER_META_DATA_RANGE capability 1
Aug 23 23:13:37.192269 0066f13dd278 NOTICE #orchagent: :- init: ACL_ENTRY_ATTR_FIELD_ACL_USER_META capability 1
Aug 23 23:13:37.192705 0066f13dd278 NOTICE #orchagent: :- init: ACL_ENTRY_ATTR_ACTION_SET_ACL_META_DATA capability 1
Mlnx 2700 T1
Details if related