From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39188) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zv2Vn-00078X-1T for qemu-devel@nongnu.org; Sat, 07 Nov 2015 07:22:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zv2Vj-0002bu-SO for qemu-devel@nongnu.org; Sat, 07 Nov 2015 07:22:27 -0500 Received: from relay.parallels.com ([195.214.232.42]:60596) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zv2Vj-0002bX-Lq for qemu-devel@nongnu.org; Sat, 07 Nov 2015 07:22:23 -0500 References: <1446657582-21619-1-git-send-email-den@openvz.org> <1446657582-21619-4-git-send-email-den@openvz.org> <20151106140908.GN12285@stefanha-x1.localdomain> From: "Denis V. Lunev" Message-ID: <563DECEA.40409@openvz.org> Date: Sat, 7 Nov 2015 15:22:02 +0300 MIME-Version: 1.0 In-Reply-To: <20151106140908.GN12285@stefanha-x1.localdomain> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Juan Quintela 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 >> CC: Juan Quintela >> CC: Stefan Hajnoczi >> CC: Kevin Wolf >> --- >> 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