From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXYpt-000473-Im for qemu-devel@nongnu.org; Wed, 10 Aug 2016 15:06:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXYpr-0001Gr-97 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 15:06:40 -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: Max Reitz Message-ID: <0ce88d12-a583-5a5c-14ce-7667c20a38ad@redhat.com> Date: Wed, 10 Aug 2016 21:06:27 +0200 MIME-Version: 1.0 In-Reply-To: <56832672-88dd-0b93-3380-76a00580f829@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="07a3cQVgNhvvbWhPb94lCEXTcEBpKh0gW" 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: Colin Lord , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org, Marc Mari This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --07a3cQVgNhvvbWhPb94lCEXTcEBpKh0gW From: Max Reitz To: Colin Lord , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org, Marc Mari Message-ID: <0ce88d12-a583-5a5c-14ce-7667c20a38ad@redhat.com> Subject: Re: [PATCH v7 3/4] blockdev: Add dynamic module loading for block drivers 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> In-Reply-To: <56832672-88dd-0b93-3380-76a00580f829@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10.08.2016 21:04, 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. Ah, that makes sense, thanks for explaining. Patches 1-3: Reviewed-by: Max Reitz --07a3cQVgNhvvbWhPb94lCEXTcEBpKh0gW Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEvBAEBCAAZBQJXq3szEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrWq3 B/wLVgz5M3In/hP0t84kqo5vXo15Qml8DnDa3c3wkuCO2sIxg3m0i9lrDtcIrWQD yxavPgktIeHeENUGJNRUEk4GqPrIcriFmXzHelDs8m25CgKvKd5afO65KaCybB5w azo+saIxRGEdXY29iqR9CGSo49uxNoj72+vtprVNDm7zZyHhYrBxFKocxfl/+jfa f/6stOCx4nZePf6NqUwgn8TnhyYYaNqTcAiS+rzSKEdX6SoaPZhH6wdPaPpjAkGG STX0aR432xIcyEhvQeb1jQMJrrNP5RwHatFwHuEXRZU+2jQEn8x2xk2Q/14edo7P 9ZwQdFFK2DRKZ1RZfyFDSrj/ =2bMP -----END PGP SIGNATURE----- --07a3cQVgNhvvbWhPb94lCEXTcEBpKh0gW--