From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51460) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUtfo-00039P-GB for qemu-devel@nongnu.org; Thu, 27 Aug 2015 05:40:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUtfi-0003My-5i for qemu-devel@nongnu.org; Thu, 27 Aug 2015 05:40:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35440) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUtfh-0003Mi-RJ for qemu-devel@nongnu.org; Thu, 27 Aug 2015 05:40:38 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 5FA1DC1C49F0 for ; Thu, 27 Aug 2015 09:40:37 +0000 (UTC) Date: Thu, 27 Aug 2015 10:40:34 +0100 From: "Daniel P. Berrange" Message-ID: <20150827094033.GE24486@redhat.com> References: <1439798975-2488-1-git-send-email-markmb@redhat.com> <1439798975-2488-2-git-send-email-markmb@redhat.com> <20150827091935.GC24486@redhat.com> <20150827113541.28697140@markmb_rh> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150827113541.28697140@markmb_rh> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] Add dynamic module loading for block drivers Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marc =?utf-8?B?TWFyw60=?= Cc: qemu-devel On Thu, Aug 27, 2015 at 11:35:41AM +0200, Marc Mar=C3=AD wrote: > On Thu, 27 Aug 2015 10:19:35 +0100 > "Daniel P. Berrange" wrote: >=20 > > On Mon, Aug 17, 2015 at 10:09:34AM +0200, Marc Mar=C3=AD wrote: > > > Extend the current module interface to allow for block drivers to > > > be loaded dynamically on request. > > >=20 > > > 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. > >=20 > > Seems like we would just need to split the iscsi opts registration ou= t > > into a separate file that is permanently linked. > >=20 > > > All the necessary module information is located in a new structure > > > found in include/qemu/module_block.h > > >=20 > > > Signed-off-by: Marc Mar=C3=AD > > > --- > > > 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 > > >=20 > > > 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; > >=20 > > Nit-pick 'size_t' is a better type for loop iterators, especially > > when combined with a sizeof() comparison. Some comment in later > > functions too. > >=20 > > > + > > > QLIST_FOREACH(drv1, &bdrv_drivers, list) { > > > if (!strcmp(drv1->format_name, format_name)) { > > > return drv1; > > > } > > > } > > > + > > > + for (i =3D 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. > > > + */ > >=20 > > Can you explaiin what you mean here about "if the library has been > > deleted" ? > >=20 > > 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 ? >=20 > 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. > =20 > I avoided that because glib already has it. It just increments the > reference count. Which is not important unless we want to dlclose it. >=20 > 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 --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|