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

[Qemu-devel] [PATCH V7 0/6] 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.

v4:
  General change:
  rebased on upstream since error path for qcow2_write_snapshots() already
exist in upstream. removed old patch 1 since it is fixed by Max in upstream.
  5: moved the snapshot_l1_update event just before write operation, instead of
before overlap check, since it is more straight.
  6: remove a duplicated error path test about flush after snapshot list
update, add a filter which replace number to X, since now in error in report
detailed message including error cluster number.
  Address Stefan's comments:
  1, 2, 4: add *errp to store detailed error message, instead of error_report()
and compile time determined debug printf message.
  3: do not free cluster when fail in header update for safety reason.
  Address Eric's comments:
  1, 2, 4: add *errp to store detailed error message, instead of error_report()
and compile time determined debug printf message.
  5: squashed patches that add and use debug events.
  6: added comments about test only on Linux.

v5:
  General change:
  6: rebased on upstream, use case number 070, adjust 070.out due to error
message change in this version.

  Address Max's comments:
  1 use error_setg_errno() when possible, remove "ret =" in functions when
possible since the function does not need to return int value, fix 32bit/
64bit issue in printf for "sizeof" and "offse", typo fix.
  2 use error_setg_errno() when possible, fix 32bit/64bit issue in printf
for "sizeof" and "offse", typo fix.
  3 typo fix in comments.
  5 typo fix in commit message.

  Address Eric's comments:
  2 fix 32bit/64bit issue in printf for "sizeof" and "offse".

v6:
  Address Jeff's comments:
  6: add quote for image name in test case.

v7:
  Rebased on Stefan's block tree, since I need to test after Fam's
cache mode series.
  6: change case number to 075 to avoid conflict, add a comments in
case that it covers only default cache mode, qemu-img snapshot would
not be affected by case's cache setting.

Wenchao Xia (6):
  1 snapshot: add parameter *errp in snapshot create
  2 qcow2: add error message in qcow2_write_snapshots()
  3 qcow2: do not free clusters when fail in header update in qcow2_write_snapshots
  4 qcow2: cancel the modification on fail in qcow2_snapshot_create()
  5 blkdebug: add debug events for snapshot
  6 qemu-iotests: add test for qcow2 snapshot

 block/blkdebug.c                 |    4 +
 block/qcow2-snapshot.c           |  105 ++++++++++++++++---
 block/qcow2.h                    |    4 +-
 block/rbd.c                      |   19 ++--
 block/sheepdog.c                 |   28 +++--
 block/snapshot.c                 |   19 +++-
 blockdev.c                       |   10 +-
 include/block/block.h            |    4 +
 include/block/block_int.h        |    5 +-
 include/block/snapshot.h         |    5 +-
 qemu-img.c                       |   10 +-
 savevm.c                         |   12 ++-
 tests/qemu-iotests/075           |  218 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/075.out       |   35 ++++++
 tests/qemu-iotests/common.filter |    7 ++
 tests/qemu-iotests/group         |    1 +
 16 files changed, 429 insertions(+), 57 deletions(-)
 create mode 100755 tests/qemu-iotests/075
 create mode 100644 tests/qemu-iotests/075.out

[Qemu-devel] [PATCH V7 1/6] snapshot: add parameter *errp in snapshot create

[Wenchao Xia]

The return value is only used for error report before this patch,
so change the function protype to return void.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c    |   30 +++++++++++++++++++++++++-----
 block/qcow2.h             |    4 +++-
 block/rbd.c               |   19 ++++++++++---------
 block/sheepdog.c          |   28 ++++++++++++++++++----------
 block/snapshot.c          |   19 +++++++++++++------
 blockdev.c                |   10 ++++------
 include/block/block_int.h |    5 +++--
 include/block/snapshot.h  |    5 +++--
 qemu-img.c                |   10 ++++++----
 savevm.c                  |   12 ++++++++----
 10 files changed, 93 insertions(+), 49 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index ad8bf3d..670a58d 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -347,7 +347,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs,
 }
 
 /* if no id is provided, a new one is constructed */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+void qcow2_snapshot_create(BlockDriverState *bs,
+                           QEMUSnapshotInfo *sn_info,
+                           Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *new_snapshot_list = NULL;
@@ -366,7 +368,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     /* Check that the ID is unique */
     if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
-        return -EEXIST;
+        error_setg(errp, "Snapshot with id %s already exist", sn_info->id_str);
+        return;
     }
 
     /* Populate sn with passed data */
