[ovs-dev] [PATCH 06/14] connmgr: Use version of underlying rconn

[Simon Horman]

On Mon, Nov 12, 2012 at 10:20:26AM -0800, Ben Pfaff wrote:
> On Wed, Nov 07, 2012 at 05:03:04PM +0900, Simon Horman wrote:
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> It seems that OFPUTIL_P_NONE is not really a meaningful protocol enum
> value.  It just ends up being marked as NOT_REACHED() in every switch
> statement, for example.  I'd suggest avoiding that either by changing
> OFPUTIL_P_NONE from an enum to a macro or by just using 0 (which is a
> perfectly reasonable value for a bit-mask, really).

To be honest I'm not a big fan of the hybrid enum/#ifdef approach
to OFPUTIL_P_* values. But I do see your point. I have changed the code
to use

#define OFPUTIL_P_NONE 0

> But I'm also concerned that the protocol of an ofconn can initially be
> "none".  Grepping for ofconn_get_protocol() or looking for ->protocol in
> connmgr.c, I see a number of places where asynchronous messages (that
> is, messages not in response to some request) are encoded to an ofconn.
> I expect that all of those would assert-fail if ->protocol was 0.  If
> any of those can trigger sending a message on an ofconn before its
> protocol is determined, then we've got a race in this code.

I'm unsure if any of those paths can trigger, but I do agree
that it looks hairy.

How about this approach? My assumption is that if the underlying rconn
has no protocol then there is no connection and replies don't make
any sense.

----------------------------------------------------------------
connmgr: Use version of underlying rconn

Signed-off-by: Simon Horman <horms at verge.net.au>

---

v5
* Use #define for OFPUTIL_P_NONE
* Ensure that if ofconn->rconn is connected that a valid
  value for ofconn->protocol is always used by fetching the
  value from the rconn in ofconn_get_protocol() as necessary
  and always using ofconn_get_protocol() to access ofconn->protocol.

v4
* Rebase
---
 lib/ofp-util.h    |    2 ++
 ofproto/connmgr.c |   18 +++++++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index f276804..0be5a39 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -58,6 +58,8 @@ int ofputil_netmask_to_wcbits(ovs_be32 netmask);
  * to implement set union and intersection.
  */
 enum ofputil_protocol {
+#define OFPUTIL_P_NONE 0
+
     /* OpenFlow 1.0-based protocols. */
     OFPUTIL_P_OF10     = 1 << 0, /* OpenFlow 1.0 flow format. */
     OFPUTIL_P_OF10_TID = 1 << 1, /* OF1.0 + flow_mod_table_id extension. */
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index ba93a1d..5c7e1c5 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -840,10 +840,18 @@ ofconn_get_invalid_ttl_to_controller(struct ofconn *ofconn)
 
 /* Returns the currently configured protocol for 'ofconn', one of OFPUTIL_P_*.
  *
- * The default, if no other format has been set, is OFPUTIL_P_OPENFLOW10. */
+ * The default, if no other format has been set, is OFPUTIL_P_NONE. */
 enum ofputil_protocol
 ofconn_get_protocol(struct ofconn *ofconn)
 {
+    if (ofconn->protocol == OFPUTIL_P_NONE &&
+        rconn_is_connected(ofconn->rconn)) {
+        int version = rconn_get_version(ofconn->rconn);
+        if (version > 0) {
+            ofconn_set_protocol(ofconn,
+                                ofputil_protocol_from_ofp_version(version));
+        }
+    }
     return ofconn->protocol;
 }
 
@@ -1034,7 +1042,7 @@ ofconn_flush(struct ofconn *ofconn)
     int i;
 
     ofconn->role = NX_ROLE_OTHER;
-    ofconn->protocol = OFPUTIL_P_OF10;
+    ofconn_set_protocol(ofconn, OFPUTIL_P_NONE);
     ofconn->packet_in_format = NXPIF_OPENFLOW10;
 
     /* Disassociate 'ofconn' from all of the ofopgroups that it initiated that
@@ -1317,7 +1325,7 @@ connmgr_send_port_status(struct connmgr *mgr,
         if (ofconn_receives_async_msg(ofconn, OAM_PORT_STATUS, reason)) {
             struct ofpbuf *msg;
 
-            msg = ofputil_encode_port_status(&ps, ofconn->protocol);
+            msg = ofputil_encode_port_status(&ps, ofconn_get_protocol(ofconn));
             ofconn_send(ofconn, msg, NULL);
         }
     }
@@ -1340,7 +1348,7 @@ connmgr_send_flow_removed(struct connmgr *mgr,
              * also prevents new flows from being added (and expiring).  (It
              * also prevents processing OpenFlow requests that would not add
              * new flows, so it is imperfect.) */
-            msg = ofputil_encode_flow_removed(fr, ofconn->protocol);
+            msg = ofputil_encode_flow_removed(fr, ofconn_get_protocol(ofconn));
             ofconn_send_reply(ofconn, msg);
         }
     }
@@ -1411,7 +1419,7 @@ schedule_packet_in(struct ofconn *ofconn, struct ofputil_packet_in pin)
      * while (until a later call to pinsched_run()). */
     pinsched_send(ofconn->schedulers[pin.reason == OFPR_NO_MATCH ? 0 : 1],
                   pin.fmd.in_port,
-                  ofputil_encode_packet_in(&pin, ofconn->protocol,
+                  ofputil_encode_packet_in(&pin, ofconn_get_protocol(ofconn),
                                            ofconn->packet_in_format),
                   do_send_packet_in, ofconn);
 }
-- 
1.7.10.4