Re: [Qemu-devel] [PATCH 22/34] block: Exclude nested options only for children in append_open_options()

[Max Reitz <mreitz@redhat.com>]

On 13.05.2015 14:49, Max Reitz wrote:
> On 08.05.2015 19:21, Kevin Wolf wrote:
>> Some drivers have nested options (e.g. blkdebug rule arrays), which
>> don't belong to a child node and shouldn't be removed. Don't remove all
>> options with "." in their name, but check for the complete prefixes of
>> actually existing child nodes.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block.c                   | 30 +++++++++++++++++++-----------
>>   include/block/block_int.h |  1 +
>>   2 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e329a47..e9a1d76 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -81,7 +81,7 @@ static QLIST_HEAD(, BlockDriver) bdrv_drivers =
>>     static int bdrv_open_inherit(BlockDriverState **pbs, const char 
>> *filename,
>>                                const char *reference, QDict *options, 
>> int flags,
>> -                             BlockDriverState* parent,
>> +                             BlockDriverState* parent, const char 
>> *child_name,
>>                                const BdrvChildRole *child_role,
>>                                BlockDriver *drv, Error **errp);
>>   @@ -1174,8 +1174,8 @@ int bdrv_open_backing_file(BlockDriverState 
>> *bs, QDict *parent_options,
>>       backing_hd = NULL;
>>       ret = bdrv_open_inherit(&backing_hd,
>>                               *backing_filename ? backing_filename : 
>> NULL,
>> -                            reference, options, 0, bs, &child_backing,
>> -                            NULL, &local_err);
>> +                            reference, options, 0, bs, "backing",
>> +                            &child_backing, NULL, &local_err);
>>       if (ret < 0) {
>>           bs->open_flags |= BDRV_O_NO_BACKING;
>>           error_setg(errp, "Could not open backing file: %s",
>> @@ -1238,7 +1238,7 @@ int bdrv_open_image(BlockDriverState **pbs, 
>> const char *filename,
>>       }
>>         ret = bdrv_open_inherit(pbs, filename, reference, 
>> image_options, 0,
>> -                            parent, child_role, NULL, errp);
>> +                            parent, bdref_key, child_role, NULL, errp);
>>     done:
>>       qdict_del(options, bdref_key);
>> @@ -1312,11 +1312,13 @@ out:
>>     static void bdrv_attach_child(BlockDriverState *parent_bs,
>>                                 BlockDriverState *child_bs,
>> +                              const char *child_name,
>>                                 const BdrvChildRole *child_role)
>>   {
>>       BdrvChild *child = g_new(BdrvChild, 1);
>>       *child = (BdrvChild) {
>>           .bs     = child_bs,
>> +        .name   = child_name,
>>           .role   = child_role,
>>       };
>>   @@ -1340,7 +1342,7 @@ static void 
>> bdrv_attach_child(BlockDriverState *parent_bs,
>>    */
>>   static int bdrv_open_inherit(BlockDriverState **pbs, const char 
>> *filename,
>>                                const char *reference, QDict *options, 
>> int flags,
>> -                             BlockDriverState* parent,
>> +                             BlockDriverState* parent, const char 
>> *child_name,
>>                                const BdrvChildRole *child_role,
>>                                BlockDriver *drv, Error **errp)
>>   {
>> @@ -1376,7 +1378,7 @@ static int bdrv_open_inherit(BlockDriverState 
>> **pbs, const char *filename,
>>           }
>>           bdrv_ref(bs);
>>           if (child_role) {
>> -            bdrv_attach_child(parent, bs, child_role);
>> +            bdrv_attach_child(parent, bs, child_name, child_role);
>>           }
>>           *pbs = bs;
>>           return 0;
>> @@ -1519,7 +1521,7 @@ static int bdrv_open_inherit(BlockDriverState 
>> **pbs, const char *filename,
>>       }
>>         if (child_role) {
>> -        bdrv_attach_child(parent, bs, child_role);
>> +        bdrv_attach_child(parent, bs, child_name, child_role);
>>       }
>>         QDECREF(options);
>> @@ -1563,7 +1565,7 @@ int bdrv_open(BlockDriverState **pbs, const 
>> char *filename,
>>                 BlockDriver *drv, Error **errp)
>>   {
>>       return bdrv_open_inherit(pbs, filename, reference, options, 
>> flags, NULL,
>> -                             NULL, drv, errp);
>> +                             NULL, NULL, drv, errp);
>>   }
>>     typedef struct BlockReopenQueueEntry {
>> @@ -2093,7 +2095,7 @@ void bdrv_append(BlockDriverState *bs_new, 
>> BlockDriverState *bs_top)
>>       /* The contents of 'tmp' will become bs_top, as we are
>>        * swapping bs_new and bs_top contents. */
>>       bdrv_set_backing_hd(bs_top, bs_new);
>> -    bdrv_attach_child(bs_top, bs_new, &child_backing);
>> +    bdrv_attach_child(bs_top, bs_new, "backing", &child_backing);
>>   }
>>     static void bdrv_delete(BlockDriverState *bs)
>> @@ -4037,13 +4039,19 @@ static bool append_open_options(QDict *d, 
>> BlockDriverState *bs)
>>   {
>>       const QDictEntry *entry;
>>       QemuOptDesc *desc;
>> +    BdrvChild *child;
>>       bool found_any = false;
>>         for (entry = qdict_first(bs->options); entry;
>>            entry = qdict_next(bs->options, entry))
>>       {
>> -        /* Only take options for this level */
>> -        if (strchr(qdict_entry_key(entry), '.')) {
>> +        /* Exclude options for children */
>> +        QLIST_FOREACH(child, &bs->children, next) {
>> +            if (strstart(qdict_entry_key(entry), child->name, NULL)) {
>
> I think the prefix should be "#{child->name}.", tested like "if 
> (strstart(..., &ptr) && *ptr == '.')".

Make that "(*ptr == '.' || !*ptr)", I think.

> It doesn't matter now, so I'm not sure whether I can give an R-b 
> anyway... I guess when in doubt, back out. So I won't. :-)
>
> Max
>
>> +                break;
>> +            }
>> +        }
>> +        if (child) {
>>               continue;
>>           }
>>   diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 2fad5f8..90da3f7 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -336,6 +336,7 @@ extern const BdrvChildRole child_format;
>>     typedef struct BdrvChild {
>>       BlockDriverState *bs;
>> +    const char *name;
>>       const BdrvChildRole *role;
>>       QLIST_ENTRY(BdrvChild) next;
>>   } BdrvChild;
>