@@ -382,7 +385,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     /* Allocate the L1 table of the snapshot and copy the current one there. */
     l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
     if (l1_table_offset < 0) {
-        ret = l1_table_offset;
+        error_setg_errno(errp, -l1_table_offset,
+                         "Failed in allocation of snapshot L1 table");
         goto fail;
     }
 
@@ -397,12 +401,22 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     ret = qcow2_pre_write_overlap_check(bs, 0, sn->l1_table_offset,
                                         s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in overlap check for snapshot L1 table at %"
+                         PRIu64 " with size %" PRIu64,
+                         sn->l1_table_offset,
+                         (uint64_t)(s->l1_size * sizeof(uint64_t)));
         goto fail;
     }
 
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
                       s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in update of snapshot L1 table at %"
+                         PRIu64 " with size %" PRIu64,
+                         sn->l1_table_offset,
+                         (uint64_t)(s->l1_size * sizeof(uint64_t)));
         goto fail;
     }
 
@@ -416,6 +430,10 @@ 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) {
+        error_setg_errno(errp, -ret,
+                         "Failed in update of refcount for snapshot at %"
+                         PRIu64 " with size %d",
+                         s->l1_table_offset, s->l1_size);
         goto fail;
     }
 
@@ -431,6 +449,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     ret = qcow2_write_snapshots(bs);
     if (ret < 0) {
+        /* Following line will be replaced with more detailed error later */
+        error_setg(errp, "Failed in write of snapshot");
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
         s->nb_snapshots--;
@@ -452,14 +472,14 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
       qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
-    return 0;
+    return;
 
 fail:
     g_free(sn->id_str);
     g_free(sn->name);
     g_free(l1_table);
 
-    return ret;
+    return;
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
diff --git a/block/qcow2.h b/block/qcow2.h
index 303eb26..c56a5b6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -481,7 +481,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 int qcow2_expand_zero_clusters(BlockDriverState *bs);
 
 /* qcow2-snapshot.c functions */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
+void qcow2_snapshot_create(BlockDriverState *bs,
+                           QEMUSnapshotInfo *sn_info,
+                           Error **errp);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
 int qcow2_snapshot_delete(BlockDriverState *bs,
                           const char *snapshot_id,
diff --git a/block/rbd.c b/block/rbd.c
index 4a1ea5b..5740f5e 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -860,14 +860,16 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }
 
-static int qemu_rbd_snap_create(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info)
+static void qemu_rbd_snap_create(BlockDriverState *bs,
+                                 QEMUSnapshotInfo *sn_info,
+                                 Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
 
     if (sn_info->name[0] == '\0') {
-        return -EINVAL; /* we need a name for rbd snapshots */
+        error_setg(errp, "rbd snapshot requires a valid name");
+        return; /* we need a name for rbd snapshots */
     }
 
     /*
@@ -876,20 +878,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
      */
     if (sn_info->id_str[0] != '\0' &&
         strcmp(sn_info->id_str, sn_info->name) != 0) {
-        return -EINVAL;
+        error_setg(errp, "rbd snapshot id should be empty or equal to name");
+        return;
     }
 
     if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
-        return -ERANGE;
+        error_setg(errp, "rbd snapshot name is too long");
+        return;
     }
 
     r = rbd_snap_create(s->image, sn_info->name);
     if (r < 0) {
-        error_report("failed to create snap: %s", strerror(-r));
-        return r;
+        error_setg_errno(errp, -r, "Failed to create snapshot");
     }
-
-    return 0;
 }
 
 static int qemu_rbd_snap_remove(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b4ae50f..55a2223 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2114,7 +2114,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     return acb->ret;
 }
 
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static void sd_snapshot_create(BlockDriverState *bs,
+                               QEMUSnapshotInfo *sn_info,
+                               Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;
@@ -2127,10 +2129,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
             s->name, sn_info->vm_state_size, s->is_snapshot);
 
     if (s->is_snapshot) {
-        error_report("You can't create a snapshot of a snapshot VDI, "
-                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
+        error_setg(errp, "You can't create a snapshot of a snapshot VDI, "
+                   "%s (%" PRIu32 ")", s->name, s->inode.vdi_id);
 
-        return -EINVAL;
+        return;
     }
 
     DPRINTF("%s %s\n", sn_info->name, sn_info->id_str);
@@ -2147,21 +2149,24 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     /* refresh inode. */
     fd = connect_to_sdog(s);
     if (fd < 0) {
-        ret = fd;
+        error_setg_errno(errp, -fd, "Failed to connect");
         goto cleanup;
     }
 
     ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
                        s->inode.nr_copies, datalen, 0, false, s->cache_flags);
     if (ret < 0) {
-        error_report("failed to write snapshot's inode.");
+        error_setg_errno(errp, -ret, "Failed to write snapshot's inode");
         goto cleanup;
     }
 
     ret = do_sd_create(s, &new_vid, 1);
     if (ret < 0) {
-        error_report("failed to create inode for snapshot. %s",
-                     strerror(errno));
+        error_setg(errp,
+                   "Failed to create inode for snapshot: %d (%s), "
+                   "errno %d (%s)",
+                   ret, strerror(-ret),
+                   errno, strerror(errno));
         goto cleanup;
     }
 
