Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Kevin Wolf <kwolf@redhat.com>]

Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > From: Benoît Canet <benoit@irqsave.net>
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c                   | 65 ++++++++++++++++++++++++++++++++++++++++-------
> >  block/blkverify.c         |  2 +-
> >  blockdev.c                |  2 +-
> >  include/block/block.h     | 20 +++++++--------
> >  include/block/block_int.h | 12 ++++++---
> >  5 files changed, 77 insertions(+), 24 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index e1bc732..3e0994b 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5088,21 +5088,68 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> >      return bs->drv->bdrv_amend_options(bs, options);
> >  }
> >  
> > -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs)
> > +/* Used to recurse on single child block filters.
> > + * Single child block filter will store their child in bs->file.
> > + */
> > +bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
> > +                                      BlockDriverState *candidate)
> >  {
> > -    if (bs->drv->bdrv_check_ext_snapshot) {
> > -        return bs->drv->bdrv_check_ext_snapshot(bs);
> > +    if (!bs->drv) {
> > +        return false;
> > +    }
> > +
> > +    if (!bs->drv->authorizations[BS_IS_A_FILTER]) {
> > +        if (bs == candidate) {
> > +            return true;
> > +        } else {
> > +            return false;
> > +        }
> 
> This seems to break external snapshots; after this patch, I can no
> longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> doing something incorrectly.
> 
> Instead of checking for bs == candidate, was it intended to check to
> see if !strcmp(bs->filename, candidiate->filename) was true?

If believe the problem is in bdrv_is_first_non_filter(): It starts with
bs->file, whereas the first non-filter is obviously the top-level BDS
itself. I suspect the following patch fixes it (it makes the simple
snapshotting case work again, but I'm not sure if it forbids everything
that should be forbidden).

In any case, this shows that...

- ...our testing is still lacking (no qemu-iotests case for live
  snapshots? Seriously? Expect it to be broken then.)

- ...not all patch authors do a good share of manual testing

- ...I have relaxed my reviewing too much. I wasn't convinced that this
  patch is right, because the whole logic confused me, but I couldn't
  point to a bug. I shouldn't have merged it when in doubt.

Jeff, would you like to submit a qemu-iotests case for snapshotting?

Benoît, can you check whether the patch below is correct?

Kevin


diff --git a/block.c b/block.c
index ac0ccac..1299484 100644
--- a/block.c
+++ b/block.c
@@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
     QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
         bool perm;
 
-        if (!bs->file) {
-            continue;
-        }
-
-        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
+        perm = bdrv_recurse_is_first_non_filter(bs, candidate);
 
         /* candidate is the first non filter */
         if (perm) {