Re: [Qemu-devel] [PATCH 1/2] Add dynamic module loading for block drivers

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Aug 27, 2015 at 11:35:41AM +0200, Marc Marí wrote:
> On Thu, 27 Aug 2015 10:19:35 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Mon, Aug 17, 2015 at 10:09:34AM +0200, Marc Marí wrote:
> > > Extend the current module interface to allow for block drivers to
> > > be loaded dynamically on request.
> > > 
> > > The only block drivers that can be converted into modules are the
> > > drivers that don't perform any init operation except for
> > > registering themselves. This is why libiscsi has been disabled as a
> > > module.
> > 
> > Seems like we would just need to split the iscsi opts registration out
> > into a separate file that is permanently linked.
> > 
> > > All the necessary module information is located in a new structure
> > > found in include/qemu/module_block.h
> > > 
> > > Signed-off-by: Marc Marí <markmb@redhat.com>
> > > ---
> > >  block.c                     | 73
> > > +++++++++++++++++++++++++++++++++++-- configure
> > > |  2 +- include/qemu/module.h       |  3 ++
> > >  include/qemu/module_block.h | 89
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > util/module.c               | 38 ++++++------------- 5 files
> > > changed, 174 insertions(+), 31 deletions(-) create mode 100644
> > > include/qemu/module_block.h
> > > 
> > > diff --git a/block.c b/block.c
> > > index d088ee0..f24a624 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -27,6 +27,7 @@
> > >  #include "block/block_int.h"
> > >  #include "block/blockjob.h"
> > >  #include "qemu/error-report.h"
> > > +#include "qemu/module_block.h"
> > >  #include "qemu/module.h"
> > >  #include "qapi/qmp/qerror.h"
> > >  #include "qapi/qmp/qjson.h"
> > > @@ -277,11 +278,30 @@ void bdrv_add_close_notifier(BlockDriverState
> > > *bs, Notifier *notify) BlockDriver *bdrv_find_format(const char
> > > *format_name) {
> > >      BlockDriver *drv1;
> > > +    int i;
> > 
> > Nit-pick  'size_t' is a better type for loop iterators, especially
> > when combined with a sizeof() comparison. Some comment in later
> > functions too.
> > 
> > > +
> > >      QLIST_FOREACH(drv1, &bdrv_drivers, list) {
> > >          if (!strcmp(drv1->format_name, format_name)) {
> > >              return drv1;
> > >          }
> > >      }
> > > +
> > > +    for (i = 0; i < ARRAY_SIZE(block_driver_module); ++i) {
> > > +        if (!strcmp(block_driver_module[i].format_name,
> > > format_name)) {
> > > +
> > > block_module_load_one(block_driver_module[i].library_name);
> > > +            /* Copying code is not nice, but this way the current
> > > discovery is
> > > +             * not modified. Calling recursively could fail if the
> > > library
> > > +             * has been deleted.
> > > +             */
> > 
> > Can you explaiin what you mean here about "if the library has been
> > deleted" ?
> > 
> > Are you referring to possibilty of dlclose()ing the previously loaded
> > library, or about possibility of the module on disk having been
> > deleted or something else ?
> 
> I was thinking of relaunching the search by calling recursively the
> function. But this would loop infinitely if somebody, manually, deleted
> the library file.

Ok, yea, that makes sense.

 > If we have multiuple disks of the same type given to QEMU, it
> > seems like we might end up calling block_module_load_one()
> > multiple times for the same module & end up loading the same
> > .so multiple times as a result. Should module_load() keep a
> > record of everything it has loaded and short-circuit itself
> > to a no-op, so that callers of module_load() don't have to
> > worry about avoiding multiple calls.
>  
> I avoided that because glib already has it. It just increments the
> reference count. Which is not important unless we want to dlclose it.
> 
> https://developer.gnome.org/glib/stable/glib-Dynamic-Loading-of-Modules.html#g-module-open

Ah good.

NB, it is almost never safe to use dlclose() in a multi-threaded
application. A module may have created a thread-local variable
with a destructor function registered. There's no way to clean
these up prior to dlclose(), so the app would crash if you
dlclose() and a thread then exits. Since it is impractical to
audit all linked library code for use of thread locals, it is
safest to just avoid any use of dlclose().

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|