@@ -2171,7 +2176,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                       s->inode.nr_copies, datalen, 0, s->cache_flags);
 
     if (ret < 0) {
-        error_report("failed to read new inode info. %s", strerror(errno));
+        error_setg(errp, "Failed to read new inode info: %d (%s), "
+                   "errno %d (%s)",
+                   ret, strerror(-ret),
+                   errno, strerror(errno));
         goto cleanup;
     }
 
@@ -2181,7 +2189,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
 cleanup:
     closesocket(fd);
-    return ret;
+    return;
 }
 
 /*
diff --git a/block/snapshot.c b/block/snapshot.c
index 9047f8d..b2a49e7 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -156,20 +156,27 @@ int bdrv_can_snapshot(BlockDriverState *bs)
     return 1;
 }
 
-int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+void bdrv_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp)
 {
     BlockDriver *drv = bs->drv;
     if (!drv) {
-        return -ENOMEDIUM;
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
+        return;
     }
     if (drv->bdrv_snapshot_create) {
-        return drv->bdrv_snapshot_create(bs, sn_info);
+        drv->bdrv_snapshot_create(bs, sn_info, errp);
+        return;
     }
     if (bs->file) {
-        return bdrv_snapshot_create(bs->file, sn_info);
+        bdrv_snapshot_create(bs->file, sn_info, errp);
+        return;
     }
-    return -ENOTSUP;
+
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, bdrv_get_device_name(bs),
+              "internal snapshot creation");
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 44755e1..83679ad 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1080,7 +1080,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     qemu_timeval tv;
     BlockdevSnapshotInternal *internal;
     InternalSnapshotState *state;
-    int ret1;
+    Error *local_err = NULL;
 
     g_assert(common->action->kind ==
              TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
@@ -1138,11 +1138,9 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     sn->date_nsec = tv.tv_usec * 1000;
     sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
-    ret1 = bdrv_snapshot_create(bs, sn);
-    if (ret1 < 0) {
-        error_setg_errno(errp, -ret1,
-                         "Failed to create snapshot '%s' on device '%s'",
-                         name, device);
+    bdrv_snapshot_create(bs, sn, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ec0797e..2867adc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -165,8 +165,9 @@ struct BlockDriver {
     int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
                                  const uint8_t *buf, int nb_sectors);
 
-    int (*bdrv_snapshot_create)(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info);
+    void (*bdrv_snapshot_create)(BlockDriverState *bs,
+                                 QEMUSnapshotInfo *sn_info,
+                                 Error **errp);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
     int (*bdrv_snapshot_delete)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 770d9bb..5f286da 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -55,8 +55,9 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
                                        QEMUSnapshotInfo *sn_info,
                                        Error **errp);
 int bdrv_can_snapshot(BlockDriverState *bs);
-int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info);
+void bdrv_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp);
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id);
 int bdrv_snapshot_delete(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index 54ae984..aadd4c7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2122,10 +2122,12 @@ static int img_snapshot(int argc, char **argv)
         sn.date_sec = tv.tv_sec;
         sn.date_nsec = tv.tv_usec * 1000;
 
-        ret = bdrv_snapshot_create(bs, &sn);
-        if (ret) {
-            error_report("Could not create snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
+        bdrv_snapshot_create(bs, &sn, &err);
+        if (error_is_set(&err)) {
+            error_report("Could not create snapshot '%s': %s",
+                         snapshot_name, error_get_pretty(err));
+            error_free(err);
+            ret = 1;
         }
         break;
 
diff --git a/savevm.c b/savevm.c
index 3f912dd..993be2e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2366,6 +2366,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     qemu_timeval tv;
     struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
+    Error *err = NULL;
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
     bs = NULL;
@@ -2439,10 +2440,13 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
-            if (ret < 0) {
-                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
+            bdrv_snapshot_create(bs1, sn, &err);
+            if (error_is_set(&err)) {
+                monitor_printf(mon,
+                               "Error while creating snapshot on '%s': %s\n",
+                               bdrv_get_device_name(bs1),
+                               error_get_pretty(err));
+                error_free(err);
             }
         }
     }
-- 
1.7.1

[Qemu-devel] [PATCH V7 2/6] qcow2: add error message in qcow2_write_snapshots()

[Wenchao Xia]

The function still returns int since qcow2_snapshot_delete() will
return the number.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 670a58d..d7ab4ae 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -152,7 +152,7 @@ fail:
 }
 
 /* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs)
+static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
@@ -183,10 +183,15 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     offset = snapshots_offset;
     if (offset < 0) {
         ret = offset;
+        error_setg_errno(errp, -ret,
+                         "Failed in allocation of cluster for snapshot list");
         goto fail;
     }
     ret = bdrv_flush(bs);
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in flush after snapshot list cluster "
+                         "allocation");
         goto fail;
     }
 
@@ -194,6 +199,10 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
      * must indeed be completely free */
     ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size);
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in overlap check for snapshot list cluster "
+                         "at %" PRIi64 " with size %d",
+                         offset, snapshots_size);
         goto fail;
     }
 
