Thanks Ben for the review. I will send out the patch again address the issues pointed out.
On Tue, Mar 5, 2013 at 9:13 AM, Ben Pfaff <[email protected]> wrote: > On Fri, Mar 01, 2013 at 04:48:49PM -0800, Andy Zhou wrote: > > > > The Push action takes a single parameter. Any source allowed by > > NXAST_REG_MOVE is allowed to be pushed onto the stack. When the source > > is a bit field, its value will be right shifted to bit zero before > > being pushed onto the stack. The remaining bits will be set to zero. > > > > The Pop action also takes a single parameter. Any destination allowed by > > NXAST_REG_MOVE can be used as the destination of the action. The value, > in > > case of a bit field, will be taken from top of the stack, starting from > > bit zero. > > > > The stack size is not limited. The initial 8KB is statically allocated to > > efficiently handle most common use cases. When more stack space is > > required, the stack can grow using malloc(). > > > > Signed-off-by: Andy Zhou <[email protected]> > > nicira-ext.h > ------------ > > I believe that the comment here should mention NXAST_STACK_POP also: > > +/* Action structure for NXAST_STACK_PUSH. > + * > + * Pushes (or pop) field[offset: offset + n_bits] to (or from) > + * top of the stack. > + */ > +struct nx_action_stack { > > Also in struct nx_action_stack it seems a little odd to put some of the > padding in the middle. I'd be inclined to arrange it as: > > struct nx_action_stack { > 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 offset; /* Bit offset into the field. */ > ovs_be32 field; /* The field used for push or pop. > */ > ovs_be16 n_bits; /* (n_bits + 1) bits of the field. > */ > uint8_t zero[6]; /* Reserved for future use, must > be zero.*/ > }; > > or similarly, so that the padding is at the end. > > nx-match.c > ---------- > > There is a stray parenthesis inside the error message in > nxm_parse_stack_action(). > > Some functions are explicitly declared "inline" In lib/nx-match.c. We > prefer to let the compiler figure out what to inline. (We do use > "inline" in header files, but not typically in .c files.) > > " * " => " *" here: > + struct ofpact_stack * stack_action) > > You can omit the memset() calls from nxm_stack_to_nxast_common__() > because ofputil_put_*() will zero them, see the comment in ofp-util.h: > > /* For each OpenFlow action <ENUM> that has a corresponding action > structure > * struct <STRUCT>, this defines two functions: > * > * void ofputil_init_<ENUM>(struct <STRUCT> *action); > * > * Initializes the parts of 'action' that identify it as having type > <ENUM> > * and length 'sizeof *action' and zeros the rest. For actions that > have > * variable length, the length used and cleared is that of struct > <STRUCT>. > * > * struct <STRUCT> *ofputil_put_<ENUM>(struct ofpbuf *buf); > * > * Appends a new 'action', of length 'sizeof(struct <STRUCT>)', to > 'buf', > * initializes it with ofputil_init_<ENUM>(), and returns it. > */ > > I'd be inclined to write nxm_stack_push_to_nxast() and > nxm_stack_pop_to_nxast() as one-liners, e.g.: > > nxm_stack_to_nxast_common__(stack, > ofputil_put_NXAST_STACK_PUSH(openflow)); > > Stray space before ) here: > + struct flow *flow, struct ofpbuf *stack ) > > " * " => " *" here: > + char * flow_str = flow_to_string(flow); > > nx-match.h > ---------- > > Missing space before "*" here: > + const struct flow*, struct ofpbuf *); > > ofp-actions.h > ------------- > > I think this should mention the "pop" actions too: > > +/* OFPACT_STACK_PUSH. > + * > + * Used for NXAST_STACK_PUSH */ > +struct ofpact_stack { > > ofp-parse.c > ----------- > > Doesn't this parse a "pop" as a "push"? I feel like I must be wrong > about that, because some of the tests should exercise this, but I don't > see why it works out OK: > + case OFPUTIL_NXAST_STACK_PUSH: > + case OFPUTIL_NXAST_STACK_POP: > + nxm_parse_stack_action(ofpact_put_STACK_PUSH(ofpacts), arg); > + break; > > (OK, so out of curiosity, now I've run the tests. They fail.) > > ofproto-dpif.c > -------------- > > In struct action_xlate_ctx, the big comment says a 4 kB initial stack > but the little one says 1 kB. Also I think I'd prefer "1024 / > sizeof(union mf_subvalue)" over "64". > > Is there a reason to declare the 'stack' member as "struct ofpbuf *" > instead of "struct ofpbuf"? > > utilities/ovs-ofctl.8.in > ------------------------ > > It still looks like there's a stray -> here: > +Example: \fBpop:\->NXM_NX_REG2[0..5]\fR pops the value from top of the > stack. > > Thanks, > > Ben. >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
