qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 0/3] Dynamic module loading for block drivers


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v5 0/3] Dynamic module loading for block drivers
Date: Mon, 25 Jul 2016 21:38:59 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0

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 <address@hidden>

> 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
>>>
>>
>>
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]