On Apr 9, 2014, at 10:51 AM, Ben Pfaff <b...@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 <jrajaha...@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?
> In general the code looks good but it has more repetition than I
> usually like. Is there a good way to avoid some of that? Maybe some
> pattern like this (not tested):
>
Will consider this for the revision.
Jarno
> void
> memcpy_masked(void *dst_, const void *src_, const void *mask_, size_t size)
> {
> uint8_t *dst = dst_;
> const uint8_t *src = src_;
> const uint8_t *mask = mask_;
> size_t i;
>
> if (mask) {
> for (i = 0; i < size; i++) {
> dst[i] = src[i] | (dst[i] & ~mask[i]);
> }
> } else {
> memcpy(dst, src, size);
> }
> }
>
> #define copy_masked(dst, src, mask) \
> ((void) sizeof((dst) == (src)), \
> (void) sizeof((dst) == (mask)), \
> memcpy_masked(dst, src, mask))
> #define get_mask(a, type) \
> (nl_attr_get_size(a) >= 2 * sizeof(type) \
> ? (const type *) nl_attr_get(a) + 1 \
> : NULL)
>
> ...
> switch (type) {
> case OVS_KEY_ATTR_PRIORITY:
> copy_masked(&md->skb_priority,
> nl_attr_get_u32(a), get_mask(a, uint32_t));
> break;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
