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. This patch adds a redundant newline at the end of ofp-parse.c 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'. 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. 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? Looks good >> 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. Looks good Ethan _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
