[Qemu-devel] [PATCH V3 0/7] qcow2: rollback the modification on fail in snapshot creation

[Qemu-devel] [PATCH V3 0/7] qcow2: rollback the modification on fail in snapshot creation

[Wenchao Xia]

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

[Qemu-devel] [PATCH V3 1/7] qcow2: restore nb_snapshots when fail in snapshot creation

[Wenchao Xia]

If it is not restored after qcow2_write_snapshots() fail, a core
dump will happen in bdrv_close() since access of invalid pointer.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e7e6013..40393b2 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -331,7 +331,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     QCowSnapshot *new_snapshot_list = NULL;
     QCowSnapshot *old_snapshot_list = NULL;
     QCowSnapshot sn1, *sn = &sn1;
-    int i, ret;
+    int i, ret, old_snapshot_num = 0;
     uint64_t *l1_table = NULL;
     int64_t l1_table_offset;
 
@@ -403,6 +403,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         memcpy(new_snapshot_list, s->snapshots,
                s->nb_snapshots * sizeof(QCowSnapshot));
         old_snapshot_list = s->snapshots;
+        old_snapshot_num = s->nb_snapshots;
     }
     s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
@@ -411,6 +412,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
+        s->nb_snapshots = old_snapshot_num;
         goto fail;
     }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 1/7] qcow2: restore nb_snapshots when fail in snapshot creation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

On 09/08/2013 08:57 PM, Wenchao Xia wrote:
> If it is not restored after qcow2_write_snapshots() fail, a core
> dump will happen in bdrv_close() since access of invalid pointer.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 1/7] qcow2: restore nb_snapshots when fail in snapshot creation

[Stefan Hajnoczi]

On Mon, Sep 09, 2013 at 10:57:56AM +0800, Wenchao Xia wrote:
> If it is not restored after qcow2_write_snapshots() fail, a core
> dump will happen in bdrv_close() since access of invalid pointer.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)

Good candidate for -stable.  Please add:

Cc: qemu-stable@nongnu.org

[Qemu-devel] [PATCH V3 2/7] qcow2: free allocated cluster on fail in qcow2_write_snapshots()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 40393b2..9e2d695 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -186,7 +186,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     }
     ret = bdrv_flush(bs);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
     /* The snapshot list position has not yet been updated, so these clusters
@@ -194,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, offset,
                                         s->snapshots_size);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
 
@@ -278,6 +278,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     return 0;
 
 fail:
+    /* free the new snapshot table */
+    qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
+                        QCOW2_DISCARD_ALWAYS);
     return ret;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 2/7] qcow2: free allocated cluster on fail in qcow2_write_snapshots()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On 09/08/2013 08:57 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 2/7] qcow2: free allocated cluster on fail in qcow2_write_snapshots()

[Stefan Hajnoczi]

On Mon, Sep 09, 2013 at 10:57:57AM +0800, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 40393b2..9e2d695 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -186,7 +186,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>      }
>      ret = bdrv_flush(bs);
>      if (ret < 0) {
> -        return ret;
> +        goto fail;
>      }
>  
>      /* The snapshot list position has not yet been updated, so these clusters
> @@ -194,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>      ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, offset,
>                                          s->snapshots_size);
>      if (ret < 0) {
> -        return ret;
> +        goto fail;
>      }
>  
>  
> @@ -278,6 +278,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>      return 0;
>  
>  fail:
> +    /* free the new snapshot table */
> +    qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
> +                        QCOW2_DISCARD_ALWAYS);

It is safer to skip qcow2_free_clusters() when the final
bdrv_pwrite_sync() fails.  We don't know if the header was updated on
disk.  If it was updated, then freeing the clusters may allow them to be
reallocated later (this will cause corruption).

[Qemu-devel] [PATCH V3 3/7] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 9e2d695..e0b7a5a 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -378,13 +378,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
             sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
-        goto fail;
+        goto dealloc_cluster;
     }
 
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
                       s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
-        goto fail;
+        goto dealloc_cluster;
     }
 
     g_free(l1_table);