@@ -227,24 +236,40 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
         ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
         if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Failed in write of snapshot header at %"
+                             PRIi64 " with size %d",
+                             offset, (int)sizeof(h));
             goto fail;
         }
         offset += sizeof(h);
 
         ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra));
         if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Failed in write of extra snapshot data at %"
+                             PRIi64 " with size %d",
+                             offset, (int)sizeof(extra));
             goto fail;
         }
         offset += sizeof(extra);
 
         ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
         if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Failed in write of snapshot id string at %"
+                             PRIi64 " with size %d",
+                             offset, id_str_size);
             goto fail;
         }
         offset += id_str_size;
 
         ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
         if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Failed in write of snapshot name string at %"
+                             PRIi64 " with size %d",
+                             offset, name_size);
             goto fail;
         }
         offset += name_size;
@@ -256,6 +281,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
      */
     ret = bdrv_flush(bs);
     if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in flush after snapshot list update");
         goto fail;
     }
 
@@ -268,6 +295,10 @@ 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) {
+        error_setg_errno(errp, -ret,
+                         "Failed in update of image header at %d with size %d",
+                         (int)offsetof(QCowHeader, nb_snapshots),
+                         (int)sizeof(header_data));
         goto fail;
     }
 
@@ -283,6 +314,9 @@ fail:
         qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
                             QCOW2_DISCARD_ALWAYS);
     }
+    if (errp) {
+        g_assert(error_is_set(errp));
+    }
     return ret;
 }
 
@@ -447,10 +481,8 @@ void qcow2_snapshot_create(BlockDriverState *bs,
     s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
 
-    ret = qcow2_write_snapshots(bs);
+    ret = qcow2_write_snapshots(bs, errp);
     if (ret < 0) {
-        /* Following line will be replaced with more detailed error later */
-        error_setg(errp, "Failed in write of snapshot");
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
         s->nb_snapshots--;
@@ -624,9 +656,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
             s->snapshots + snapshot_index + 1,
             (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
     s->nb_snapshots--;
-    ret = qcow2_write_snapshots(bs);
+    ret = qcow2_write_snapshots(bs, errp);
     if (ret < 0) {
-        error_setg(errp, "Failed to remove snapshot from snapshot list");
         return ret;
     }
 
-- 
1.7.1

[Qemu-devel] [PATCH V7 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots

[Wenchao Xia]

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d7ab4ae..55746c4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -299,6 +299,14 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in update of image header at %d with size %d",
                          (int)offsetof(QCowHeader, nb_snapshots),
                          (int)sizeof(header_data));
+
+        /*
+         * If the snapshot data part has been updated on disk, then the
+         * clusters at snapshot_offset may be used in next snapshot operation.
+         * If we free those clusters in fail path, they may be allocated and
+         * made dirty causing damage, so skip cluster free to be safe.
+         */
+        snapshots_offset = 0;
         goto fail;
     }
 
-- 
1.7.1

[Qemu-devel] [PATCH V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Wenchao Xia]

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 55746c4..5f787bc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -400,6 +400,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
     int i, ret;
     uint64_t *l1_table = NULL;
     int64_t l1_table_offset;
+    Error *err = NULL;
 
     memset(sn, 0, sizeof(*sn));
 
@@ -448,7 +449,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
                          PRIu64 " with size %" PRIu64,
                          sn->l1_table_offset,
                          (uint64_t)(s->l1_size * sizeof(uint64_t)));
-        goto fail;
+        goto dealloc_cluster;
     }
 
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
@@ -459,7 +460,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
                          PRIu64 " with size %" PRIu64,
                          sn->l1_table_offset,
                          (uint64_t)(s->l1_size * sizeof(uint64_t)));
