From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38537)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VRMRW-0006Om-7l
	for qemu-devel@nongnu.org; Wed, 02 Oct 2013 09:26:26 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VRMRJ-00060W-B0
	for qemu-devel@nongnu.org; Wed, 02 Oct 2013 09:26:17 -0400
Received: from mail-we0-x22d.google.com ([2a00:1450:400c:c03::22d]:53376)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VRMRJ-000606-46
	for qemu-devel@nongnu.org; Wed, 02 Oct 2013 09:26:05 -0400
Received: by mail-we0-f173.google.com with SMTP id u57so84898wes.32
	for <qemu-devel@nongnu.org>; Wed, 02 Oct 2013 06:26:04 -0700 (PDT)
Date: Wed, 2 Oct 2013 14:28:48 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20131002122848.GD12200@stefanha-thinkpad.redhat.com>
References: <1378695482-29805-1-git-send-email-xiawenc@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1378695482-29805-1-git-send-email-xiawenc@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH V3 0/7] qcow2: rollback the modification on
 fail in snapshot creation
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com

On Mon, Sep 09, 2013 at 10:57:55AM +0800, Wenchao Xia wrote:
> V2:
>   1: all fail case will goto fail section.
>   2: add the goto code.
> v3:
>   Address Stefan's comments:
>   2: don't goto fail after allocation failure.
>   3: use sn->l1size correctly in qcow2_free_cluster().
>   4-7: add test case to verify the error paths.
>   Other:
>   1: new patch fix a existing bug, which will be exposed in error path test.
> 
> 
> Wenchao Xia (7):
>   1 qcow2: restore nb_snapshots when fail in snapshot creation
>   2 qcow2: free allocated cluster on fail in qcow2_write_snapshots()
>   3 qcow2: cancel the modification on fail in qcow2_snapshot_create()
>   4 blkdebug: add debug events for snapshot
>   5 qcow2: use debug events for snapshot
>   6 qcow2: print message for error path in snapshot creation
>   7 qemu-iotests: add test for qcow2 snapshot
> 
>  block/blkdebug.c           |    4 +
>  block/qcow2-snapshot.c     |   80 ++++++++++++++--
>  include/block/block.h      |    4 +
>  tests/qemu-iotests/063     |  229 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/063.out |   37 +++++++
>  tests/qemu-iotests/group   |    1 +
>  6 files changed, 348 insertions(+), 7 deletions(-)
>  create mode 100755 tests/qemu-iotests/063
>  create mode 100644 tests/qemu-iotests/063.out

Makes sense but keep in mind that it's better to leak clusters than to
corrupt the image further.  We have to be very careful in these error
paths, especially when a big operation could have failed partway
through.  I left comments where I thought the patches need to be
changed.