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

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

v8:
  Address Stefan's comments:
  1/8: typo fix.
  2/8: remove the type case for sizeof and offsetof.
  3/8, 4/8: new patches help appending error message and detect error.
  5/8: old patch that skip cluster free when header update fail, is removed.
Instead, this patch improved qcow2_write_snapshots()'s rollback procedure by
restore header.
  6/8: new variable *err_rollback is introduced to detect sub function's
rollback error. With new function introduced by patch 3/8, message pending
is simplified so old variable Error *err is removed.
  Note: patch 5/8 and 6/8 does a full mirrored rollback operation, and
follows the rule that skip following steps when one step fail in rollback
procedure.
  8/8: changed the qcow2 header update fail case correspondly.

Wenchao Xia (8):
  1 snapshot: add parameter *errp in snapshot create
  2 qcow2: add error message in qcow2_write_snapshots()
  3 util: add error_append()
  4 qcow2: return int for qcow2_free_clusters()
  5 qcow2: full rollback on fail in qcow2_write_snapshots()
  6 qcow2: rollback on fail in qcow2_snapshot_create()
  7 blkdebug: add debug events for snapshot
  8 qemu-iotests: add test for qcow2 snapshot

 block/blkdebug.c                 |    4 +
 block/qcow2-refcount.c           |    8 +-
 block/qcow2-snapshot.c           |  164 ++++++++++++++++++++++++-----
 block/qcow2.h                    |   10 +-
 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 +-
 include/qapi/error.h             |    6 +
 qemu-img.c                       |   10 +-
 savevm.c                         |   12 ++-
 tests/qemu-iotests/075           |  216 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/075.out       |   32 ++++++
 tests/qemu-iotests/common.filter |    7 ++
 tests/qemu-iotests/group         |    1 +
 util/error.c                     |   21 ++++
 19 files changed, 505 insertions(+), 76 deletions(-)
 create mode 100755 tests/qemu-iotests/075
 create mode 100644 tests/qemu-iotests/075.out

[Qemu-devel] [PATCH V8 1/8] 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>
---
 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..8cac42d 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 exists", 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 d1c812d..7001900 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2116,7 +2116,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;
@@ -2129,10 +2131,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);
@@ -2149,21 +2151,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;
     }
 
@@ -2173,7 +2178,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;
     }
 
@@ -2183,7 +2191,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 6a85961..2e45898 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 8b132d7..28b7a14 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 a4b3931..97dec9a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2167,10 +2167,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 V8 2/8] 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>
---
 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 8cac42d..5619fc3 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 %zu",
