alloc_ofp_port does not allocate the port number correctly

alloc_ofp_port() does not allocate the port number correctly if the port number passed initially is already in use. The following if block if (ofp_port >= ofproto->max_ports || bitmap_is_set(ofproto->ofp_port_ids, ofp_port)) { is entered when either of the two conditions is true but the while block after this is not entered if the second condition above is true and the first condition is false. This results in an existing port_number to be re-assigned! Signed-off-by: Krishna Kondaka <[email protected]> Signed-off-by: Justin Pettit <[email protected]>
osrg · Jan 11, 2013 · 6033d9d · 6033d9d
1 parent 8e70e19
commit 6033d9d
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 19 deletions.
diff --git a/AUTHORS b/AUTHORS
@@ -44,6 +44,7 @@ Joe Stringer            [email protected]
 Jun Nakajima            [email protected]
 Justin Pettit           [email protected]
 Keith Amidon            [email protected]
+Krishna Kondaka         [email protected]
 Kyle Mestery            [email protected]
 Leo Alterman            [email protected]
 Luca Giraudo            [email protected]

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
@@ -1613,38 +1613,30 @@ static uint16_t
 alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
 {
     uint16_t ofp_port;
+    uint16_t end_port_no = ofproto->alloc_port_no;
 
     ofp_port = simap_get(&ofproto->ofp_requests, netdev_name);
     ofp_port = ofp_port ? ofp_port : OFPP_NONE;
 
     if (ofp_port >= ofproto->max_ports
             || bitmap_is_set(ofproto->ofp_port_ids, ofp_port)) {
-        bool retry = ofproto->alloc_port_no ? true : false;
-
         /* Search for a free OpenFlow port number.  We try not to
          * immediately reuse them to prevent problems due to old
          * flows. */
-        while (ofp_port >= ofproto->max_ports) {
-            for (ofproto->alloc_port_no++;
-                 ofproto->alloc_port_no < ofproto->max_ports;
-                 ofproto->alloc_port_no++) {
-                if (!bitmap_is_set(ofproto->ofp_port_ids,
-                                   ofproto->alloc_port_no)) {
-                    ofp_port = ofproto->alloc_port_no;
-                    break;
-                }
+        for (;;) {
+            if (++ofproto->alloc_port_no >= ofproto->max_ports) {
+                ofproto->alloc_port_no = 0;
             }
-            if (ofproto->alloc_port_no >= ofproto->max_ports) {
-                if (retry) {
-                    ofproto->alloc_port_no = 0;
-                    retry = false;
-                } else {
-                    return OFPP_NONE;
-                }
+            if (!bitmap_is_set(ofproto->ofp_port_ids,
+                               ofproto->alloc_port_no)) {
+                ofp_port = ofproto->alloc_port_no;
+                break;
+            }
+            if (ofproto->alloc_port_no == end_port_no) {
+                return OFPP_NONE;
             }
         }
     }
-
     bitmap_set1(ofproto->ofp_port_ids, ofp_port);
     return ofp_port;
 }