On 08/11/2016 09:29 PM, Fam Zheng wrote:
> On Thu, 08/11 12:03, Colin Lord wrote:
>> On 08/10/2016 11:23 PM, Fam Zheng wrote:
>>> On Wed, 08/10 21:06, Max Reitz wrote:
>>>> 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 <mar...@redhat.com>
>>>>>>>
>>>>>>> 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 format
>>>>>>> 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 found
>>>>>>> in module_block.h
>>>>>>>
>>>>>>> Signed-off-by: Marc Marí <mar...@redhat.com>
>>>>>>> Signed-off-by: Colin Lord <cl...@redhat.com>
>>>>>>> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
>>>>>>> ---
>>>>>>> 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;
>>>>>>> }
>>>>>>>
>>>>>>> -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;
>>>>>>> }
>>>>>>>
>>>>>>> +BlockDriver *bdrv_find_format(const char *format_name)
>>>>>>> +{
>>>>>>> + BlockDriver *drv1;
>>>>>>> + size_t i;
>>>>>>> +
>>>>>>> + drv1 = bdrv_do_find_format(format_name);
>>>>>>> + if (drv1) {
>>>>>>> + return drv1;
>>>>>>> + }
>>>>>>> +
>>>>>>> + /* The driver isn't registered, maybe we need to load a module */
>>>>>>> + for (i = 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_name);
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> + return bdrv_do_find_format(format_name);
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> Did you reintroduce this function for dmg? I thought Fam is taking care
>>>>>> of that? I'm confused as to how Fam's patch for dmg and this series are
>>>>>> 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[] = {
>>>>>>
>>>>> 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).
>>>>>
>>>>> Essentially, if a user specifies the driver explicitly as part of their
>>>>> call to qemu, eg driver=gluster, 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=gluster on the command line, the
>>>>> gluster module would not be found. The modules could be found by probing
>>>>> 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_format
>>>>> gets called when the driver is specified on the command line. This makes
>>>>> 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 again.
>>>>
>>>> Ah, that makes sense, thanks for explaining.
>>>>
>>>> Patches 1-3:
>>>>
>>>> Reviewed-by: Max Reitz <mre...@redhat.com>
>>>>
>>>
>>> If we apply this series first, there will be an intermediate state that the
>>> main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better
>>> to
>>> make it clear, in case downstream backportings want to keep the library
>>> dependency intact.
>>>
>>> So, let's add a word to this commit message, at least.
>>>
>>> Fam
>>>
>>>
>>>
>> I guess I'm a little confused about the issue. It seems to me the only
>> difference between now and before is that if libbz2 is enabled, it will
>> be linked to the main binary rather than a module. But before, the
>> modules were always loaded unconditionally at startup anyway, so I'm not
>> seeing how there is a difference. Either way libbz2 would be loaded. I'm
>> just not really sure what sort of message I should be adding to the
>> commit message to indicate this.
>
> What I propose to be added in the commit message is this:
>
> --- 8< ---
>
> This spoils the purpose of 5505e8b76f (block/dmg: make it modular).
>
> Before this patch, if module build is enabled, block-dmg.so is linked to
> libbz2, whereas the main binary is not. In downstream, theoretically, it means
> only the qemu-block-extra package depends on libbz2, while the main QEMU
> package needn't to. With this patch, we (temporarily) change the case so that
> the main QEMU depends on libbz2 again.
>
> --- >8 ---
>
>>
>> Also, would you like me to try and port your patch for dmg to work on
>> top of this series? I'd still prefer if this series was applied first
>> because 1) if the dmg patch is done first, this series will have to
>> change parts of that patch anyway since the block module objects aren't
>> being loaded unconditionally anymore. That means the bz2 parts would
>> have to be converted to being dynamically linked at runtime, so it makes
>> sense to me to just do it that way the first time rather than going back
>> to change it. And 2) I am leaving soon and may or may not have time to
>> make this series work on top of the dmg patch. But I'm happy to try and
>> make your patch to work on top of this series in the little time I have
>> before I leave.
>
> I think it is fine for me to rebase on top of yours because: 1) practically
> it probably has no impact - Debian and ArchLinux both put block-dmg.so in the
> main QEMU package; 2) it's not a bug to introduce an extra dependency (the
> only
> special thing is, though, in this case it is not absolutely necessary, but it
> makes the development easier); 3) we will fix it within the same release.
>
> Fam
>
Okay sounds good, I'll add the comment and resend to the mailing list.
thanks,
Colin
