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. From: Isaku Yamahata <[email protected]> lib/ofp-util: preparation for OF12 of ofp-util 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]> --- v5 [Simon Horman] * Re-instate OFPUTIL_OFPAT12_SET_FIELD case in parse_named_action() It is safe to use NOT_REACHED() here as the NULL name change in v4 ensures the case should not occur under normal use. Thanks to Ben Pfaff for explaining this. v4 [Simon Horman] * Give OFPAT12_SET_FIELD a NULL name in ofp-util.def and omit it from the case statement in parse_named_action(). The actual name, "set_field", can be used once there is code to back the case statement in parse_named_action(). This was suggested by Ben Pfaff as an alternative to using NOT_REACHED() in the OFPAT12_SET_FIELD case in parse_named_action(), which would cause breakage if a user attempted to use this action. v3 * No change v2 * No change v1 [Isaku Yamahata] * Initial Post --- lib/ofp-actions.c | 3 +++ lib/ofp-parse.c | 6 ++++++ lib/ofp-print.c | 1 + lib/ofp-util.c | 3 +++ lib/ofp-util.def | 23 +++++++++++++++++++++++ lib/ofp-util.h | 6 ++++++ 6 files changed, 42 insertions(+) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 898455e..cf886cf 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -283,6 +283,7 @@ ofpact_from_nxast(const union ofp_action *a, enum ofputil_action_code code, case OFPUTIL_ACTION_INVALID: #define OFPAT10_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: #define OFPAT11_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: #include "ofp-util.def" NOT_REACHED(); @@ -397,6 +398,7 @@ ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out) switch (code) { case OFPUTIL_ACTION_INVALID: #define OFPAT11_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: #include "ofp-util.def" NOT_REACHED(); @@ -659,6 +661,7 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out) switch (code) { case OFPUTIL_ACTION_INVALID: #define OFPAT10_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM: #include "ofp-util.def" NOT_REACHED(); diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 0d904b1..2bd765b 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -350,6 +350,12 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow, ofpact_put_SET_VLAN_PCP(ofpacts)->vlan_pcp = pcp; break; + case OFPUTIL_OFPAT12_SET_FIELD: + NOT_REACHED(); /* This will be implemented by later patch and + * enabled using a non-NULL name in + * OFPAT12_ACTION(OFPAT12_SET_FIELD, ...) */ + break; + case OFPUTIL_OFPAT10_STRIP_VLAN: ofpact_put_STRIP_VLAN(ofpacts); break; diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 99e6456..d00c015 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -457,6 +457,7 @@ ofputil_action_bitmap_to_name(uint32_t bit) case OFPUTIL_A_SET_NW_TOS: return "SET_NW_TOS"; case OFPUTIL_A_SET_TP_SRC: return "SET_TP_SRC"; case OFPUTIL_A_SET_TP_DST: return "SET_TP_DST"; + case OFPUTIL_A_SET_FIELD: return "SET_FIELD"; case OFPUTIL_A_ENQUEUE: return "ENQUEUE"; case OFPUTIL_A_COPY_TTL_OUT: return "COPY_TTL_OUT"; case OFPUTIL_A_COPY_TTL_IN: return "COPY_TTL_IN"; diff --git a/lib/ofp-util.c b/lib/ofp-util.c index ce9bb74..1c7a6a7 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -3514,6 +3514,7 @@ ofputil_action_code_from_name(const char *name) NULL, #define OFPAT10_ACTION(ENUM, STRUCT, NAME) NAME, #define OFPAT11_ACTION(ENUM, STRUCT, NAME) NAME, +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) NAME, #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) NAME, #include "ofp-util.def" }; @@ -3543,6 +3544,7 @@ ofputil_put_action(enum ofputil_action_code code, struct ofpbuf *buf) #define OFPAT10_ACTION(ENUM, STRUCT, NAME) \ case OFPUTIL_##ENUM: return ofputil_put_##ENUM(buf); #define OFPAT11_ACTION OFPAT10_ACTION +#define OFPAT12_ACTION OFPAT10_ACTION #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) \ case OFPUTIL_##ENUM: return ofputil_put_##ENUM(buf); #include "ofp-util.def" @@ -3567,6 +3569,7 @@ ofputil_put_action(enum ofputil_action_code code, struct ofpbuf *buf) return s; \ } #define OFPAT11_ACTION OFPAT10_ACTION +#define OFPAT12_ACTION OFPAT10_ACTION #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) \ void \ ofputil_init_##ENUM(struct STRUCT *s) \ diff --git a/lib/ofp-util.def b/lib/ofp-util.def index 391c14b..6f5113e 100644 --- a/lib/ofp-util.def +++ b/lib/ofp-util.def @@ -36,6 +36,28 @@ OFPAT11_ACTION(OFPAT11_SET_TP_DST, ofp_action_tp_port, "mod_tp_dst") //OFPAT11_ACTION(OFPAT11_SET_NW_TTL, ofp11_action_nw_ttl, "set_nw_ttl") //OFPAT11_ACTION(OFPAT11_DEC_NW_TTL, ofp_action_header, "dec_ttl") +#ifndef OFPAT12_ACTION +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) +#endif +//OFPAT12_ACTION(OFPAT12_OUTPUT, , "output") +//OFPAT12_ACTION(OFPAT12_COPY_TTL_OUT, ofp_action_header, "copy_ttl_out") +//OFPAT12_ACTION(OFPAT12_COPY_TTL_IN, ofp_action_header, "copy_ttl_in") +//OFPAT12_ACTION(OFPAT12_SET_MPLS_TTL, , "set_mpls_ttl") +//OFPAT12_ACTION(OFPAT12_DEC_MPLS_TTL, ofp_action_header, "dec_mpls_ttl") +//OFPAT12_ACTION(OFPAT12_PUSH_VLAN, , "push_vlan") +//OFPAT12_ACTION(OFPAT12_POP_VLAN, ofp_action_header, "pop_vlan") +//OFPAT12_ACTION(OFPAT12_PUSH_MPLS, , "push_mpls") +//OFPAT12_ACTION(OFPAT12_POP_MPLS, , "pop_mpls") +//OFPAT12_ACTION(OFPAT12_SET_QUEUE, , "set_queue") +//OFPAT12_ACTION(OFPAT12_GROUP, , "group") +//OFPAT12_ACTION(OFPAT12_SET_NW_TTL, , "set_nw_ttl") +//OFPAT12_ACTION(OFPAT12_DEC_NW_TTL, ofp_action_header, "dec_ttl") +//Use non-NULL name for OFPAT12_SET_FIELD once the code for +//the OFPUTIL_OFPAT12_SET_FIELD case in parse_named_action() is implemented +//OFPAT12_ACTION(OFPAT12_SET_FIELD, ofp12_action_set_field, "set_field") +OFPAT12_ACTION(OFPAT12_SET_FIELD, ofp12_action_set_field, NULL) +//OFPAT12_ACTION(OFPAT12_EXPERIMENTER, , ) + #ifndef NXAST_ACTION #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) #endif @@ -62,4 +84,5 @@ NXAST_ACTION(NXAST_DEC_TTL_CNT_IDS, nx_action_cnt_ids, 1, NULL) #undef OFPAT10_ACTION #undef OFPAT11_ACTION +#undef OFPAT12_ACTION #undef NXAST_ACTION diff --git a/lib/ofp-util.h b/lib/ofp-util.h index 9cc3028..bdc783b 100644 --- a/lib/ofp-util.h +++ b/lib/ofp-util.h @@ -399,6 +399,7 @@ enum ofputil_action_bitmap { OFPUTIL_A_GROUP = 1 << 24, OFPUTIL_A_SET_NW_TTL = 1 << 25, OFPUTIL_A_DEC_NW_TTL = 1 << 26, + OFPUTIL_A_SET_FIELD = 1 << 27, }; /* Abstract ofp_switch_features. */ @@ -551,6 +552,7 @@ enum OVS_PACKED_ENUM ofputil_action_code { OFPUTIL_ACTION_INVALID, #define OFPAT10_ACTION(ENUM, STRUCT, NAME) OFPUTIL_##ENUM, #define OFPAT11_ACTION(ENUM, STRUCT, NAME) OFPUTIL_##ENUM, +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) OFPUTIL_##ENUM, #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) OFPUTIL_##ENUM, #include "ofp-util.def" }; @@ -559,6 +561,7 @@ enum OVS_PACKED_ENUM ofputil_action_code { enum { #define OFPAT10_ACTION(ENUM, STRUCT, NAME) + 1 #define OFPAT11_ACTION(ENUM, STRUCT, NAME) + 1 +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) + 1 #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) + 1 OFPUTIL_N_ACTIONS = 1 #include "ofp-util.def" @@ -588,6 +591,9 @@ void *ofputil_put_action(enum ofputil_action_code, struct ofpbuf *buf); #define OFPAT11_ACTION(ENUM, STRUCT, NAME) \ void ofputil_init_##ENUM(struct STRUCT *); \ struct STRUCT *ofputil_put_##ENUM(struct ofpbuf *); +#define OFPAT12_ACTION(ENUM, STRUCT, NAME) \ + void ofputil_init_##ENUM(struct STRUCT *); \ + struct STRUCT *ofputil_put_##ENUM(struct ofpbuf *); #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) \ void ofputil_init_##ENUM(struct STRUCT *); \ struct STRUCT *ofputil_put_##ENUM(struct ofpbuf *); -- 1.7.10.4 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