-        goto fail;
+        goto dealloc_cluster;
     }
 
     g_free(l1_table);
@@ -476,7 +477,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
                          "Failed in update of refcount for snapshot at %"
                          PRIu64 " with size %d",
                          s->l1_table_offset, s->l1_size);
-        goto fail;
+        goto dealloc_cluster;
     }
 
     /* Append the new snapshot to the snapshot list */
@@ -494,7 +495,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
         s->nb_snapshots--;
-        goto fail;
+        goto restore_refcount;
     }
 
     g_free(old_snapshot_list);
@@ -514,6 +515,22 @@ void qcow2_snapshot_create(BlockDriverState *bs,
 #endif
     return;
 
+restore_refcount:
+    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1)
+        < 0 && errp) {
+        /* Nothing can be done now, need image check later */
+        error_setg(&err, "%s\nqcow2: Error in restoring refcount in snapshot",
+                   error_get_pretty(*errp));
+        error_free(*errp);
+        *errp = NULL;
+        error_propagate(errp, err);
+    }
+
+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

[Qemu-devel] [PATCH V7 5/6] blkdebug: add debug events for snapshot

[Wenchao Xia]

Some code in qcow2-snapshot.c directly accesses bs->file, so in those
places errors can't be injected by other events. Since the code in
qcow2-snapshot.c is similar to the other qcow2 internal code (in regards
to e.g. the L1 table), add some debug events.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/blkdebug.c       |    4 ++++
 block/qcow2-snapshot.c |    3 +++
 include/block/block.h  |    4 ++++
 3 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 37cf028..891b549 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/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5f787bc..2548de7 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -207,6 +207,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     }
 
 
