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

[Qemu-devel] [PATCH V6 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.

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/070           |  216 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/070.out       |   35 ++++++
 tests/qemu-iotests/common.filter |    7 ++
 tests/qemu-iotests/group         |    1 +
 16 files changed, 427 insertions(+), 57 deletions(-)
 create mode 100755 tests/qemu-iotests/070
 create mode 100644 tests/qemu-iotests/070.out

[Qemu-devel] [PATCH V6 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 3529c68..532b7f6 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 922e190..c0a3d01 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 ef387de..7b70810 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2053,7 +2053,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;
@@ -2066,10 +2068,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);
@@ -2086,22 +2088,25 @@ 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, s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,
                        1, s->inode.copy_policy);
     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;
     }
 
@@ -2111,7 +2116,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;
     }
 
@@ -2121,7 +2129,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 a05c0c0..3122e3c 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -138,20 +138,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 b260477..67c2a9b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1078,7 +1078,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);
@@ -1136,11 +1136,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 1666066..a2ba326 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -164,8 +164,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 012bf22..a998316 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -47,8 +47,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 bf3fb4f..d3613af 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2080,10 +2080,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 2f631d4..617d6eb 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 V6 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 532b7f6..6a1d9de 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 V6 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 6a1d9de..70e329e 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 V6 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 70e329e..685ef8b 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 V6 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 16d2b91..3d5f7cf 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 685ef8b..d2e5275 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 3560deb..cbccc3d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -476,6 +476,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 V6 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/070           |  216 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/070.out       |   35 ++++++
 tests/qemu-iotests/common.filter |    7 ++
 tests/qemu-iotests/group         |    1 +
 4 files changed, 259 insertions(+), 0 deletions(-)
 create mode 100755 tests/qemu-iotests/070
 create mode 100644 tests/qemu-iotests/070.out

diff --git a/tests/qemu-iotests/070 b/tests/qemu-iotests/070
new file mode 100755
index 0000000..154acc4
--- /dev/null
+++ b/tests/qemu-iotests/070
@@ -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, 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/070.out b/tests/qemu-iotests/070.out
new file mode 100644
index 0000000..1969e3f
--- /dev/null
+++ b/tests/qemu-iotests/070.out
@@ -0,0 +1,35 @@
+QA output created by 070
+
+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 8e7b1a4..e8eba65 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 c57ff35..b18b241 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -75,3 +75,4 @@
 067 rw auto
 068 rw auto
 069 rw auto
+070 rw auto
-- 
1.7.1

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

[Wenchao Xia]

  ping?

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

[Wenchao Xia]

Hi, Any comments for it? respin?

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

[Max Reitz]

On 11.11.2013 00:56, Wenchao Xia wrote:
> This test will focus on the low level procedure of qcow2 snapshot
> operations, now it covers only the create operation. Overlap error
> paths are not checked since no good way to trigger those errors.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Aside from the fact that I/O test 070 is already taken:

Reviewed-by: Max Reitz <mreitz@redhat.com>

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

[Wenchao Xia]

于 2013/11/30 4:56, Max Reitz 写道:
> On 11.11.2013 00:56, Wenchao Xia wrote:
>> This test will focus on the low level procedure of qcow2 snapshot
>> operations, now it covers only the create operation. Overlap error
>> paths are not checked since no good way to trigger those errors.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Aside from the fact that I/O test 070 is already taken:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
Thanks for review:), will rebase a version.