From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46576) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRlia-00070y-GY for qemu-devel@nongnu.org; Mon, 25 Jul 2016 15:39:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRliY-00008A-3u for qemu-devel@nongnu.org; Mon, 25 Jul 2016 15:39:11 -0400 References: <1469025050-12715-1-git-send-email-clord@redhat.com> <1b3ab482-d6e1-c6a9-c8f7-ee886a7b12e1@redhat.com> From: Max Reitz Message-ID: Date: Mon, 25 Jul 2016 21:38:59 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="huCGiHOQo7UEEbipHcNW6cR9odL0M2pF4" Subject: Re: [Qemu-devel] [PATCH v5 0/3] 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 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --huCGiHOQo7UEEbipHcNW6cR9odL0M2pF4 From: Max Reitz To: Colin Lord , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org Message-ID: Subject: Re: [Qemu-devel] [PATCH v5 0/3] Dynamic module loading for block drivers References: <1469025050-12715-1-git-send-email-clord@redhat.com> <1b3ab482-d6e1-c6a9-c8f7-ee886a7b12e1@redhat.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 25.07.2016 15:56, Colin Lord wrote: > On 07/23/2016 02:21 PM, Max Reitz wrote: >> On 20.07.2016 16:30, Colin Lord wrote: >>> Here's v5 of the modularization series. Since it seems the concensus = is >>> that modularizing the format drivers is unnecessary, this series no >>> longer modularizes those and is thus much shorter than before. >>> >>> v5: >>> - No format drivers are modularized, therefore the probe functions ar= e >>> all being left completely untouched. The bdrv_find_format function = is >>> also left untouched as a result. >> >> You also left the (host) device probing functions untouched in this >> revision. However, those are actually only used by protocol drivers >> (raw-posix and raw-win32, to be specific). >> >> Probably fine since I think it's impossible to build raw-posix or >> raw-win32 as a module anyway (because bdrv_file is used as a "static" >> reference in block.c). >> >>> - Remove dmg from block-obj-m since it is not a target of the >>> modularization effort. >> >> Hm, I'm afraid I don't quite understand the reasoning behind this. >> Intuitively, I'd say "Doesn't matter, it was already modular, so what >> prevents it from staying that way?" >> >> Is it because the changes to util/module.c in patch 3 break how the dm= g >> module worked, e.g. that it was always implicitly fully loaded on qemu= >> startup if it was available, but now modules are only loaded on reques= t? >> > Yes, that's pretty much it. Previously all the modules would get loaded= > during initialization, but since the third patch adds dynamic loading > that no longer happens. As we aren't loading format drivers on demand, > dmg.o should be added to block-obj-y instead of block-obj-m so that it > doesn't get modularized. OK, then, for the series: Reviewed-by: Max Reitz > Also, I should mention that the third patch of this series is not quite= > right. I was looking at some stuff with John on Friday and he found a > couple lines I somehow didn't delete from qemu/Makefile (around line 25= 0): >=20 > block-modules =3D $(foreach o,$(block-obj-m),"$(basename $(subst > /,-,$o))",) NULL > util/module.o-cflags =3D -D'CONFIG_BLOCK_MODULES=3D$(block-modules)' >=20 > I was pretty sure I had taken care of these but I guess not. It doesn't= > actually affect anything as all it's doing is setting > CONFIG_BLOCK_MODULES (which is no longer used anywhere once patch 3 get= s > applied), but it's obviously not good to have unused code sitting > around. Should I submit another version with this fixed? While these lines should eventually of course be removed, this can simply be done in a follow-up patch just as well. Max >=20 > Colin >=20 >> Max >> >>> - Modify module_block.py to only include the library name and protoco= l >>> name fields in the generated struct. The other fields are no longer= >>> necessary for the drivers that are being modularized. >>> >>> v4: >>> - Fix indentation of the generated header file module_block.h >>> - Drivers and probe functions are now all located in the block/ >>> directory, rather than being split between block/ and block/probe/.= In >>> addition the header files for each probe/driver pair are in the blo= ck/ >>> directory, not the include/block/driver/ directory (which no longer= >>> exists). >>> - Since the probe files are in block/ now, they follow the naming >>> pattern of format-probe.c >>> - Renamed crypto probe file to be crypto-probe.c, luks is no longer i= n >>> the filename >>> - Fixed formatting of parallels_probe() function header >>> - Enforced consistent naming convention for the probe functions. They= >>> now follow the pattern bdrv_format_probe(). >>> >>> Colin Lord (1): >>> blockdev: prepare iSCSI block driver for dynamic loading >>> >>> Marc Mari (2): >>> blockdev: Add dynamic generation of module_block.h >>> blockdev: Add dynamic module loading for block drivers >>> >>> Makefile | 7 +++ >>> block.c | 37 ++++++++++++--- >>> block/Makefile.objs | 3 +- >>> block/iscsi.c | 36 -------------- >>> include/qemu/module.h | 3 ++ >>> scripts/modules/module_block.py | 102 ++++++++++++++++++++++++++++++= ++++++++++ >>> util/module.c | 38 +++++---------- >>> vl.c | 38 +++++++++++++++ >>> 8 files changed, 193 insertions(+), 71 deletions(-) >>> create mode 100644 scripts/modules/module_block.py >>> >> >> >=20 --huCGiHOQo7UEEbipHcNW6cR9odL0M2pF4 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 iQEvBAEBCAAZBQJXlmrTEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrYjE B/9w1W6TwvnzEuixHXWPjLTYrMTbUDOsIWgQykqHTua3gMD3KuujRqc6O0HuAibB NPSSBg7101yR1Pj52eP7Cj+G6Wtlz12E8WBOoOvVKlMeaV1cGYmg20AaSaDOYKpv R0cnDi7aZ+BSybQulz8B/YasFFVk4m4o0bWMnP3K3/x50eLVMQpHcThh492mULXU A756ILLObsheOwkSlgW5kt9InyrAUkSgbs5E6wkgA/j/FDwbSN0LKc6J77Haoizf X2QGtKZQM8oqzs324YoJTzYlxM++1WTAUdUbKDlROAAckvQUgbmZl4Q5mKPIirP0 UxsKJSZtTddf6KHi4CbPDp7z =aFtQ -----END PGP SIGNATURE----- --huCGiHOQo7UEEbipHcNW6cR9odL0M2pF4--