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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4384 bytes --]

On 23.11.2015 16:59, 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(-)

Thanks, now I don't need to fix it myself. :-)

(I would have had to do that for an in-work series of mine)

> 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,
>          .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;
>          }
>  

A good general solution, but I think bdrv_refresh_filename() may be bad
enough to break with general solutions. ;-)

bdrv_refresh_filename() only considers "file" and "backing" (actually,
it only supports "file" for now, I'm working on "backing", though). The
only drivers with other children are quorum, blkdebug, blkverify and
VMDK. The former three have their own implementation of
bdrv_refresh_filename(), so they don't use append_open_options() at all.
The latter, however, (VMDK) does not.

This change to append_open_options results in the extent.%d options
simply being omitted altogether because bdrv_refresh_filename() does not
fetch them. Before, they were included in the VMDK BDS's options, which
is not ideal but works more or less.

In order to "fix" this, I see three ways right now:
1. Just care about "file" and "backing". bdrv_refresh_filename()
   doesn't support anything else, so that will be fine.
2. Implement bdrv_refresh_filename() specifically for VMDK so
   append_open_options() will never have to handle anything but "file"
   and "backing".
3. Fix bdrv_refresh_filename() so that it handles all children and not
   just "file" and "backing".

Since we are shooting for 2.6 anyway (I assume ;-)), I think we should
go for option 3. This means that this patch is fine, and I'll see to
fixing bdrv_refresh_filename() (because I'm working on that anyway).

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]