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

Simon Horman horms at verge.net.au
Tue Sep 4 01:32:48 UTC 2012


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 <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.

Here is a fresh revision of the patch.

From: Isaku Yamahata <yamahata at valinux.co.jp>

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 <yamahata at valinux.co.jp>
Signed-off-by: Simon Horman <horms at verge.net.au>

---

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




More information about the dev mailing list