[sorry about double-post, something happened at gmail that caused some copies of this to be dropped]
On Tue, Sep 04, 2012 at 10:32:48AM +0900, Simon Horman wrote: > On Mon, Sep 03, 2012 at 09:24:19AM +0900, Simon Horman wrote: > > 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 <[email protected]> > > > > > > > > > > > > > > > > Add necessary macros to ofp-util for OF12 support. > > > > > > > > This is just a place holder. > > > > > > > > > > > > > > > > Signed-off-by: Isaku Yamahata <[email protected]> > > > > > > > > Signed-off-by: Simon Horman <[email protected]> > > > > > > > > > > > > > > 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. > > Here is a fresh revision of the patch. Applied to master, thank you. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