+                             offset, 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 %zu",
+                             offset, 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 %zu with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         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 V8 3/8] util: add error_append()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/qapi/error.h |    6 ++++++
 util/error.c         |   21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7d4c696..bcfb724 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -30,6 +30,12 @@ typedef struct Error Error;
 void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 
 /**
+ * Append message to err if err != NULL && *err != NULL. "\n" will be inserted
+ * after old message.
+ */
+void error_append(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+
+/**
  * Set an indirect pointer to an error given a ErrorClass value and a
  * printf-style human message, followed by a strerror() string if
  * @os_error is not zero.
diff --git a/util/error.c b/util/error.c
index 3ee362a..64bbb2d 100644
--- a/util/error.c
+++ b/util/error.c
@@ -46,6 +46,27 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     errno = saved_errno;
 }
 
+void error_append(Error **err, const char *fmt, ...)
+{
+    va_list ap;
+    gchar *msg, *msg_old;
+
+    if (!err || !*err) {
+        return;
+    }
+
+    va_start(ap, fmt);
+    msg = g_strdup_vprintf(fmt, ap);
+    va_end(ap);
+
+    msg_old = (*err)->msg;
+    (*err)->msg = g_strdup_printf("%s\n%s", msg_old, msg);
+
+    g_free(msg);
+    g_free(msg_old);
+
+}
+
 void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                      const char *fmt, ...)
 {
-- 
1.7.1

Re: [Qemu-devel] [PATCH V8 3/8] util: add error_append()

[Peter Crosthwaite]

On Fri, Jan 3, 2014 at 1:08 PM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/qapi/error.h |    6 ++++++
>  util/error.c         |   21 +++++++++++++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 7d4c696..bcfb724 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -30,6 +30,12 @@ typedef struct Error Error;
>  void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>
>  /**
> + * Append message to err if err != NULL && *err != NULL. "\n" will be inserted
> + * after old message.
> + */
> +void error_append(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> +
> +/**
>   * Set an indirect pointer to an error given a ErrorClass value and a
>   * printf-style human message, followed by a strerror() string if
>   * @os_error is not zero.
> diff --git a/util/error.c b/util/error.c
> index 3ee362a..64bbb2d 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -46,6 +46,27 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      errno = saved_errno;
>  }
>
> +void error_append(Error **err, const char *fmt, ...)

errp

> +{
> +    va_list ap;
> +    gchar *msg, *msg_old;
> +
> +    if (!err || !*err) {

Should appending to an unset error really be a nop? You should just
set the error as normal in this case (error_set()?).

This avoids callers having to:

if (error_is_set(&err)) {
   error_append(&err);
} else {
   error_set(&err);
}

so they can just append if they want appending.

Generally speaking, appending to nothing should give you something.

Regards,
Peter

> +        return;
> +    }
> +
> +    va_start(ap, fmt);
> +    msg = g_strdup_vprintf(fmt, ap);
> +    va_end(ap);
> +
> +    msg_old = (*err)->msg;
> +    (*err)->msg = g_strdup_printf("%s\n%s", msg_old, msg);
> +
> +    g_free(msg);
> +    g_free(msg_old);
> +
> +}
> +
>  void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>                       const char *fmt, ...)
>  {
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH V8 3/8] util: add error_append()

[Wenchao Xia]

于 2014/1/3 23:32, Peter Crosthwaite 写道:
> On Fri, Jan 3, 2014 at 1:08 PM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   include/qapi/error.h |    6 ++++++
>>   util/error.c         |   21 +++++++++++++++++++++
>>   2 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 7d4c696..bcfb724 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -30,6 +30,12 @@ typedef struct Error Error;
>>   void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>>
>>   /**
>> + * Append message to err if err != NULL && *err != NULL. "\n" will be inserted
>> + * after old message.
>> + */
>> +void error_append(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>> +
>> +/**
>>    * Set an indirect pointer to an error given a ErrorClass value and a
>>    * printf-style human message, followed by a strerror() string if
>>    * @os_error is not zero.
>> diff --git a/util/error.c b/util/error.c
>> index 3ee362a..64bbb2d 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -46,6 +46,27 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>       errno = saved_errno;
>>   }
>>
>> +void error_append(Error **err, const char *fmt, ...)
>
> errp
>
>> +{
>> +    va_list ap;
>> +    gchar *msg, *msg_old;
>> +
>> +    if (!err || !*err) {
>
> Should appending to an unset error really be a nop? You should just
> set the error as normal in this case (error_set()?).
>
> This avoids callers having to:
>
> if (error_is_set(&err)) {
>     error_append(&err);
> } else {
>     error_set(&err);
> }
>
> so they can just append if they want appending.
>
> Generally speaking, appending to nothing should give you something.
>
> Regards,
> Peter
>

   Make sense, will fix as above, thanks for review.

>> +        return;
>> +    }
>> +
>> +    va_start(ap, fmt);
>> +    msg = g_strdup_vprintf(fmt, ap);
>> +    va_end(ap);
>> +
>> +    msg_old = (*err)->msg;
>> +    (*err)->msg = g_strdup_printf("%s\n%s", msg_old, msg);
>> +
>> +    g_free(msg);
>> +    g_free(msg_old);
>> +
>> +}
>> +
>>   void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>>                        const char *fmt, ...)
>>   {
>> --
>> 1.7.1
>>
>>
>

[Qemu-devel] [PATCH V8 4/8] qcow2: return int for qcow2_free_clusters()

[Wenchao Xia]

The return value can help caller check whether error happens,
and it need not to have *errp since the return value already tip
what happend.

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c974abe..4eb413a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -761,9 +761,9 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
     return offset;
 }
 
-void qcow2_free_clusters(BlockDriverState *bs,
-                          int64_t offset, int64_t size,
-                          enum qcow2_discard_type type)
+int qcow2_free_clusters(BlockDriverState *bs,
+                        int64_t offset, int64_t size,
+                        enum qcow2_discard_type type)
 {
     int ret;
 
@@ -773,6 +773,8 @@ void qcow2_free_clusters(BlockDriverState *bs,
         fprintf(stderr, "qcow2_free_clusters failed: %s\n", strerror(-ret));
         /* TODO Remember the clusters to free them later and avoid leaking */
     }
+
+    return ret;
 }
 
 /*
diff --git a/block/qcow2.h b/block/qcow2.h
index c56a5b6..161549a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -435,9 +435,9 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, int64_t size);
 int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
     int nb_clusters);
 int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size);
-void qcow2_free_clusters(BlockDriverState *bs,
-                          int64_t offset, int64_t size,
-                          enum qcow2_discard_type type);
+int qcow2_free_clusters(BlockDriverState *bs,
+                        int64_t offset, int64_t size,
+                        enum qcow2_discard_type type);
 void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
                              int nb_clusters, enum qcow2_discard_type type);
 
-- 
1.7.1

[Qemu-devel] [PATCH V8 5/8] qcow2: full rollback on fail in qcow2_write_snapshots()

[Wenchao Xia]

Header restore step is added, and old code section "fail" is
renamed to "dealloc_sn_table" to tip better, now "fail" section
does not rollback anything on disk. When one step in rollback
fails, following steps will be skipped, to make sure no dangling
pointer is left.

A new parameter "*errp_rollback" is added to tell caller the result
of rollback procedure.

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5619fc3..5cac714 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -152,7 +152,9 @@ fail:
 }
 
 /* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
+static int qcow2_write_snapshots(BlockDriverState *bs,
+                                 Error **errp,
+                                 Error **errp_rollback)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
@@ -162,9 +164,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     struct {
         uint32_t nb_snapshots;
         uint64_t snapshots_offset;
-    } QEMU_PACKED header_data;
+    } QEMU_PACKED header_data, header_data_old;
     int64_t offset, snapshots_offset;
-    int ret;
+    int ret, ret0;
 
     /* compute the size of the snapshots */
     offset = 0;
@@ -192,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret,
                          "Failed in flush after snapshot list cluster "
                          "allocation");
