[openib-general] [PATCH] Bug in gen1 opensm/osm_port.c revision 520

[John Polstra]

Revision 520 of gen1/trunk/src/userspace/osm/opensm/osm_port.c
attempted to fix incorrect usage of cl_list_t structures.  Although
the original code was definitely wrong, I believe the fix in revision
520 is incomplete.  As evidence, consider this code snippet from
line 1003 of revision 520:

    currPortsList = &nextPortsList;
    cl_list_construct( &nextPortsList );
    cl_list_init( &nextPortsList, 10 );
    p_physp = (osm_physp_t*)cl_list_remove_head( currPortsList );
    while ( p_physp != NULL )
    {
        /* ... */
    }

The body of the loop will never execute, because currPortsList points
to the nextPortsList header, and nextPortsList has just been made
empty.

I think the intent of the code is that both currPortsList and
nextPortsList should be cl_list_t structures rather than pointers to
cl_list_t structures.  The snippet above should then look like this:

    currPortsList = nextPortsList;
    cl_list_construct( &nextPortsList );
    cl_list_init( &nextPortsList, 10 );
    p_physp = (osm_physp_t*)cl_list_remove_head( &currPortsList );
    while ( p_physp != NULL )
    {
        /* ... */
    }

So the first statement copies the whole list header (structure
copy) before reinitializing nextPortsList.

The attached patch is relative to the previous revision (357).  I
think it's the correct way to fix this code.

John

-------------- next part --------------
Index: osm_port.c
===================================================================
RCS file: /a/jdp/isicvs/osm/opensm/osm_port.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -r1.1.1.1 -r1.2
--- osm_port.c	11 Aug 2004 18:12:29 -0000	1.1.1.1
+++ osm_port.c	11 Aug 2004 18:20:13 -0000	1.2
@@ -888,7 +888,7 @@
   IN osm_bind_handle_t   *h_bind )
 {
   cl_list_t tmpPortsList;
-  osm_physp_t *p_physp, *p_src_physp;
+  osm_physp_t *p_physp, *p_src_physp = NULL;
   uint8_t path_array[IB_SUBNET_PATH_HOPS_MAX];
   uint8_t i = 0;
   osm_dr_path_t *p_dr_path;
@@ -943,8 +943,8 @@
   cl_map_t physp_map;
   cl_map_t      visited_map;
   osm_dr_path_t * p_dr_path;
-  cl_list_t     *currPortsList;
-  cl_list_t     *nextPortsList;
+  cl_list_t     currPortsList;
+  cl_list_t     nextPortsList;
   cl_qmap_t const     *p_port_tbl;
   osm_port_t    *p_port;
   osm_physp_t   *p_physp, *p_remote_physp;
@@ -967,8 +967,8 @@
      BFS from OSM port until we find the target physp but avoid 
      going through mapped ports
   */
-  cl_list_construct( nextPortsList );
-  cl_list_init( nextPortsList, 10 );
+  cl_list_construct( &nextPortsList );
+  cl_list_init( &nextPortsList, 10 );
 
   p_port_tbl = &p_subn->port_guid_tbl;
   port_guid = p_subn->sm_port_guid;
@@ -995,15 +995,15 @@
   CL_ASSERT( p_physp );
   CL_ASSERT( osm_physp_is_valid( p_physp ) );
 
-  cl_list_insert_tail( nextPortsList, p_physp );
+  cl_list_insert_tail( &nextPortsList, p_physp );
   
   while (next_list_is_full == TRUE) 
   {
     next_list_is_full = FALSE;
     currPortsList = nextPortsList;
-    cl_list_construct( nextPortsList );
-    cl_list_init( nextPortsList, 10 );
-    p_physp = (osm_physp_t*)cl_list_remove_head( currPortsList );
+    cl_list_construct( &nextPortsList );
+    cl_list_init( &nextPortsList, 10 );
+    p_physp = (osm_physp_t*)cl_list_remove_head( &currPortsList );
     while ( p_physp != NULL )
     {
       /* If we are in a switch - need to go out through all the other
@@ -1046,30 +1046,30 @@
           }
           
           /* add the p_remote_physp to the nextPortsList */
-          cl_list_insert_tail( nextPortsList, p_remote_physp );
+          cl_list_insert_tail( &nextPortsList, p_remote_physp );
           next_list_is_full = TRUE;
         }
       }
 
-      p_physp = (osm_physp_t*)cl_list_remove_head( currPortsList );
+      p_physp = (osm_physp_t*)cl_list_remove_head( &currPortsList );
       if ( reached_dest == TRUE )
       {
         /* free the rest of the currPortsList */
         while ( p_physp != NULL )
-          p_physp = (osm_physp_t*)cl_list_remove_head( currPortsList );
+          p_physp = (osm_physp_t*)cl_list_remove_head( &currPortsList );
         /* free the nextPortsList, if items were added to it */
-        p_physp = (osm_physp_t*)cl_list_remove_head( nextPortsList );
+        p_physp = (osm_physp_t*)cl_list_remove_head( &nextPortsList );
         while ( p_physp != NULL )
-          p_physp = (osm_physp_t*)cl_list_remove_head( nextPortsList );
+          p_physp = (osm_physp_t*)cl_list_remove_head( &nextPortsList );
         next_list_is_full = FALSE;
       }
     }
-    cl_list_destroy( currPortsList );
+    cl_list_destroy( &currPortsList );
   }
 
   /* cleanup */
  Exit:
-  cl_list_destroy( nextPortsList );
+  cl_list_destroy( &nextPortsList );
   cl_map_destroy( &physp_map );
   cl_map_destroy( &visited_map );
 }