Support for ACL extensions in metadata #1178
Conversation
Signed-off-by: Bandaru Viswanath <bandaru.viswanath@broadcom.com>
bandaru-viswanath
force-pushed
the
acl-extensions
branch
from
bc4b976
to
9ee3c92
Compare
| if (md->objecttype != SAI_OBJECT_TYPE_ACL_ENTRY || | ||
| md->attrid < SAI_ACL_ENTRY_ATTR_ACTION_START || | ||
| md->attrid > SAI_ACL_ENTRY_ATTR_ACTION_END) | ||
| if (md->objecttype != SAI_OBJECT_TYPE_ACL_ENTRY || |
There was a problem hiding this comment.
Those kind od changes needs to be introduced with changes that demonstrates need for relaxing this conditions
There was a problem hiding this comment.
@kcudnik I am trying to basically use the ACL changes from Jai's PR #1119 as an extension since that PR is not approved as yet. I am copy pasting the relevant extension enum here.
typedef enum _sai_acl_entry_attr_extensions_t
{
SAI_ACL_ENTRY_ATTR_EXTENSIONS_RANGE_START = SAI_ACL_ENTRY_ATTR_END,
/**
* @brief ACL bind point for TAM object
*
* Bind (or unbind) a TAM object.
*
* @type sai_acl_action_data_t sai_object_id_t
* @flags CREATE_AND_SET
* @objects SAI_OBJECT_TYPE_TAM
* @allownull true
* @default disabled
*/
SAI_ACL_ENTRY_ATTR_ACTION_TAM_OBJECT,
SAI_ACL_ENTRY_ATTR_EXTENSIONS_RANGE_END
} sai_acl_entry_attr_extensions_t;
As you can see the sanity checker doesn't allow any extensions for ACL module since it explicitly checks for the value to be between ACTION_START and ACTION_END. So, it prevents any extension since the extensions range starts after ACTION_END.
Let me know if this helps.
There was a problem hiding this comment.
my question here is why this change is separate from adding that extension acl entry ?
There was a problem hiding this comment.
Basically, this specific extension is a short term measure since Jai's PR is not merged in. The proposed change (to metachecker) on the other hand is intended to be generic.
There was a problem hiding this comment.
the problem im having (as in second added comment) is that not all acl entry extension attributes will be action/field, it could be regular acl_entry attribute like SAI_ACL_ENTRY_ATTR_ADMIN_STATE
lucky enough currently there are only 2 non field/action attributes and possibly extension attributes will be in that range
|
|
||
| if (metadata->isextensionattr == true) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
problem with this is that, this function checkes whether attr id is acl field or acl action, but if attribute is some other attribute and is extension, then this function will also return true :( and i think there is no good way to annotate that
There was a problem hiding this comment.
also change "if (metadata->isextensionattr == true)" to "if (metadata->isextensionattr)"
meta/saisanitycheck.c
Outdated
| md->attrid < SAI_ACL_ENTRY_ATTR_ACTION_START || | ||
| md->attrid > SAI_ACL_ENTRY_ATTR_ACTION_END) | ||
| if (md->objecttype != SAI_OBJECT_TYPE_ACL_ENTRY || | ||
| ((md->isextensionattr == false) && |
There was a problem hiding this comment.
can you bring back previous condition and add this one before it:
if (md->objecttype == SAI_OBJECT_TYPE_ACL_ENTRY && metadata->isextensionattr)
{
/* allow acl action data on excetnsion acl etnry attributes */
break;
}
i think this will make code more readable
Signed-off-by: Bandaru Viswanath <bandaru.viswanath@broadcom.com>
bandaru-viswanath
force-pushed
the
acl-extensions
branch
from
198f2eb
to
d1cbc3e
Compare
meta/saisanitycheck.c
Outdated
| @@ -1856,8 +1861,13 @@ void check_attr_acl_fields( | |||
| case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_OBJECT_ID: | |||
| case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_OBJECT_LIST: | |||
|
|
|||
| if (md->objecttype != SAI_OBJECT_TYPE_ACL_ENTRY || | |||
| md->attrid < SAI_ACL_ENTRY_ATTR_ACTION_START || | |||
| if (md->objecttype == SAI_OBJECT_TYPE_ACL_ENTRY && md->isextensionattr) | |||
There was a problem hiding this comment.
Done! @kcudnik could you please check now ?
bandaru-viswanath
force-pushed
the
acl-extensions
branch
from
e0bb0d4
to
08e0410
Compare
Signed-off-by: Bandaru Viswanath <bandaru.viswanath@broadcom.com>
bandaru-viswanath
force-pushed
the
acl-extensions
branch
from
08e0410
to
b76d3b8
Compare
|
@kcudnik Can this be merged in ? |
SAI sanitychecker doesn't expect any extensions to be developed for ACL attributes. This PR aims to add support by adding checks whether the 'under-check-attribute' is an extension attribute. The value range for an attribute between SAI_ACL_ENTRY_ATTR_ACTION_START and SAI_ACL_ENTRY_ATTR_ACTION_END is valid only for non-extensions. The extensions are to have attributes beyond SAI_ACL_ENTRY_ATTR_ACTION_END. This PR allows the range check to be conditional for extension attributes. Signed-off-by: Kumaresh Perumal <kperumal@barefootnetworks.com>
SAI sanitychecker doesn't expect any extensions to be developed for ACL attributes. This PR aims to add support by adding checks whether the 'under-check-attribute' is an extension attribute. The value range for an attribute between SAI_ACL_ENTRY_ATTR_ACTION_START and SAI_ACL_ENTRY_ATTR_ACTION_END is valid only for non-extensions. The extensions are to have attributes beyond SAI_ACL_ENTRY_ATTR_ACTION_END. This PR allows the range check to be conditional for extension attributes.
If there is a better way to enhance the metachecker, this PR can be changed accordingly.
Signed-off-by: bandaru.viswanath@broadcom.com