@@ -397,7 +397,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
     if (ret < 0) {
-        goto fail;
+        goto dealloc_cluster;
     }
 
     /* Append the new snapshot to the snapshot list */
@@ -416,7 +416,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
         s->nb_snapshots = old_snapshot_num;
-        goto fail;
+        goto restore_refcount;
     }
 
     g_free(old_snapshot_list);
@@ -429,6 +429,18 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 #endif
     return 0;
 
+restore_refcount:
+    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1)
+        < 0) {
+        /* Nothing can be done none now, need image check later */
+        error_report("qcow2: Error in restoring refcount in snapshot");
+    }
+
+dealloc_cluster:
+    qcow2_free_clusters(bs, sn->l1_table_offset,
+                        sn->l1_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_ALWAYS);
+
 fail:
     g_free(sn->id_str);
     g_free(sn->name);
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 3/7] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On 09/08/2013 08:57 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 

>  
> +restore_refcount:
> +    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1)
> +        < 0) {
> +        /* Nothing can be done none now, need image check later */

s/none //

> +        error_report("qcow2: Error in restoring refcount in snapshot");
> +    }

Do we need to (attempt to) mark image metadata to mark the image
corrupted at this point?  Is it still wise to try and fall through to
freeing the clusters?

> +
> +dealloc_cluster:
> +    qcow2_free_clusters(bs, sn->l1_table_offset,
> +                        sn->l1_size * sizeof(uint64_t),
> +                        QCOW2_DISCARD_ALWAYS);
> +
>  fail:
>      g_free(sn->id_str);
>      g_free(sn->name);
> 

On the surface, this makes sense, but I'd rather defer the technical
review to someone more familiar with qcow2 code.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH V3 4/7] blkdebug: add debug events for snapshot

[Wenchao Xia]

Some code in qcow2-snapshot.c directly access bs->file, so in those
points error can't be injected by other events. Since the code in
qcow2-snapshot.c is qcow2's internal detail similar as L1 table,
so add some debug events.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/blkdebug.c      |    4 ++++
 include/block/block.h |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5d33e03..30eda44 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -186,6 +186,10 @@ static const char *event_names[BLKDBG_EVENT_MAX] = {
 
     [BLKDBG_FLUSH_TO_OS]                    = "flush_to_os",
     [BLKDBG_FLUSH_TO_DISK]                  = "flush_to_disk",
+
+    [BLKDBG_SNAPSHOT_L1_UPDATE]             = "snapshot_l1_update",
+    [BLKDBG_SNAPSHOT_LIST_UPDATE]           = "snapshot_list_update",
+    [BLKDBG_SNAPSHOT_HEADER_UPDATE]         = "snapshot_header_update",
 };
 
 static int get_event_by_name(const char *name, BlkDebugEvent *event)
diff --git a/include/block/block.h b/include/block/block.h
index e6b391c..e62098d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -432,6 +432,10 @@ typedef enum {
     BLKDBG_FLUSH_TO_OS,
     BLKDBG_FLUSH_TO_DISK,
 
+    BLKDBG_SNAPSHOT_L1_UPDATE,
+    BLKDBG_SNAPSHOT_LIST_UPDATE,
+    BLKDBG_SNAPSHOT_HEADER_UPDATE,
+
     BLKDBG_EVENT_MAX,
 } BlkDebugEvent;
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 4/7] blkdebug: add debug events for snapshot

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On 09/08/2013 08:57 PM, Wenchao Xia wrote:
> Some code in qcow2-snapshot.c directly access bs->file, so in those
> points error can't be injected by other events. Since the code in
> qcow2-snapshot.c is qcow2's internal detail similar as L1 table,
> so add some debug events.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/blkdebug.c      |    4 ++++
>  include/block/block.h |    4 ++++
>  2 files changed, 8 insertions(+), 0 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH V3 5/7] qcow2: use debug events for snapshot

[Wenchao Xia]

