[ovs-dev] [PATCH 2/2] nicira-ext: New Nicira vendor action NXAST_NOTE.

Justin Pettit jpettit at nicira.com
Fri Nov 12 23:28:00 UTC 2010


On Nov 10, 2010, at 4:28 PM, Ben Pfaff wrote:

> +/* Action structure for NXAST_NOTE.
> + *
> + * This action has no effect.  It is variable length.  The switch does not
> + * attempt to interpret the user-defined 'note' data in any way.  A controller
> + * can use this action to attach arbitrary metadata to a flow.
> + */
> +struct nx_action_note {
> +    uint16_t type;                  /* OFPAT_VENDOR. */
> +    uint16_t len;                   /* At least 16. */
> +    uint32_t vendor;                /* NX_VENDOR_ID. */
> +    uint16_t subtype;               /* NXAST_NOTE. */
> +    uint8_t note[6];                /* Start of user-defined data. */
> +    /* Possibly followed by additional user-defined data. */
> +};

It would be good to mention that the note needs to be a multiple of OFP_ACTION_ALIGN.

I would also mention in the description the same warning as in the commit message that this may go away in the future.

> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -282,6 +282,22 @@ str_to_action(char *str, struct ofpbuf *b)
>             nah = put_action(b, sizeof *nah, OFPAT_VENDOR);
>             nah->vendor = htonl(NX_VENDOR_ID);
>             nah->subtype = htons(NXAST_POP_QUEUE);
> +        } else if (!strcasecmp(act, "note")) {
> +            struct nx_action_note *nan;
> +            size_t arg_len, length;
> +
> +            if (!arg) {
> +                arg = "";
> +            }
> +            arg_len = strlen(arg);
> +
> +            length = ROUND_UP(offsetof(struct nx_action_note, note) + arg_len,
> +                              OFP_ACTION_ALIGN);
> +
> +            nan = put_action(b, length, OFPAT_VENDOR);
> +            nan->vendor = htonl(NX_VENDOR_ID);
> +            nan->subtype = htons(NXAST_NOTE);
> +            memcpy(nan->note, arg, arg_len);

Looking through the code that translates a string into actions, I don't see anything that enforces that a packet will fit in an OpenFlow message (at least on the "ovs-ofctl add-flow" code path).  This is a problem with normal flows, but it will be exacerbated by not enforcing any sort of limits on the length of these notes.

While my secret (or not so secret) hope is that we come up with an alternate way to store this information, it may be worth documenting this capability in the ovs-ofctl man page.

> static void
> ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah)
> {
> @@ -217,6 +265,10 @@ ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah)
>         ds_put_cstr(string, "pop_queue");
>         break;
> 
> +    case NXAST_NOTE:
> +        print_note(string, (const struct nx_action_note *) nah);
> +        break;
> +

I had some concerns about consistency of printing notes with ASCII and binary data, which we discussed in person.  Based on that discussion, you were going to rework some of this patch so that it always took in hex data and output it.

> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -10,6 +10,7 @@ udp dl_vlan_pcp=7 idle_timeout=5 actions=strip_vlan output:0
> 
> +actions=note:abc,note:abcdefghi,note:
> ...
> +flow_mod: ADD: actions=note:"abc",note:"abcdefghi",note: 7f 00 00 00 00 00

This backspace wasn't printable in my mail viewer.  If you were to keep this test, it would be worth adding a comment to the action definition so that people know that you were using a special sequence.

--Justin






More information about the dev mailing list