From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43635) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXZ6y-0005MR-8q for qemu-devel@nongnu.org; Wed, 10 Aug 2016 15:24:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXZ6w-0006KK-12 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 15:24:19 -0400 References: <1470679640-18366-1-git-send-email-clord@redhat.com> <1470679640-18366-4-git-send-email-clord@redhat.com> <62914d01-ac55-efba-2b2d-866cb833c823@redhat.com> <56832672-88dd-0b93-3380-76a00580f829@redhat.com> From: Colin Lord Message-ID: <457c20b2-767f-858c-7431-b971b8c01426@redhat.com> Date: Wed, 10 Aug 2016 15:24:08 -0400 MIME-Version: 1.0 In-Reply-To: <56832672-88dd-0b93-3380-76a00580f829@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v7 3/4] blockdev: Add dynamic module loading for block drivers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Marc Mari , qemu-block@nongnu.org On 08/10/2016 03:04 PM, Colin Lord wrote: > On 08/10/2016 02:37 PM, Max Reitz wrote: >> On 08.08.2016 20:07, Colin Lord wrote: >>> From: Marc Mari >>> >>> 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. >>> >>> In addition, only the protocol drivers are being modularized, as they >>> are the only ones which see significant performance benefits. The for= mat >>> drivers do not generally link to external libraries, so modularizing >>> them is of no benefit from a performance perspective. >>> >>> All the necessary module information is located in a new structure fo= und >>> in module_block.h >>> >>> Signed-off-by: Marc Mar=C3=AD >>> Signed-off-by: Colin Lord >>> Reviewed-by: Stefan Hajnoczi >>> --- >>> Makefile | 3 --- >>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++= ++++------ >>> block/Makefile.objs | 3 +-- >>> include/qemu/module.h | 3 +++ >>> util/module.c | 38 +++++++++---------------------- >>> 5 files changed, 70 insertions(+), 39 deletions(-) >>> >> >> [...] >> >>> diff --git a/block.c b/block.c >>> index 30d64e6..6c5e249 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -26,6 +26,7 @@ >>> #include "block/block_int.h" >>> #include "block/blockjob.h" >>> #include "qemu/error-report.h" >>> +#include "module_block.h" >>> #include "qemu/module.h" >>> #include "qapi/qmp/qerror.h" >>> #include "qapi/qmp/qbool.h" >>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >>> return bs; >>> } >>> =20 >>> -BlockDriver *bdrv_find_format(const char *format_name) >>> +static BlockDriver *bdrv_do_find_format(const char *format_name) >>> { >>> BlockDriver *drv1; >>> + >>> QLIST_FOREACH(drv1, &bdrv_drivers, list) { >>> if (!strcmp(drv1->format_name, format_name)) { >>> return drv1; >>> } >>> } >>> + >>> return NULL; >>> } >>> =20 >>> +BlockDriver *bdrv_find_format(const char *format_name) >>> +{ >>> + BlockDriver *drv1; >>> + size_t i; >>> + >>> + drv1 =3D bdrv_do_find_format(format_name); >>> + if (drv1) { >>> + return drv1; >>> + } >>> + >>> + /* The driver isn't registered, maybe we need to load a module *= / >>> + for (i =3D 0; i < ARRAY_SIZE(block_driver_modules); ++i) { >>> + if (!strcmp(block_driver_modules[i].format_name, format_name= )) { >>> + block_module_load_one(block_driver_modules[i].library_na= me); >>> + break; >>> + } >>> + } >>> + >>> + return bdrv_do_find_format(format_name); >>> +} >>> + >> >> Did you reintroduce this function for dmg? I thought Fam is taking car= e >> of that? I'm confused as to how Fam's patch for dmg and this series ar= e >> supposed to interact; the fact that the script added in patch 2 breaks >> down with Fam's patch isn't exactly helping... >> >> Hm, so is this series now supposed to be applied without Fam's patch >> with the idea of sorting dmg out later on? >> >> Max >> >>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >>> { >>> static const char *whitelist_rw[] =3D { >> > I'm not completely sure how Fam's patch is supposed to interact with > this series actually. I'm kind of hoping it can be done on top of my > patches at some future point, but in either case this revision was not > done with the dmg patch in mind. The change in find_format was actually > due to a bug I discovered in my patch series (I fixed it in v6, but you > may have missed that). >=20 > Essentially, if a user specifies the driver explicitly as part of their > call to qemu, eg driver=3Dgluster, there was a bug in v5 where if the > driver was modularized, it would not be found/loaded. So since gluster > was modularized, if you said driver=3Dgluster on the command line, the > gluster module would not be found. The modules could be found by probin= g > perfectly fine, this only happened when the driver was specified > manually. The reason is because the drivers get searched based on the > format field if they're specified manually, which means bdrv_find_forma= t > gets called when the driver is specified on the command line. This make= s > it necessary for bdrv_find_format to take into account modularized > drivers even though the format drivers are not being modularized. That'= s > also why the format field was added to the module_block header file aga= in. >=20 > Colin >=20 Oops, this was meant to be a reply to your response of patch 4/4, in case anyone gets confused. Colin