For those error paths which can't be triggered by other debug event,
use snapshot debug events.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e0b7a5a..92b87f8 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -198,6 +198,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     }
 
 
+    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_LIST_UPDATE);
     /* Write all snapshots to the new list */
     for(i = 0; i < s->nb_snapshots; i++) {
         sn = s->snapshots + i;
@@ -264,6 +265,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     header_data.nb_snapshots        = cpu_to_be32(s->nb_snapshots);
     header_data.snapshots_offset    = cpu_to_be64(snapshots_offset);
 
+    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_HEADER_UPDATE);
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
                            &header_data, sizeof(header_data));
     if (ret < 0) {
@@ -375,6 +377,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         l1_table[i] = cpu_to_be64(s->l1_table[i]);
     }
 
+    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_L1_UPDATE);
     ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
             sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 5/7] qcow2: use debug events for snapshot

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On 09/08/2013 08:58 PM, Wenchao Xia wrote:
> For those error paths which can't be triggered by other debug event,
> use snapshot debug events.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

I would have squashed 4 and 5 into a single patch (declaring the enums
doesn't add any functionality without the event actually using the enum
values, and neither patch is so large in isolation where combining them
would ips the balance towards too large).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH V3 6/7] qcow2: print message for error path in snapshot creation

[Wenchao Xia]

The message will be print out with a macro enabled, which can
be used to check which error path is taken.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 92b87f8..7f24486 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -26,6 +26,8 @@
 #include "block/block_int.h"
 #include "block/qcow2.h"
 
+/* #define QCOW2_SNAPSHOT_PRINT_ERROR_PATH */
+
 typedef struct QEMU_PACKED QCowSnapshotHeader {
     /* header is 8 byte aligned */
     uint64_t l1_table_offset;
@@ -182,10 +184,16 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
     offset = snapshots_offset;
     if (offset < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in allocation of snapshot list\n");
+#endif
         return offset;
     }
     ret = bdrv_flush(bs);
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in flush after snapshot list allocation\n");
+#endif
         goto fail;
     }
 
@@ -194,6 +202,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, offset,
                                         s->snapshots_size);
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in overlap check before update snapshot list\n");
+#endif
         goto fail;
     }
 
@@ -227,24 +238,36 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
         ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
         if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+            printf("qcow2: Failed in update of snapshot header\n");
+#endif
             goto fail;
         }
         offset += sizeof(h);
 
         ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra));
         if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+            printf("qcow2: Failed in update of extra snapshot info\n");
+#endif
             goto fail;
         }
         offset += sizeof(extra);
 
         ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
         if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+            printf("qcow2: Failed in update of snapshot id\n");
+#endif
             goto fail;
         }
         offset += id_str_size;
 
         ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
         if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+            printf("qcow2: Failed in update of snapshot name\n");
+#endif
             goto fail;
         }
         offset += name_size;
@@ -256,6 +279,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
      */
     ret = bdrv_flush(bs);
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in flush after update snapshot list\n");
+#endif
         goto fail;
     }
 
@@ -269,6 +295,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
                            &header_data, sizeof(header_data));
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in update qcow2 header in snapshot creation\n");
+#endif
         goto fail;
     }
 
@@ -366,6 +395,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
     if (l1_table_offset < 0) {
         ret = l1_table_offset;
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in allocation of L1 table for snapshot\n");
+#endif
         goto fail;
     }
 
@@ -381,12 +413,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
             sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in overlap check before update L1 table for "
+               "snapshot\n");
+#endif
         goto dealloc_cluster;
     }
 
+    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_L1_UPDATE);
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
                       s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in update of L1 table for snapshot\n");
+#endif
         goto dealloc_cluster;
     }
 
@@ -400,6 +440,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
     if (ret < 0) {
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in update refcount for snapshot\n");
+#endif
         goto dealloc_cluster;
     }
 
@@ -419,6 +462,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
         s->nb_snapshots = old_snapshot_num;
+#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
+        printf("qcow2: Failed in update of snapshot list and header\n");
+#endif
         goto restore_refcount;
     }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 6/7] qcow2: print message for error path in snapshot creation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

