[ovs-dev] [PATCH 1/5] lib/ofp-util: preparation for OF12 of ofp-util

Simon Horman horms at verge.net.au
Mon Sep 3 00:24:19 UTC 2012


On Fri, Aug 31, 2012 at 09:49:38AM -0700, Ben Pfaff wrote:
> On Fri, Aug 31, 2012 at 02:06:30PM +0900, Simon Horman wrote:
> > On Thu, Aug 30, 2012 at 09:02:14PM -0700, Ben Pfaff wrote:
> > > On Fri, Aug 31, 2012 at 11:51:19AM +0900, Simon Horman wrote:
> > > > On Thu, Aug 30, 2012 at 11:32:06AM -0700, Ben Pfaff wrote:
> > > > > On Thu, Aug 30, 2012 at 10:40:23AM +0900, Simon Horman wrote:
> > > > > > From: Isaku Yamahata <yamahata at valinux.co.jp>
> > > > > > 
> > > > > > Add necessary macros to ofp-util for OF12 support.
> > > > > > This is just a place holder.
> > > > > > 
> > > > > > Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
> > > > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > > > 
> > > > > I think that the new NOT_REACHED() in parse_named_action() can actually
> > > > > be hit if the user attempts to use this action.  I'd rather avoid adding
> > > > > that kind of breakage, even temporarily.  I can see a few ways to avoid
> > > > > it.  The most obvious ones are to leave OFPAT12_SET_FIELD commented out
> > > > > with the rest of the new actions in ofp-util.def or to give NULL as its
> > > > > name in ofp-util.def.
> > > > 
> > > > That sounds reasonable to me. I have gone for the NULL name option
> > > > in the revised patch (v4) below.
> > > 
> > > I get:
> > > 
> > > cc1: warnings being treated as errors
> > > ../lib/ofp-parse.c: In function ‘parse_named_action’:
> > > ../lib/ofp-parse.c:326: error: enumeration value ‘OFPUTIL_OFPAT12_SET_FIELD’ not handled in switch
> > 
> > Oh, sorry, I somehow missed that.  And in that case I'm unsure of the value
> > of setting the name of OFPUTIL_OFPAT12_SET_FIELD to NULL.
> 
> It is that, when you do that, the NOT_REACHED() can indeed not be
> reached: the action does not have a name so the function will not be
> called for that action.

Thanks for the clarification, I was a bit lost there.

With that in mind it sounds like the best option is to give
OFPUTIL_OFPAT12_SET_FIELD a NULL name and re-instate its case
in the switch in parse_named_action().

> > In any case, taking a small step back, the problem, as I see it is:
> > 
> > * OFPUTIL_OFPAT12_SET_FIELD needs to exist as it is used elsewhere
> >   in the patchset.
> > * That given, I'm unsure how parse_named_action() should handle it.
> >   - Silently ignored?
> >   - Using ovs_fatal(), which would seem to be consistent with
> >   other code in parse_named_action() and its caller str_to_ofpacts().
> 
> It's fine to have a fatal error but NOT_REACHED() is inappropriate
> for code that can actually be reached.  If the action has a name, then
> the code here can be reached.

Ok, thanks for the clarification.



More information about the dev mailing list