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

[Wen Congyang <wency@cn.fujitsu.com>]

On 11/23/2015 11:59 PM, 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                   | 19 +++++++++++++++----
>  include/block/block_int.h |  1 +
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 23d9e10..02125e2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename,
>  
>  static BdrvChild *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,

The child_name may be allocated in the caller's stack. For example:
In the function quorum_open():
    for (i = 0; i < s->num_children; i++) {
        char indexstr[32];
        ret = snprintf(indexstr, 32, "children.%d", i);
        assert(ret < 32);

        s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
                                         &child_format, false, &local_err);
        if (local_err) {
            ret = -EINVAL;
            goto close_exit;
        }

        opened[i] = true;
    }

Thanks
Wen Congyang

>          .role   = child_role,
>      };
>  
> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>          bs->backing = NULL;
>          goto out;
>      }
> -    bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing);
> +    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing);
>      bs->open_flags &= ~BDRV_O_NO_BACKING;
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
>      pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename,
>          goto done;
>      }
>  
> -    c = bdrv_attach_child(parent, bs, child_role);
> +    c = bdrv_attach_child(parent, bs, bdref_key, child_role);
>  
>  done:
>      qdict_del(options, bdref_key);
> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
>  {
>      const QDictEntry *entry;
>      QemuOptDesc *desc;
> +    BdrvChild *child;
>      bool found_any = false;
> +    const char *p;
>  
>      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, &p)
> +                && (!*p || *p == '.'))
> +            {
> +                break;
> +            }
> +        }
> +        if (child) {
>              continue;
>          }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 77dc165..b2325aa 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -351,6 +351,7 @@ extern const BdrvChildRole child_format;
>  
>  struct BdrvChild {
>      BlockDriverState *bs;
> +    const char *name;
>      const BdrvChildRole *role;
>      QLIST_ENTRY(BdrvChild) next;
>      QLIST_ENTRY(BdrvChild) next_parent;
> 


-- 
This message has been scanned for viruses and
dangerous content by FCNIC, and is
believed to be clean.