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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 27.11.2015 um 18:58 hat Max Reitz geschrieben:
> 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.

Are you sure? As far as I can tell, this patch should only keep options
that were previously removed, but not remove options that were
previously kept (with the exception of direct use of child names, which
I added here to address your review comments for v1).

Specifically for "extents.%d", this is a child name and is therefore
omitted. However, it contains a '.', so it was already removed without
this patch.

I'm accepting proof of the contrary in the form of a test case. ;-)

> 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).

Yes, I agree.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]