+    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;
@@ -292,6 +293,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     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) {
@@ -452,6 +454,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
         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) {
diff --git a/include/block/block.h b/include/block/block.h
index 36efaea..8901683 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -515,6 +515,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

[Qemu-devel] [PATCH V7 6/6] 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/075           |  218 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/075.out       |   35 ++++++
 tests/qemu-iotests/common.filter |    7 ++
 tests/qemu-iotests/group         |    1 +
 4 files changed, 261 insertions(+), 0 deletions(-)
 create mode 100755 tests/qemu-iotests/075
 create mode 100644 tests/qemu-iotests/075.out

diff --git a/tests/qemu-iotests/075 b/tests/qemu-iotests/075
new file mode 100755
index 0000000..fac50c8
--- /dev/null
+++ b/tests/qemu-iotests/075
@@ -0,0 +1,218 @@
+#!/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/>.
+#
+# Note: This case uses qemu-img snapshot create, and only the default
+# cache mode is covered now.
+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!
+
+BLKDEBUG_CONF="$TEST_DIR/blkdebug.conf"
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm "$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
+# bind the errno correctly and filter the output of image check and qemu-img,
+# if you want to run it on other OS
+_supported_os Linux
+
+
+IMGOPTS="compat=1.1"
+
+CLUSTER_SIZE=65536
+
+SIZE=1G
+
+BLKDBG_TEST_IMG="blkdebug:$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 > "$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 update new L1 table for snapshot"
+
+_make_test_img $SIZE
+
+cat > "$BLKDEBUG_CONF" <<EOF
+[inject-error]
+event = "snapshot_l1_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 "$BLKDBG_TEST_IMG" 2>&1 | _filter_number
+$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 > "$BLKDEBUG_CONF" <<EOF
+[set-state]
+state = "1"
+event = "snapshot_l1_update"
+new_state = "2"
+
+[inject-error]
+state = "2"
+event = "refblock_update_part"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+
+[set-state]
+state = "2"
+event = "refblock_alloc"
+new_state = "3"
+EOF
+
+$QEMU_IMG snapshot -c snap1 "$BLKDBG_TEST_IMG" 2>&1 | _filter_number
+$QEMU_IMG snapshot -l "$TEST_IMG"
+_check_test_img 2>&1
+
+# path 4: fail in snapshot list allocation or its flush 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 or its flush"
+
+_make_test_img $SIZE
+
+cat > "$BLKDEBUG_CONF" <<EOF
+[set-state]
+state = "1"
+event = "snapshot_l1_update"
+new_state = "2"
+
+[inject-error]
+state = "2"
+event = "cluster_alloc"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+# fail directly or in flush, are both OK.
+err=`$QEMU_IMG snapshot -c snap1 "$BLKDBG_TEST_IMG" 2>&1 | grep "Failed" | grep "allocation" | grep "list"`
+if ! test -z "$err"
+then
+    echo "Error happens as expected"
+fi
+$QEMU_IMG snapshot -l "$TEST_IMG"
+_check_test_img 2>&1
+
+
+# path 5: fail in snapshot list update
+echo
+echo "Path 5: fail in snapshot list update"
+
+_make_test_img $SIZE
+
+cat > "$BLKDEBUG_CONF" <<EOF
+[inject-error]
+event = "snapshot_list_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 "$BLKDBG_TEST_IMG" 2>&1 | _filter_number
+$QEMU_IMG snapshot -l "$TEST_IMG"
+_check_test_img 2>&1
+
+# path 6: 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 7: fail in update qcow2 header, it would have leaked cluster since not
+# discard the allocated ones for safe reason, see qcow2-snapshot.c.
+echo
+echo "Path 7: fail in update qcow2 header"
+
+_make_test_img $SIZE
+
+cat > "$BLKDEBUG_CONF" <<EOF
+[inject-error]
+event = "snapshot_header_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 "$BLKDBG_TEST_IMG" 2>&1 | _filter_number
+$QEMU_IMG snapshot -l "$TEST_IMG"
+_check_test_img 2>&1 | _filter_number
+
+# path 8: fail in overlap check before update L1 table for snapshot
+# path 9: 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/075.out b/tests/qemu-iotests/075.out
new file mode 100644
index 0000000..16eb4fc
--- /dev/null
+++ b/tests/qemu-iotests/075.out
@@ -0,0 +1,35 @@
+QA output created by 075
+
+Path 1: fail in allocation of L1 table
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': Failed in allocation of snapshot L1 table: Input/output error
+No errors were found on the image.
+
+Path 2: fail in update new L1 table for snapshot
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': Failed in update of snapshot L1 table at X with size X: 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': Failed in update of refcount for snapshot at X with size X: Input/output error
+No errors were found on the image.
+
+Path 4: fail in snapshot list allocation or its flush
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+Error happens as expected
+No errors were found on the image.
+
+Path 5: fail in snapshot list update
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': Failed in write of snapshot header at X with size X: Input/output error
+No errors were found on the image.
+
+Path 7: fail in update qcow2 header
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': Failed in update of image header at X with size X: Input/output error
+Leaked cluster X refcount=X reference=X
+
+1 leaked clusters were found on the image.
+This means waste of disk space, but no harm to data.
+*** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 9c82c77..34a21c5 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -146,6 +146,13 @@ _filter_win32()
     sed -e 's/\r//g'
 }
 
+# replace number with X
+_filter_number()
+{
+    sed -e 's/ \([0-9]\+\)/ X/g' \
+        -e 's/=\([0-9]\+\)/=X/g'
+}
+
 # sanitize qemu-io output
 _filter_qemu_io()
 {
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cc750c9..31fcbff 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -79,3 +79,4 @@
 070 rw auto
 073 rw auto
 074 rw auto
+075 rw auto
-- 
1.7.1

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

[Wenchao Xia]

hello, OK to merge?

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

[Wenchao Xia]

ping?

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

[Wenchao Xia]

Hello, any comments? I hope to have a new year gift....

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

[Stefan Hajnoczi]

On Wed, Dec 18, 2013 at 10:13:40AM +0800, Wenchao Xia wrote:
> Hello, any comments? I hope to have a new year gift....

It's in the queue, I hope to review it this week.

Re: [Qemu-devel] [PATCH V7 2/6] qcow2: add error message in qcow2_write_snapshots()

[Stefan Hajnoczi]

On Thu, Dec 05, 2013 at 08:02:48PM +0800, Wenchao Xia wrote:
> @@ -227,24 +236,40 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>  
>          ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
>          if (ret < 0) {
> +            error_setg_errno(errp, -ret,
> +                             "Failed in write of snapshot header at %"
> +                             PRIi64 " with size %d",
> +                             offset, (int)sizeof(h));

The casts in this patch are not necessary:
size_t == %zu
ssize_t == %zd

(Very minor point but in case you respin)

Re: [Qemu-devel] [PATCH V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Stefan Hajnoczi]

On Thu, Dec 05, 2013 at 08:02:50PM +0800, Wenchao Xia wrote:
> +restore_refcount:
> +    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1)
> +        < 0 && errp) {
> +        /* Nothing can be done now, need image check later */
> +        error_setg(&err, "%s\nqcow2: Error in restoring refcount in snapshot",
> +                   error_get_pretty(*errp));
> +        error_free(*errp);
> +        *errp = NULL;
> +        error_propagate(errp, err);
> +    }