On 09/08/2013 08:58 PM, Wenchao Xia wrote:
> The message will be print out with a macro enabled, which can

s/print/printed/

> be used to check which error path is taken.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 46 insertions(+), 0 deletions(-)
> 

> @@ -381,12 +413,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
>              sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
>      if (ret < 0) {
> +#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
> +        printf("qcow2: Failed in overlap check before update L1 table for "
> +               "snapshot\n");
> +#endif
>          goto dealloc_cluster;
>      }
>  
> +    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_L1_UPDATE);

Should this BLKDBG be part of patch 5?

In general, the move to avoid fprintf except under recompilation seems
okay; but it seems odd to be removing the diagnosis altogether.  If you
had gone one step further and refactored the code to wire in Error*
support, then you could change fprintf to passing the Error back up the
stack to the caller rather than losing it except during a debug build.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 6/7] qcow2: print message for error path in snapshot creation

[Stefan Hajnoczi]

On Mon, Sep 30, 2013 at 04:08:53PM -0600, Eric Blake wrote:
> On 09/08/2013 08:58 PM, Wenchao Xia wrote:
> > The message will be print out with a macro enabled, which can
> 
> s/print/printed/
> 
> > be used to check which error path is taken.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > ---
> >  block/qcow2-snapshot.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 46 insertions(+), 0 deletions(-)
> > 
> 
> > @@ -381,12 +413,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> >      ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> >              sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
> >      if (ret < 0) {
> > +#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
> > +        printf("qcow2: Failed in overlap check before update L1 table for "
> > +               "snapshot\n");
> > +#endif
> >          goto dealloc_cluster;
> >      }
> >  
> > +    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_L1_UPDATE);
> 
> Should this BLKDBG be part of patch 5?
> 
> In general, the move to avoid fprintf except under recompilation seems
> okay; but it seems odd to be removing the diagnosis altogether.  If you
> had gone one step further and refactored the code to wire in Error*
> support, then you could change fprintf to passing the Error back up the
> stack to the caller rather than losing it except during a debug build.

I agree with Eric.  Use Error* and make snapshot commands print a
detailed error to the monitor.

When diagnostics are compiled out we can't help troubleshoot user
problems.

Stefan

Re: [Qemu-devel] [PATCH V3 6/7] qcow2: print message for error path in snapshot creation

[Wenchao Xia]

于 2013/10/2 20:23, Stefan Hajnoczi 写道:
> On Mon, Sep 30, 2013 at 04:08:53PM -0600, Eric Blake wrote:
>> On 09/08/2013 08:58 PM, Wenchao Xia wrote:
>>> The message will be print out with a macro enabled, which can
>> s/print/printed/
>>
>>> be used to check which error path is taken.
>>>
>>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   block/qcow2-snapshot.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 46 insertions(+), 0 deletions(-)
>>>
>>> @@ -381,12 +413,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>>       ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
>>>               sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
>>>       if (ret<  0) {
>>> +#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
>>> +        printf("qcow2: Failed in overlap check before update L1 table for "
>>> +               "snapshot\n");
>>> +#endif
>>>           goto dealloc_cluster;
>>>       }
>>>
>>> +    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_L1_UPDATE);
>> Should this BLKDBG be part of patch 5?
>>
>> In general, the move to avoid fprintf except under recompilation seems
>> okay; but it seems odd to be removing the diagnosis altogether.  If you
>> had gone one step further and refactored the code to wire in Error*
>> support, then you could change fprintf to passing the Error back up the
>> stack to the caller rather than losing it except during a debug build.
> I agree with Eric.  Use Error* and make snapshot commands print a
> detailed error to the monitor.
>
> When diagnostics are compiled out we can't help troubleshoot user
> problems.
>
> Stefan
>
Make sense, will add *errp to report the errors.

[Qemu-devel] [PATCH V3 7/7] qemu-iotests: add test for qcow2 snapshot

[Wenchao Xia]

