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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
