On Mon, Feb 06, 2012 at 09:25:10AM -0800, Ben Pfaff wrote: > Thank you for the review. > > On Mon, Feb 06, 2012 at 02:34:01PM +0900, Simon Horman wrote: > > On Fri, Feb 03, 2012 at 12:43:50PM -0800, Ben Pfaff wrote: > > > OpenFlow actions have always been somewhat awkward to handle. > > > Moreover, over time we've started creating actions that require more > > > complicated parsing. When we maintain those actions internally in > > > their wire format, we end up parsing them multiple times, whenever > > > we have to look at the set of actions. > > > > > > When we add support for OpenFlow 1.1 or later protocols, the situation > > > will get worse, because these newer protocols support many of the same > > > actions but with different representations. It becomes unrealistic to > > > handle each protocol in its wire format. > > > > > > This commit adopts a new strategy, by converting OpenFlow actions into > > > an internal form from the wire format when they are read, and converting > > > them back to the wire format when flows are dumped. I believe that this > > > will be more maintainable over time. > > [snip] > > > > @@ -290,15 +328,15 @@ bundle_parse(struct ofpbuf *b, const char *s) > > > slave_type = strtok_r(NULL, ", ", &save_ptr); > > > slave_delim = strtok_r(NULL, ": ", &save_ptr); > > > > Not strictly related to this patch, but it is unclear to me how it > > is ensured that the strtok_r calls above (and others above them) > > will not return NULL and it seems that bundle_parse__() will be > > rather unhappy if any of the values are Null. > > bundle_parse__() checks that its slave_delim argument is nonnull. If > the final value returned by strtok_r() is nonnull, I think that all of > the previous values returned must also be nonnull. Do you see any > holes?
Sorry for missing the significance of that check, I no longer see a hole. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
