On Wed, Aug 17, 2011 at 12:33:55AM +0200, Philippe Jung wrote: > Please find a new version of the patch against master branch, non word > wrapped, with updated doc. Also fixed a typo in INSTALL.Linux still > referencing --with-l26
Thanks. This version applies well. I have some comments to add to Jesse's. Please observe the existing coding style, which we've tried to describe in the CodingStyle file. The most striking way that your code differs is in comments. We don't usually use // comments (as your patch does), and we don't format /* */ comments the same way that your patch does. Please add a little more white space, in particular please use a blank line to separate a comment above a block of code from the block of code that precedes a comment. For example, I would add a blank line before the comment "// Untag only if untagged-pvid mode and vlan=tag" in ofproto-dpif.c and before the comment "/* This port is an access port. All packets are tagged with "vlan" */" in ofproto.h. Please add an item to NEWS and add your name to AUTHORS. The comparison "out_bundle->vlan_mode < PORT_VLAN_TRUNK" in set_dst() worries me a bit. We don't usually use inequalities on enums in this way, because it causes surprises when the enum values are reordered or when new enum values are added. I think that a "switch" statement might be a good way to write that function's top-level logic. I see several other comparisons of the same form. If you think that there is a lot of need for this kind of check, you can change the enum values to bits (1 << 0, 1 << 1, 1 << 2, etc.) and then use & to check for one of a group of values. I didn't yet check that the logic looks correct. I guess that it will be easier to verify once the comparisons are converted into something that is easier to reason about. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
