[PATCH] Add cleanup routines.

[PATCH] Add cleanup routines.

[Hannes Weisbach]

Dynamically loaded library handles are saved in a list and dlclosed() on
exit. The list of struct ibv_driver *, as well as the global
struct ibv_device ** list are free()d.

Signed-off-by: Hannes Weisbach <hannes_weisbach-hi6Y0CQ0nG0@public.gmane.org>
---
 src/device.c | 10 ++++++++++
 src/init.c   | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/src/device.c b/src/device.c
index beb7b3c..d5b76bb 100644
--- a/src/device.c
+++ b/src/device.c
@@ -305,3 +305,13 @@ void __ibv_ack_async_event(struct ibv_async_event *event)
 	}
 }
 default_symver(__ibv_ack_async_event, ibv_ack_async_event);
+
+FINI static void ibverbs_deinit()
+{
+	size_t i;
+	for (i = 0; i < num_devices; i++) {
+		/* driver callback needed. May not be malloc'd memory */
+		free(device_list[i]);
+	}
+	free(device_list);
+}
diff --git a/src/init.c b/src/init.c
index d0e4b1c..2a8bca4 100644
--- a/src/init.c
+++ b/src/init.c
@@ -67,6 +67,11 @@ struct ibv_driver_name {
 	struct ibv_driver_name *next;
 };
 
+struct ibv_so_list {
+	void *dlhandle;
+	struct ibv_so_list *next;
+};
+
 struct ibv_driver {
 	const char	       *name;
 	ibv_driver_init_func	init_func;
@@ -77,6 +82,7 @@ struct ibv_driver {
 static struct ibv_sysfs_dev *sysfs_dev_list;
 static struct ibv_driver_name *driver_name_list;
 static struct ibv_driver *head_driver, *tail_driver;
+static struct ibv_so_list *so_list;
 
 static int find_sysfs_devs(void)
 {
@@ -193,7 +199,14 @@ void verbs_register_driver(const char *name, verbs_driver_init_func init_func)
 static void load_driver(const char *name)
 {
 	char *so_name;
-	void *dlhandle;
+	struct ibv_so_list *elem;
+	struct ibv_so_list **list;
+
+	elem = malloc(sizeof(*elem));
+	if(!elem)
+		return;
+
+	elem->next = NULL;
 
 #define __IBV_QUOTE(x)	#x
 #define IBV_QUOTE(x)	__IBV_QUOTE(x)
@@ -205,16 +218,25 @@ static void load_driver(const char *name)
 		     name) < 0) {
 		fprintf(stderr, PFX "Warning: couldn't load driver '%s'.\n",
 			name);
-		return;
+		goto err;
 	}
 
-	dlhandle = dlopen(so_name, RTLD_NOW);
-	if (!dlhandle) {
+	elem->dlhandle = dlopen(so_name, RTLD_NOW);
+	if (!elem->dlhandle) {
 		fprintf(stderr, PFX "Warning: couldn't load driver '%s': %s\n",
 			name, dlerror());
-		goto out;
+		goto err;
 	}
 
+	for (list = &so_list; *list; list = &(*list)->next)
+		;
+
+	*list = elem;
+
+	goto out;
+
+err:
+	free(elem);
 out:
 	free(so_name);
 }
@@ -573,3 +595,22 @@ out:
 
 	return num_devices;
 }
+
+FINI static void unload_drivers()
+{
+	struct ibv_driver *driver;
+	struct ibv_so_list * list;
+
+	for (driver = head_driver; driver;) {
+		struct ibv_driver *tmp = driver;
+		driver = driver->next;
+		free(tmp);
+	}
+
+	for (list = so_list; list;) {
+		struct ibv_so_list *tmp = list;
+		list = list->next;
+		dlclose(tmp->dlhandle);
+		free(tmp);
+	}
+}
-- 
1.8.5.5


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add cleanup routines.

[Yann Droneaud]

Hi,

Le vendredi 11 avril 2014 à 18:50 +0200, Hannes Weisbach a écrit :
> Dynamically loaded library handles are saved in a list and dlclosed() on
> exit. The list of struct ibv_driver *, as well as the global
> struct ibv_device ** list are free()d.
> 

Please adds some explanation, in particular the purpose of the changes.
Your commit message only explain "how", but you should also explain
"why".

> Signed-off-by: Hannes Weisbach <hannes_weisbach-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  src/device.c | 10 ++++++++++
>  src/init.c   | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/src/device.c b/src/device.c
> index beb7b3c..d5b76bb 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -305,3 +305,13 @@ void __ibv_ack_async_event(struct ibv_async_event *event)
>  	}
>  }
>  default_symver(__ibv_ack_async_event, ibv_ack_async_event);
> +
> +FINI static void ibverbs_deinit()

In C, empty prototype declare a function which accept any parameter. So
perhaps void ibverbs(void) is what you mean.

