[ovs-dev] [PATCH] Add Nicira vendor extension actions NXAST_STACK_PUSH/POP

Ethan Jackson ethan at nicira.com
Thu Feb 28 01:12:16 UTC 2013


Ah I was half way through reviewing when Ben sent out his review.  I had a
couple of comments which he didn't mention so I thought I'd send this out
anyway.

 +/* Action structure for NXAST_STACK_PUSH.
> + *
> + * Pushes value[ofs:ofs+n_bits] onto the top of the stack.
> + */
> +struct nx_action_stack{
>

You need a space after nx_action_stack here.

I think we need to fill out the documentation here.  It should note that
this structure is used for both NXAST_STACK_PUSH and NXAST_STACK_POP, as
well as explain in detail how these actions work.  See NXAST_BUNDLE for an
example of the typical level of detail.


> +    ovs_be16 type;                  /* OFPAT_VENDOR. */
> +    ovs_be16 len;                   /* Length is 16. */
> +    ovs_be32 vendor;                /* NX_VENDOR_ID. */
> +    ovs_be16 subtype;               /* NXAST_REG_PUSH or NXAST_REG_POP. */
> +    ovs_be16 ofs_nbits;             /* (ofs << 6) | (n_bits - 1). */
> +    ovs_be32 reg;                   /* The register used for push or pop.
> */
>

I would call this "field", as it doesn't necessarily have to be a register.
 One could push a vlan tag directly onto the stack for example.

Also would you mind putting an additional "zero" field at the end?
Something like:

uint8_t zero[8]; /* Reserved. Must be zero. */

I expect we'll need to extend these actions at some point.  Doing this will
help us do so in a backwards compatible way.


> +
> +/* nxm_parse_stack_push(), nxm_parse_stack_pop(). */
> +void
> +nxm_parse_stack_push(struct ofpact_stack_push *push, const char *s)
> +{
> +    s = mf_parse_subfield(&push->src, s);
> +    if (*s != '\0') {
> +        ovs_fatal(0, "%s: trailing garbage following push source", s);
> +    }
> +}
> +
> +void
> +nxm_parse_stack_pop(struct ofpact_stack_pop *pop, const char *s)
> +{
> +    s = mf_parse_subfield(&pop->dst, s);
> +    if (*s != '\0') {
> +        ovs_fatal(0, "%s: trailing garbage following pop destination", s);
> +    }
> +}
> +
>

Here we'll need to enforce that the zero field suggested above is actually
zero.



> +/* nxm_execute_stack_push(), nxm_execute_stack_pop(). */
> +static void
> +nx_stack_push(struct ofpbuf* stack, union mf_value * v)
> +{
> +    /* Assume we have infinitely deep stack.  ofpbuf will dynamically
> +       allocate memory to grow the stack if necessary */
> +    ofpbuf_put(stack, v, sizeof *v);
> +}
>

It's not clear if this function is really adding anything.  Can we just
delete it?

+static union mf_value*
> +nx_stack_pop(struct ofpbuf* stack)
> +{
> +    union mf_value* v =  NULL;
> +    if (stack->size) {
> +        stack -> size -= sizeof *v;
> +        v = (union mf_value*)((char*)stack->data + stack->size);
> +    }
> +
> +    return v;
> +}
> +
>

Any reason we can't just use ofpbuf_try_pull() instead?


In the man page there is a line which is much longer than 79 characters.
 If you could wrap it that would be nice.

Thanks,
Ethan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130227/e4ddb716/attachment-0003.html>


More information about the dev mailing list