We get here if writing the new snapshot list failed.  If
qcow2_update_snapshot_refcount(..., -1) also fails I think we should
skip qcow2_free_clusters() below.  We don't know the exact state of the
disk image anymore - better to leak clusters than to have a dangling
reference.

> +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 V7 1/6] snapshot: add parameter *errp in snapshot create

[Stefan Hajnoczi]

On Thu, Dec 05, 2013 at 08:02:47PM +0800, Wenchao Xia wrote:
> @@ -366,7 +368,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>  
>      /* Check that the ID is unique */
>      if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
> -        return -EEXIST;
> +        error_setg(errp, "Snapshot with id %s already exist", sn_info->id_str);

s/exist/exists/

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

[Stefan Hajnoczi]

On Thu, Dec 05, 2013 at 08:02:46PM +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.
> 
> v4:
>   General change:
>   rebased on upstream since error path for qcow2_write_snapshots() already
> exist in upstream. removed old patch 1 since it is fixed by Max in upstream.
>   5: moved the snapshot_l1_update event just before write operation, instead of
> before overlap check, since it is more straight.
>   6: remove a duplicated error path test about flush after snapshot list
> update, add a filter which replace number to X, since now in error in report
> detailed message including error cluster number.
>   Address Stefan's comments:
>   1, 2, 4: add *errp to store detailed error message, instead of error_report()
> and compile time determined debug printf message.
>   3: do not free cluster when fail in header update for safety reason.
>   Address Eric's comments:
>   1, 2, 4: add *errp to store detailed error message, instead of error_report()
> and compile time determined debug printf message.
>   5: squashed patches that add and use debug events.
>   6: added comments about test only on Linux.
> 
> v5:
>   General change:
>   6: rebased on upstream, use case number 070, adjust 070.out due to error
> message change in this version.
> 
>   Address Max's comments:
>   1 use error_setg_errno() when possible, remove "ret =" in functions when
> possible since the function does not need to return int value, fix 32bit/
> 64bit issue in printf for "sizeof" and "offse", typo fix.
>   2 use error_setg_errno() when possible, fix 32bit/64bit issue in printf
> for "sizeof" and "offse", typo fix.
>   3 typo fix in comments.
>   5 typo fix in commit message.
> 
>   Address Eric's comments:
>   2 fix 32bit/64bit issue in printf for "sizeof" and "offse".
> 
> v6:
>   Address Jeff's comments:
>   6: add quote for image name in test case.
> 
> v7:
>   Rebased on Stefan's block tree, since I need to test after Fam's
> cache mode series.
>   6: change case number to 075 to avoid conflict, add a comments in
> case that it covers only default cache mode, qemu-img snapshot would
> not be affected by case's cache setting.
> 
> Wenchao Xia (6):
>   1 snapshot: add parameter *errp in snapshot create
>   2 qcow2: add error message in qcow2_write_snapshots()
>   3 qcow2: do not free clusters when fail in header update in qcow2_write_snapshots
>   4 qcow2: cancel the modification on fail in qcow2_snapshot_create()
>   5 blkdebug: add debug events for snapshot
>   6 qemu-iotests: add test for qcow2 snapshot
> 
>  block/blkdebug.c                 |    4 +
>  block/qcow2-snapshot.c           |  105 ++++++++++++++++---
>  block/qcow2.h                    |    4 +-
>  block/rbd.c                      |   19 ++--
>  block/sheepdog.c                 |   28 +++--
>  block/snapshot.c                 |   19 +++-
>  blockdev.c                       |   10 +-
>  include/block/block.h            |    4 +
>  include/block/block_int.h        |    5 +-
>  include/block/snapshot.h         |    5 +-
>  qemu-img.c                       |   10 +-
>  savevm.c                         |   12 ++-
>  tests/qemu-iotests/075           |  218 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/075.out       |   35 ++++++
>  tests/qemu-iotests/common.filter |    7 ++
>  tests/qemu-iotests/group         |    1 +
>  16 files changed, 429 insertions(+), 57 deletions(-)
>  create mode 100755 tests/qemu-iotests/075
>  create mode 100644 tests/qemu-iotests/075.out

