| Message ID | A521BD06-7228-4FA9-97DD-7DFA51BE7505@gmx.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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@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.
Am 11.04.2014 um 19:00 schrieb Yann Droneaud <ydroneaud@opteya.com>: > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + } +}
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@gmx.net> --- src/device.c | 10 ++++++++++ src/init.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 56 insertions(+), 5 deletions(-)