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