This looks very close.  I pointed out one code path where I feel freeing
clusters is too agressive (better to leak than to corrupt the image).
There are a few minor fixups that would be nice too.

Otherwise, looks good.  Good job on extending blkdebug and adding a test
case.

Re: [Qemu-devel] [PATCH V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Wenchao Xia]

于 2013/12/20 22:20, Stefan Hajnoczi 写道:
> On Thu, Dec 05, 2013 at 08:02:50PM +0800, Wenchao Xia wrote:
>> +restore_refcount:
>> +    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1)
>> +        < 0 && errp) {
>> +        /* Nothing can be done now, need image check later */
>> +        error_setg(&err, "%s\nqcow2: Error in restoring refcount in snapshot",
>> +                   error_get_pretty(*errp));
>> +        error_free(*errp);
>> +        *errp = NULL;
>> +        error_propagate(errp, err);
>> +    }
>
> We get here if writing the new snapshot list failed.  If
> qcow2_update_snapshot_refcount(..., -1) also fails I think we should
> skip qcow2_free_clusters() below.  We don't know the exact state of the
> disk image anymore - better to leak clusters than to have a dangling
> reference.
>

   Make sense, dangling point should be avoid in any case, will fix,
Thanks for reviewing!

>> +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 V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Wenchao Xia]

于 2013/12/23 10:57, Wenchao Xia 写道:
> 于 2013/12/20 22:20, Stefan Hajnoczi 写道:
>> On Thu, Dec 05, 2013 at 08:02:50PM +0800, Wenchao Xia wrote:
>>> +restore_refcount:
>>> +    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>>> s->l1_size, -1)
>>> +        < 0 && errp) {
>>> +        /* Nothing can be done now, need image check later */
>>> +        error_setg(&err, "%s\nqcow2: Error in restoring refcount in
>>> snapshot",
>>> +                   error_get_pretty(*errp));
>>> +        error_free(*errp);
>>> +        *errp = NULL;
>>> +        error_propagate(errp, err);
>>> +    }
>>
>> We get here if writing the new snapshot list failed.  If
>> qcow2_update_snapshot_refcount(..., -1) also fails I think we should
>> skip qcow2_free_clusters() below.  We don't know the exact state of the
>> disk image anymore - better to leak clusters than to have a dangling
>> reference.
>>
>
>    Make sense, dangling point should be avoid in any case, will fix,
> Thanks for reviewing!
>
>>> +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
>>>
>>>
>>
>
>
Hi, Stefan
   I have reconsidered the roll back process, there is many case we
should take care, so it is better to summarize a general rule to do such
cancel operations. I suggest: do a series of roll back operations,
when one fail, skip following roll back operation. For snapshot create,
the create action is:
allocate new L1 -> refcount+1 -> allocate sn_list -> update header
The mirrored rollback action can be:
deallocate L1 <- refcount-1 <- deallocate sn_list <- restore header

When fail happens in rollback action, simply stop following ones.
If you agree, I'd like to reorganize the patch as above.

Re: [Qemu-devel] [PATCH V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Stefan Hajnoczi]

On Mon, Dec 23, 2013 at 02:12:56PM +0800, Wenchao Xia wrote:
>   I have reconsidered the roll back process, there is many case we
> should take care, so it is better to summarize a general rule to do such
> cancel operations. I suggest: do a series of roll back operations,
> when one fail, skip following roll back operation. For snapshot create,
> the create action is:
> allocate new L1 -> refcount+1 -> allocate sn_list -> update header
> The mirrored rollback action can be:
> deallocate L1 <- refcount-1 <- deallocate sn_list <- restore header
> 
> When fail happens in rollback action, simply stop following ones.
> If you agree, I'd like to reorganize the patch as above.

I agree.  When the steps depend on each other we should skip further
operations when an error is returned in the failure path.  If the steps
are independent we can still safely clean up the independent parts.

Stefan