[openib-general] RE: [PATCHv2] OpenSM: OpenIB vendor layer: Implement osm_vendor_d elete

[Yael Kalka]

Hi Hal,
There is a problem with the patch.
1. In osm_vendor_unbind: You used free(p_bind), when the pointer was
allocated using cl_zalloc.
   You need to use cl_free.
2. There is a race between the cl_free and the receiver thread. We get a
segmentation fault due to the fact that the thread isn't destroyed before
freeing the p_bind object. There should be a way to signal the reciever
thread to exit, and the unbind should wait for that thread to join.

Thanks,
Yael

-----Original Message-----
From: Hal Rosenstock [mailto:halr at voltaire.com]
Sent: Tuesday, September 06, 2005 5:27 PM
To: Yael Kalka; Eitan Zahavi
Cc: openib-general at openib.org
Subject: [PATCHv2] OpenSM: OpenIB vendor layer: Implement
osm_vendor_delete


[same patch just generated with diff -up]

OpenSM: OpenIB vendor layer: Implement osm_vendor_delete

[I've done some testing of this; are there any regressions for this ?]

Signed-off-by: Hal Rosenstock <halr at voltaire.com>

--- osm_vendor_ibumad.c.1	2005-08-31 12:26:03.000000000 -0400
+++ osm_vendor_ibumad.c	2005-09-06 09:35:27.000000000 -0400
@@ -483,14 +483,8 @@ void
 osm_vendor_delete(
   IN osm_vendor_t** const pp_vend )
 {
-	int agent_id;
-
-	/* unregister UMAD agents */
-	for (agent_id = 0; agent_id < UMAD_CA_MAX_AGENTS; agent_id++)
-		if ( (*pp_vend)->agents[agent_id] )
-			umad_unregister( (*pp_vend)->umad_port_id, agent_id
);
 	clear_madw( *pp_vend );
-	/* make sure all ports are closed? */
+	/* make sure all ports are closed */
 	umad_done();
 	cl_free( *pp_vend );
 	*pp_vend = NULL;
@@ -593,7 +587,7 @@ Exit:
 
 /**********************************************************************
  **********************************************************************/
-int
+static int
 osm_vendor_open_port(
   IN osm_vendor_t* const p_vend,
   IN const ib_net64_t port_guid )
@@ -828,11 +822,27 @@ osm_vendor_unbind(
   IN osm_bind_handle_t h_bind)
 {
 	osm_umad_bind_info_t *p_bind = ( osm_umad_bind_info_t * ) h_bind;
-	osm_vendor_t *p_vend = p_bind->p_vend;
+	osm_vendor_t *p_vend;
+
+	if (p_bind) {
+		p_vend = p_bind->p_vend;
+
+		OSM_LOG_ENTER( p_vend->p_log, osm_vendor_unbind );
+
+		/* Unregister UMAD agents */
+		if (p_vend->agents[p_bind->agent_id1])
+			umad_unregister(p_bind->port_id, p_bind->agent_id1);
+		if (p_vend->agents[p_bind->agent_id])
+			umad_unregister(p_bind->port_id, p_bind->agent_id);
 
-	OSM_LOG_ENTER( p_vend->p_log, osm_vendor_unbind );
+		/* close port ??? */
+
+		free(p_bind);
+
+		OSM_LOG_EXIT( p_vend->p_log);
+
+	}
 
-	OSM_LOG_EXIT( p_vend->p_log);
 }
 
 /**********************************************************************

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20050911/c9266591/attachment.html>