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

Simon Horman horms at verge.net.au
Mon Feb 13 02:38:14 UTC 2012


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.



More information about the dev mailing list