> +{
> +	size_t i;
> +	for (i = 0; i < num_devices; i++) {
> +		/* driver callback needed. May not be malloc'd memory */

Seems dangerous. Such interrogation must be explicitly added in the
commit message. 

> +		free(device_list[i]);
> +	}
> +	free(device_list);
> +}
> diff --git a/src/init.c b/src/init.c
> index d0e4b1c..2a8bca4 100644
> --- a/src/init.c
> +++ b/src/init.c
> @@ -67,6 +67,11 @@ struct ibv_driver_name {
>  	struct ibv_driver_name *next;
>  };
>  
> +struct ibv_so_list {
> +	void *dlhandle;
> +	struct ibv_so_list *next;
> +};
> +
>  struct ibv_driver {
>  	const char	       *name;
>  	ibv_driver_init_func	init_func;
> @@ -77,6 +82,7 @@ struct ibv_driver {
>  static struct ibv_sysfs_dev *sysfs_dev_list;
>  static struct ibv_driver_name *driver_name_list;
>  static struct ibv_driver *head_driver, *tail_driver;
> +static struct ibv_so_list *so_list;
>  
>  static int find_sysfs_devs(void)
>  {
> @@ -193,7 +199,14 @@ void verbs_register_driver(const char *name, verbs_driver_init_func init_func)
>  static void load_driver(const char *name)
>  {
>  	char *so_name;
> -	void *dlhandle;
> +	struct ibv_so_list *elem;
> +	struct ibv_so_list **list;
> +
> +	elem = malloc(sizeof(*elem));
> +	if(!elem)
> +		return;
> +
> +	elem->next = NULL;
>  
>  #define __IBV_QUOTE(x)	#x
>  #define IBV_QUOTE(x)	__IBV_QUOTE(x)
> @@ -205,16 +218,25 @@ static void load_driver(const char *name)
>  		     name) < 0) {
>  		fprintf(stderr, PFX "Warning: couldn't load driver '%s'.\n",
>  			name);
> -		return;
> +		goto err;
>  	}
>  
> -	dlhandle = dlopen(so_name, RTLD_NOW);
> -	if (!dlhandle) {
> +	elem->dlhandle = dlopen(so_name, RTLD_NOW);
> +	if (!elem->dlhandle) {
>  		fprintf(stderr, PFX "Warning: couldn't load driver '%s': %s\n",
>  			name, dlerror());
> -		goto out;
> +		goto err;
>  	}
>  
> +	for (list = &so_list; *list; list = &(*list)->next)
> +		;
> +
> +	*list = elem;
> +
> +	goto out;
> +
> +err:
> +	free(elem);
>  out:
>  	free(so_name);
>  }
> @@ -573,3 +595,22 @@ out:
>  
>  	return num_devices;
>  }
> +
> +FINI static void unload_drivers()

same remarks about prototype.

> +{
> +	struct ibv_driver *driver;
> +	struct ibv_so_list * list;
> +
> +	for (driver = head_driver; driver;) {
> +		struct ibv_driver *tmp = driver;
> +		driver = driver->next;
> +		free(tmp);
> +	}
> +

Is it safe here to free the driver ?

> +	for (list = so_list; list;) {
> +		struct ibv_so_list *tmp = list;
> +		list = list->next;
> +		dlclose(tmp->dlhandle);
> +		free(tmp);
> +	}
> +}

Why not store the dlopen() handle in the struct ibv_driver itself ?
This way only one list has to be scanned.

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add cleanup routines.

[Hannes Weisbach]

Am 11.04.2014 um 19:00 schrieb Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>:

> Hi,
Thanks for your quick reply.

> Please adds some explanation, in particular the purpose of the changes.
> Your commit message only explain "how", but you should also explain
> "why".
Ok.

> In C, empty prototype declare a function which accept any parameter. So
> perhaps void ibverbs(void) is what you mean.
Yup, thanks.

>> +{
>> +	size_t i;
>> +	for (i = 0; i < num_devices; i++) {
>> +		/* driver callback needed. May not be malloc'd memory */
> 
> Seems dangerous. Such interrogation must be explicitly added in the
> commit message. 
Maybe it's better to leave it as a TODO and only free the device_list
itself?

>> +	for (driver = head_driver; driver;) {
>> +		struct ibv_driver *tmp = driver;
>> +		driver = driver->next;
>> +		free(tmp);
>> +	}
>> +
> 
> Is it safe here to free the driver ?
The struct itself is no longer needed.  All the driver libraries are
loaded and ibverbs_init() is call-once, so no new libraries are
dlopen()ed (and call ibv_register_driver()).
It's probably nicer first to dlclose() all libraries and then free all
struct ibv_drivers.

>> +	for (list = so_list; list;) {
>> +		struct ibv_so_list *tmp = list;
>> +		list = list->next;
>> +		dlclose(tmp->dlhandle);
>> +		free(tmp);
>> +	}
>> +}
> 
> Why not store the dlopen() handle in the struct ibv_driver itself ?
> This way only one list has to be scanned.
That was my first idea, but the struct ibv_driver doesn't exist at
that time.  First the (driver) library is dlopen()d and calls
ibv_register_driver().  Only there is the ibv_driver struct allocated,
but the handle is gone.

Best regards,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html