On 08/10/2016 03:24 PM, Colin Lord wrote:
> 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
>
Nevermind, looks like I was confused. Seems like my eyes are playing
tricks on me, sorry for the spam.
