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

[Max Reitz <mreitz@redhat.com>]

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 == '.')".

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;