This test will focus on the low level procedure of qcow2 snapshot
operations, now it covers only the create operation. Overlap error
paths are not checked since no good way to trigger those errors.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qemu-iotests/063     |  229 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/063.out |   37 +++++++
 tests/qemu-iotests/group   |    1 +
 3 files changed, 267 insertions(+), 0 deletions(-)
 create mode 100755 tests/qemu-iotests/063
 create mode 100644 tests/qemu-iotests/063.out

diff --git a/tests/qemu-iotests/063 b/tests/qemu-iotests/063
new file mode 100755
index 0000000..f3f6e16
--- /dev/null
+++ b/tests/qemu-iotests/063
@@ -0,0 +1,229 @@
+#!/bin/bash
+#
+# qcow2 internal snapshot test
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+owner=xiawenc@linux.vnet.ibm.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm $TEST_DIR/blkdebug.conf
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# only test qcow2
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+IMGOPTS="compat=1.1"
+
+CLUSTER_SIZE=65536
+
+SIZE=1G
+
+BLKDBG_TEST_IMG="blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG"
+
+errno=5
+
+once=on
+
+imm=off
+
+
+# Start test, note that the injected errors are related to qcow2's snapshot
+# logic closely, see qcow2-snapshot.c for more details.
+
+# path 1: fail in L1 table allocation for snapshot
+echo
+echo "Path 1: fail in allocation of L1 table"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "cluster_alloc"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+
+# path 2: fail in update new L1 table
+echo
+echo "Path 2: fail in overlap check before update L1 table for snapshot"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "snapshot_l1_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+# path 3: fail in update refcount block before write snapshot list
+echo
+echo "Path 3: fail in update refcount block before write snapshot list"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "refblock_alloc"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+# path 4: fail in snapshot list allocation, it is possible
+# qcow2_alloc_clusters() not fail immediately since cache hit, but in any
+# case, no error should be found in image check.
+echo
+echo "Path 4: fail in snapshot list allocation"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[set-state]
+state = "1"
+event = "cluster_alloc"
+new_state = "2"
+
+[inject-error]
+state = "2"
+event = "cluster_alloc"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+# path 5: fail in flush after snapshot list allocation, flush is related with
+# cache status, if no cache available then the flush may not trigger a error
+# in bdrv_flush(), but no error should be found in image check.
+echo
+echo "Path 5: fail in flush after snapshot list allocation"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[set-state]
+state = "1"
+event = "cluster_alloc"
+new_state = "2"
+
+[set-state]
+state = "2"
+event = "cluster_alloc"
+new_state = "3"
+
+[inject-error]
+state = "3"
+event = "flush_to_os"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+
+[set-state]
+state = "3"
+event = "flush_to_os"
+new_state = "4"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+# path 6: fail in snapshot list update
+echo
+echo "Path 6: fail in snapshot list update"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "snapshot_list_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+# path 7: fail in flush after snapshot list update, no good way to trigger it,
+# since the cache is empty and makes flush do nothing in that call, so leave
+# this path not tested
+
+# path 8: fail in update qcow2 header
+echo
+echo "Path 8: fail in update qcow2 header"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "snapshot_header_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+# path 9: fail in overlap check before update L1 table for snapshot
+# path 10: fail in overlap check before update snapshot list
+# Since those clusters are allocated at runtime, there is no good way to
+# make them overlap in this script, so skip those two paths now.
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/063.out b/tests/qemu-iotests/063.out
new file mode 100644
index 0000000..c387bfa
--- /dev/null
+++ b/tests/qemu-iotests/063.out
@@ -0,0 +1,37 @@
+QA output created by 063
+
+Path 1: fail in allocation of L1 table
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+
+Path 2: fail in overlap check before update L1 table for snapshot
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+
+Path 3: fail in update refcount block before write snapshot list
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+
+Path 4: fail in snapshot list allocation
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+
+Path 5: fail in flush after snapshot list allocation
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+
+Path 6: fail in snapshot list update
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+
+Path 8: fail in update qcow2 header
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': -5 (Input/output error)
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b696242..316b1dd 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -66,3 +66,4 @@
 059 rw auto
 060 rw auto
 062 rw auto
