On Thu, May 03, 2012 at 06:28:28PM -0700, Ethan Jackson wrote:
> Thanks for this series, I never got the email so I reviewed it off the
> mailing list.
>
> >> http://openvswitch.org/pipermail/dev/2012-April/016877.html
>
> Looks good.
>
> >> http://openvswitch.org/pipermail/dev/2012-April/016878.html
>
> Looks good.
>
> >> http://openvswitch.org/pipermail/dev/2012-April/016879.html
>
> "Returns true if 'mf' has a nonzero value in 'flow'." Should say "a
> zero value" instead.
Thanks, fixed.
> This patch adds a redundant newline at the end of ofp-parse.c
Thanks, removed.
> The comment of parse_ofp_exact_flow() does some slightly strange
> capitalization. I would expect S and FLOW to be written as 's' and
> 'flow'.
Oops, somehow I started using GNU style here. The GNU coding
standards say:
The comment on a function is much clearer if you use the
argument names to speak about the argument values. The
variable name itself should be lower case, but write it in
upper case when you are speaking about the value rather than
the variable itself. Thus, “the inode number NODE_NUM” rather
than “an inode”.
I'm not actually a fan of that style but I've written a lot of GNU
code.
Fixed.
> Out of curiosity, do you think it makes more sense from a memory
> management perspective for parse_ofp_exact_flow() to take a dynamic
> string as an argument in which to store error strings? Then it would
> be clear to callers that they need to deallocate it when done.
I agree that that is also a reasonable choice. I don't see enough of
an advantage to change it though.
> This patch checks if a field is set multiple times by noting that the
> field in the struct flow is non-zero. Is that sufficient? What if a
> user sets a register to zero before setting it so some other value,
> we'd end up overwriting? Is that alright?
It's not sufficient for a foolproof check, as you note, but my goal
here was only to catch the most likely user errors. I think that it
will do that.
> Looks good
Thanks.
> >> http://openvswitch.org/pipermail/dev/2012-April/016880.html
>
> Checking for an opening parenthesis seems like a fragile way of
> distinguishing datapath and openflow syntax, but I can't really think
> of a better way beyond trying to parse it using datapath syntax, and
> falling back on failure. Not sure if it matters enough to bother.
Fair enough. I did think about this issue, but I didn't document any
of it. So I've now added a comment:
/* Allow 'flow_s' to be either a datapath flow or an OpenFlow-like
* flow. We guess which type it is based on whether 'flow_s' contains
* an '(', since a datapath flow always contains '(') but an
* OpenFlow-like flow should not (in fact it's allowed but I believe
* that's not documented anywhere).
*
* An alternative would be to try to parse 'flow_s' both ways, but then
* it would be tricky giving a sensible error message. After all, do
* you just say "syntax error" or do you present both error messages?
* Both choices seem lousy. */
Do you have further comments before I push this?
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
