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 are >>> 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 dmg >> 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 request? >> > 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 <mre...@redhat.com> > 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 250): > > block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst > /,-,$o))",) NULL > util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)' > > 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 gets > 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 > > Colin > >> Max >> >>> - Modify module_block.py to only include the library name and protocol >>> 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 block/ >>> 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 in >>> 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 >>> >> >> >
signature.asc
Description: OpenPGP digital signature
