On 08/10/2016 03:04 PM, 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.
>
> Colin
>
Oops, this was meant to be a reply to your response of patch 4/4, in
case anyone gets confused.
Colin
