Re: [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper

[Denis V. Lunev]

On 11/06/2015 05:09 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:19:34PM +0300, Denis V. Lunev wrote:
> > to delete snapshots from all loaded block drivers.
> >
> > The patch also ensures proper locking.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: Juan Quintela <address@hidden>
> > CC: Stefan Hajnoczi <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> > ---
> >   block/snapshot.c         | 22 ++++++++++++++++++++
> >   include/block/snapshot.h |  2 ++
> >   migration/savevm.c       | 54 +++++++++---------------------------------------
> >   3 files changed, 34 insertions(+), 44 deletions(-)
> >
> > diff --git a/block/snapshot.c b/block/snapshot.c
> > index d729c76..1b4b846 100644
> > --- a/block/snapshot.c
> > +++ b/block/snapshot.c
> > @@ -384,3 +384,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
> >       *first_bad_bs = bs;
> >       return ok;
> >   }
> > +
> > +int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
> > +                             Error **err)
> > +{
> > +    int ret = 0;
> > +    BlockDriverState *bs = NULL;
> > +    QEMUSnapshotInfo sn1, *snapshot = &sn1;
> > +
> > +    while ((bs = bdrv_next(bs)) && ret == 0) {
> If ret != 0 we will iterate to the next bs.  first_bad_bs will be
> incorrect.
>
> > +        AioContext *ctx = bdrv_get_aio_context(bs);
> > +
> > +        aio_context_acquire(ctx);
> > +        if (bdrv_can_snapshot(bs) &&
> > +                bdrv_snapshot_find(bs, snapshot, name) >= 0) {
> > +            ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
> > +        }
> > +        aio_context_release(ctx);
> > +    }
> > +
> > +    *first_bad_bs = bs;
> > +    return ret;
> > +}
> Similar approach without the int return value:
>
> bool bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
>                                Error **err)
> {
>      Error *local_err = NULL;
>      BlockDriverState *bs = NULL;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
>
>      while ((bs = bdrv_next(bs)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
>
>         aio_context_acquire(bs);
>         if (bdrv_can_snapshot(bs) &&
>             bdrv_snapshot_find(bs, snapshot, name) >= 0) {
>             bdrv_snapshot_delete_by_id_or_name(bs, name, &local_err);
>         }
>         aio_context_release(bs);
>
>         if (local_err) {
>             error_propagate(err, local_err);
>             return false;
>         }
>      }
>      return true;
> }
there are 2 notes.

Personally I do not like 'bool' functions as it is unclear
whether true means success or failure. Usually false is
failure but I have faced awkward situation when conditions
where reversed.

<0 and >=0 conditions are alive for 10th of years.

secondly the you will have to assign first_bad_bs two times.
I have started from this version of code and merged them
later to the current approach. Ok, I have made a mistake :(

Den