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_weisb...@gmx.net>
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