-        goto fail;
+        goto dealloc_sn_table;
     }
 
     /* The snapshot list position has not yet been updated, so these clusters
@@ -203,7 +205,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in overlap check for snapshot list cluster "
                          "at %" PRIi64 " with size %d",
                          offset, snapshots_size);
-        goto fail;
+        goto dealloc_sn_table;
     }
 
 
@@ -240,7 +242,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot header at %"
                              PRIi64 " with size %zu",
                              offset, sizeof(h));
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += sizeof(h);
 
@@ -250,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of extra snapshot data at %"
                              PRIi64 " with size %zu",
                              offset, sizeof(extra));
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += sizeof(extra);
 
@@ -260,7 +262,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot id string at %"
                              PRIi64 " with size %d",
                              offset, id_str_size);
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += id_str_size;
 
@@ -270,7 +272,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot name string at %"
                              PRIi64 " with size %d",
                              offset, name_size);
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += name_size;
     }
@@ -283,7 +285,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     if (ret < 0) {
         error_setg_errno(errp, -ret,
                          "Failed in flush after snapshot list update");
-        goto fail;
+        goto dealloc_sn_table;
+    }
+
+    /* Start the qcow2 header update */
+    ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
+                     &header_data_old, sizeof(header_data_old));
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in read of image header at %zu with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         sizeof(header_data_old));
+        goto dealloc_sn_table;
     }
 
     QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
@@ -299,7 +312,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in update of image header at %zu with size %zu",
                          offsetof(QCowHeader, nb_snapshots),
                          sizeof(header_data));
-        goto fail;
+        goto restore_header;
     }
 
     /* free the old snapshot table */
@@ -309,11 +322,29 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     s->snapshots_size = snapshots_size;
     return 0;
 
-fail:
+restore_header:
+    ret0 = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
+                            &header_data_old, sizeof(header_data_old));
+    if (ret0 < 0) {
+        error_setg_errno(errp_rollback, -ret0,
+                         "In rollback failed to restore image header at %zu "
+                         "with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         sizeof(header_data));
+        goto fail;
+    }
+
+dealloc_sn_table:
     if (snapshots_offset > 0) {
-        qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
-                            QCOW2_DISCARD_ALWAYS);
+        ret0 = qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
+                                   QCOW2_DISCARD_ALWAYS);
+        if (ret0 < 0) {
+            error_setg_errno(errp_rollback, -ret0,
+                             "In rollback failed to free snapshot table");
+        }
     }
+
+fail:
     if (errp) {
         g_assert(error_is_set(errp));
     }
@@ -481,7 +512,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
     s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
 
-    ret = qcow2_write_snapshots(bs, errp);
+    ret = qcow2_write_snapshots(bs, errp, NULL);
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
@@ -656,7 +687,7 @@ 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, errp);
+    ret = qcow2_write_snapshots(bs, errp, NULL);
     if (ret < 0) {
         return ret;
     }
-- 
1.7.1

[Qemu-devel] [PATCH V8 6/8] qcow2: rollback on fail in qcow2_snapshot_create()

[Wenchao Xia]

A new variable *err_rollback is added to detect sub function's
rollback failure. If one step in rollback procedure fails, following
steps will be skipped, and the error message will be appended
to errp.

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5cac714..29ba534 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -423,6 +423,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
     int i, ret;
     uint64_t *l1_table = NULL;
     int64_t l1_table_offset;
+    Error *err_rollback = NULL;
 
     memset(sn, 0, sizeof(*sn));
 
@@ -471,7 +472,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_l1_table;
     }
 
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
@@ -482,7 +483,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_l1_table;
     }
 
     g_free(l1_table);