+063 rw auto
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 7/7] qemu-iotests: add test for qcow2 snapshot

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

On 09/08/2013 08:58 PM, Wenchao Xia wrote:
> This test will focus on the low level procedure of qcow2 snapshot
> operations, now it covers only the create operation. Overlap error
> paths are not checked since no good way to trigger those errors.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/063     |  229 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/063.out |   37 +++++++
>  tests/qemu-iotests/group   |    1 +
>  3 files changed, 267 insertions(+), 0 deletions(-)
>  create mode 100755 tests/qemu-iotests/063
>  create mode 100644 tests/qemu-iotests/063.out
> 

> +# only test qcow2
> +_supported_fmt qcow2
> +_supported_proto generic
> +_supported_os Linux
> +
> +IMGOPTS="compat=1.1"
> +
> +CLUSTER_SIZE=65536
> +
> +SIZE=1G
> +
> +BLKDBG_TEST_IMG="blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG"
> +
> +errno=5

Not all platforms have errno 5 tied to EIO; but then again, you filtered
this test to run only on Linux.  Is it possible to be a bit more
generic, though?

At any rate, more tests are always good.  I didn't read very closely;
but I also didn't see anything obviously wrong with the patch, and it is
self-validating whether the testsuite still passes after applying it.
So feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 7/7] qemu-iotests: add test for qcow2 snapshot

[Wenchao Xia]

于 2013/10/1 6:28, Eric Blake 写道:
> On 09/08/2013 08:58 PM, Wenchao Xia wrote:
>> This test will focus on the low level procedure of qcow2 snapshot
>> operations, now it covers only the create operation. Overlap error
>> paths are not checked since no good way to trigger those errors.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   tests/qemu-iotests/063     |  229 ++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/063.out |   37 +++++++
>>   tests/qemu-iotests/group   |    1 +
>>   3 files changed, 267 insertions(+), 0 deletions(-)
>>   create mode 100755 tests/qemu-iotests/063
>>   create mode 100644 tests/qemu-iotests/063.out
>>
>> +# only test qcow2
>> +_supported_fmt qcow2
>> +_supported_proto generic
>> +_supported_os Linux
>> +
>> +IMGOPTS="compat=1.1"
>> +
>> +CLUSTER_SIZE=65536
>> +
>> +SIZE=1G
>> +
>> +BLKDBG_TEST_IMG="blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG"
>> +
>> +errno=5
> Not all platforms have errno 5 tied to EIO; but then again, you filtered
> this test to run only on Linux.  Is it possible to be a bit more
> generic, though?
>
   I think the test can be made more generic, but it is a bit hard for 
me to find out
what number should be used on all platform now. Instead, I'd like to add 
a comments
here:"bind the errno correctly if you want run this case on other platform".

> At any rate, more tests are always good.  I didn't read very closely;
> but I also didn't see anything obviously wrong with the patch, and it is
> self-validating whether the testsuite still passes after applying it.
> So feel free to add:
>
> Reviewed-by: Eric Blake<eblake@redhat.com>
>

Re: [Qemu-devel] [PATCH V3 0/7] qcow2: rollback the modification on fail in snapshot creation

[Wenchao Xia]

于 2013/9/9 10:57, Wenchao Xia 写道:
> 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
>
Hello, any comments for it?

Re: [Qemu-devel] [PATCH V3 0/7] qcow2: rollback the modification on fail in snapshot creation

[Stefan Hajnoczi]

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.

Re: [Qemu-devel] [PATCH V3 0/7] qcow2: rollback the modification on fail in snapshot creation

[Wenchao Xia]

于 2013/10/2 20:28, Stefan Hajnoczi 写道:
> 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.
>
Thanks for review. Since Max have already added some error path code in 
upstream,
I rebased and skip cluster freeing when fail in image header update.