[ovs-dev] [of1.1 draft v2 2/7] Introduce ofpacts, an abstraction of OpenFlow actions.

Ben Pfaff blp at nicira.com
Mon Feb 6 09:25:10 PST 2012


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?



More information about the dev mailing list