@@ -499,7 +500,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_l1_table;
     }
 
     /* Append the new snapshot to the snapshot list */
@@ -512,12 +513,18 @@ void qcow2_snapshot_create(BlockDriverState *bs,
     s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
 
-    ret = qcow2_write_snapshots(bs, errp, NULL);
+    ret = qcow2_write_snapshots(bs, errp, &err_rollback);
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
         s->nb_snapshots--;
-        goto fail;
+        if (error_is_set(&err_rollback)) {
+            error_append(errp, "%s", error_get_pretty(err_rollback));
+            error_free(err_rollback);
+            goto fail;
+        } else {
+            goto restore_refcount;
+        }
     }
 
     g_free(old_snapshot_list);
@@ -537,6 +544,26 @@ 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) {
+        /* Nothing can be done now, need image check later, skip following
+           rollback action. */
+        error_append(errp,
+                    "In rollback failed to restore refcount in snapshot");
+        goto fail;
+    }
+
+dealloc_l1_table:
+    ret = qcow2_free_clusters(bs, sn->l1_table_offset,
+                              sn->l1_size * sizeof(uint64_t),
+                              QCOW2_DISCARD_ALWAYS);
+    if (ret < 0) {
+        error_append(errp,
+                     "In rollback failed to free L1 table: %s\n",
+                     strerror(-ret));
+    }
+
 fail:
     g_free(sn->id_str);
     g_free(sn->name);
-- 
1.7.1

[Qemu-devel] [PATCH V8 7/8] 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 957be2c..9c801fb 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 29ba534..8a143e7 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -209,6 +209,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;
@@ -305,6 +306,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) {
@@ -475,6 +477,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
         goto dealloc_l1_table;
     }
 
+    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 V8 8/8] 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           |  216 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/075.out       |   32 ++++++
 tests/qemu-iotests/common.filter |    7 ++
 tests/qemu-iotests/group         |    1 +
 4 files changed, 256 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..131eadf
--- /dev/null
+++ b/tests/qemu-iotests/075
@@ -0,0 +1,216 @@
+#!/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!
+
+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, normally header file will be recovered,
+# 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..9a40e4a
--- /dev/null
+++ b/tests/qemu-iotests/075.out
@@ -0,0 +1,32 @@
+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
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 776985d..6138d27 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 V8 0/8] qcow2: rollback the modification on fail in snapshot creation

[Stefan Hajnoczi]

On Fri, Jan 03, 2014 at 11:08:44AM +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.
> 
> v8:
>   Address Stefan's comments:
>   1/8: typo fix.
>   2/8: remove the type case for sizeof and offsetof.
>   3/8, 4/8: new patches help appending error message and detect error.
>   5/8: old patch that skip cluster free when header update fail, is removed.
> Instead, this patch improved qcow2_write_snapshots()'s rollback procedure by
> restore header.
>   6/8: new variable *err_rollback is introduced to detect sub function's
> rollback error. With new function introduced by patch 3/8, message pending
> is simplified so old variable Error *err is removed.
>   Note: patch 5/8 and 6/8 does a full mirrored rollback operation, and
> follows the rule that skip following steps when one step fail in rollback
> procedure.
>   8/8: changed the qcow2 header update fail case correspondly.
> 
> Wenchao Xia (8):
>   1 snapshot: add parameter *errp in snapshot create
>   2 qcow2: add error message in qcow2_write_snapshots()
>   3 util: add error_append()
>   4 qcow2: return int for qcow2_free_clusters()
>   5 qcow2: full rollback on fail in qcow2_write_snapshots()
>   6 qcow2: rollback on fail in qcow2_snapshot_create()
>   7 blkdebug: add debug events for snapshot
>   8 qemu-iotests: add test for qcow2 snapshot
> 
>  block/blkdebug.c                 |    4 +
>  block/qcow2-refcount.c           |    8 +-
>  block/qcow2-snapshot.c           |  164 ++++++++++++++++++++++++-----
>  block/qcow2.h                    |   10 +-
>  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 +-
>  include/qapi/error.h             |    6 +
>  qemu-img.c                       |   10 +-
>  savevm.c                         |   12 ++-
>  tests/qemu-iotests/075           |  216 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/075.out       |   32 ++++++
>  tests/qemu-iotests/common.filter |    7 ++
>  tests/qemu-iotests/group         |    1 +
>  util/error.c                     |   21 ++++
>  19 files changed, 505 insertions(+), 76 deletions(-)
>  create mode 100755 tests/qemu-iotests/075
>  create mode 100644 tests/qemu-iotests/075.out

I agree with Peter regarding error_append().

Overall I'm happy with this series but defer to Kevin for a final
review.  Error handling approach is becoming pretty intricate (there are
now 2 error paths "failed and then rolled back successfully" and "failed
while rolling back").

Stefan