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" <berra...@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í <mar...@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 :|
