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

["Denis V. Lunev" <den@openvz.org>]

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 <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   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