[ovs-dev] [PATCH 05/10] lib/odp: Masked set action execution and printing.

[Jesse Gross]

On Wed, Apr 9, 2014 at 1:51 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>
> On Apr 9, 2014, at 1:26 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> On Wed, Apr 09, 2014 at 01:24:07PM -0700, Jarno Rajahalme wrote:
>
>
> On Apr 9, 2014, at 10:51 AM, Ben Pfaff <blp at nicira.com> wrote:
>
> On Tue, Apr 08, 2014 at 04:38:47PM -0700, Jarno Rajahalme wrote:
>
> Masked set actions add a mask, immediately following the netlink
> attribute data, within the netlink attribute itself.  Thus the key
> attribute size for a masked set action is exactly double of the
> non-masked set action.
>
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>
>
> This adds a new restriction to key attributes: they must have a fixed
> length (because otherwise one cannot tell whether the length is
> doubled).  In fact, MPLS is already variable length, as documented in
> <linux/openvswitch.h>:
>
> OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
>  * The implementation may restrict
>  * the accepted length of the array. */
>
>
> I totally missed this, maybe due to the current userspace datapath setting
> only on lee, and linux kernel datapath not yet supporting MPLS.
>
> So I'd suggest some other way to distinguish masked set actions.
>
>
> The obvious alternative is to use a second attribute of the same
> type and length within the nested attributes of the set action. It
> is currently limited to one. The additional structure will be
> somewhat repetitive, and we?ll have to enforce that the key type and
> length are the same for both. To be more explicit we could set the
> MSB of the type, but then we might also want to support mask before
> the value, etc. That may be a bit too much flexibility, though.
>
> Thoughts?
>
>
> One alternative would be to introduce OVS_ACTION_ATTR_SET_MASKED,
> would use the format you proposed in this patch except that the mask
> would always be present.
>
>
> Thanks, I like this, as this will also make feature checking more explicit.

I think that's a reasonable thing to do as well.