* [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
@ 2013-10-14 21:52 ` Wenchao Xia
2013-11-02 12:39 ` Max Reitz
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots() Wenchao Xia
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-10-14 21:52 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz, pbonzini, 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 | 27 +++++++++++++++++++++++----
block/qcow2.h | 4 +++-
block/rbd.c | 21 ++++++++++++---------
block/sheepdog.c | 29 ++++++++++++++++++++---------
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, 95 insertions(+), 47 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 3529c68..b373f9a 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 */
@@ -383,6 +386,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
if (l1_table_offset < 0) {
ret = l1_table_offset;
+ error_setg(errp,
+ "Failed in allocation of snapshot L1 table: %d (%s)",
+ ret, strerror(-ret));
goto fail;
}
@@ -397,12 +403,20 @@ 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(errp, "Failed in overlap check for snapshot L1 table at %"
+ PRIu64 " with size %" PRIu64 ": %d (%s)",
+ sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
+ ret, strerror(-ret));
goto fail;
}
ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
s->l1_size * sizeof(uint64_t));
if (ret < 0) {
+ error_setg(errp, "Failed in update of snapshot L1 table at %"
+ PRIu64 " with size %" PRIu64 ": %d (%s)",
+ sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
+ ret, strerror(-ret));
goto fail;
}
@@ -416,6 +430,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
*/
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
if (ret < 0) {
+ error_setg(errp, "Failed in update of refcount for snapshot at %"
+ PRIu64 " with size %d: %d (%s)",
+ s->l1_table_offset, s->l1_size, ret, strerror(-ret));
goto fail;
}
@@ -431,6 +448,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 +471,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..6181999 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 need a valid name");
+ return; /* we need a name for rbd snapshots */
}
/*
@@ -876,20 +878,21 @@ 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(errp,
+ "Failed to create snapshot: %d (%s)",
+ r, strerror(-r));
}
-
- return 0;
}
static int qemu_rbd_snap_remove(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 5f81c93..67aaeb7 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1961,7 +1961,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;
@@ -1974,10 +1976,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);
@@ -1995,21 +1997,27 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
fd = connect_to_sdog(s);
if (fd < 0) {
ret = fd;
+ error_setg(errp, "Failed to connect: %d (%s)", ret, strerror(-ret));
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(errp,
+ "Failed to write snapshot's inode: %d (%s)",
+ ret, strerror(-ret));
goto cleanup;
}
ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &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;
}
@@ -2019,7 +2027,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;
}
@@ -2029,7 +2040,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 4f76e28..c4e7124 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1077,7 +1077,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);
@@ -1135,11 +1135,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 a48731d..553d843 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -161,8 +161,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 926f0a0..aa8b46f 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create Wenchao Xia
@ 2013-11-02 12:39 ` Max Reitz
2013-11-04 1:47 ` Wenchao Xia
0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2013-11-02 12:39 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, stefanha
On 14.10.2013 23:52, Wenchao Xia wrote:
> 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 | 27 +++++++++++++++++++++++----
> block/qcow2.h | 4 +++-
> block/rbd.c | 21 ++++++++++++---------
> block/sheepdog.c | 29 ++++++++++++++++++++---------
> 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, 95 insertions(+), 47 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 3529c68..b373f9a 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 */
> @@ -383,6 +386,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
> if (l1_table_offset < 0) {
> ret = l1_table_offset;
> + error_setg(errp,
> + "Failed in allocation of snapshot L1 table: %d (%s)",
> + ret, strerror(-ret));
You may want to use error_setg_errno instead (here and in many other
places in this patch).
> goto fail;
> }
>
> @@ -397,12 +403,20 @@ 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(errp, "Failed in overlap check for snapshot L1 table at %"
> + PRIu64 " with size %" PRIu64 ": %d (%s)",
> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
> + ret, strerror(-ret));
s->l1_size * sizeof(uint64_t) is of the same type as uint64_t (PRIu64)
only on 64 bit systems. On 32 bit systems, it probably is the same as
uint32_t (because of a different size_t width).
> goto fail;
> }
>
> ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
> s->l1_size * sizeof(uint64_t));
> if (ret < 0) {
> + error_setg(errp, "Failed in update of snapshot L1 table at %"
> + PRIu64 " with size %" PRIu64 ": %d (%s)",
> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
> + ret, strerror(-ret));
Same here.
> goto fail;
> }
>
> @@ -416,6 +430,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> */
> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
> if (ret < 0) {
> + error_setg(errp, "Failed in update of refcount for snapshot at %"
> + PRIu64 " with size %d: %d (%s)",
> + s->l1_table_offset, s->l1_size, ret, strerror(-ret));
> goto fail;
> }
>
> @@ -431,6 +448,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 +471,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..6181999 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 need a valid name");
*needs (or "requires")
> + return; /* we need a name for rbd snapshots */
> }
>
> /*
> @@ -876,20 +878,21 @@ 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(errp,
> + "Failed to create snapshot: %d (%s)",
> + r, strerror(-r));
> }
> -
> - return 0;
> }
>
> static int qemu_rbd_snap_remove(BlockDriverState *bs,
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 5f81c93..67aaeb7 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1961,7 +1961,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;
> @@ -1974,10 +1976,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);
> @@ -1995,21 +1997,27 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> fd = connect_to_sdog(s);
> if (fd < 0) {
> ret = fd;
> + error_setg(errp, "Failed to connect: %d (%s)", ret, strerror(-ret));
You may remove the "ret = fd" line before the error_setg(), if you want
to; but then you'd have to replace the ret in the error_setg() by fd (it
should be an error_setg_errno() again, anyway). Do as you please.
> 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(errp,
> + "Failed to write snapshot's inode: %d (%s)",
> + ret, strerror(-ret));
> goto cleanup;
> }
>
> ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &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));
This and the following hunk are actually places, where error_setg()
seems better than error_setg_errno() (because it's unclear whether -ret
or errno contains the true error value).
Max
> goto cleanup;
> }
>
> @@ -2019,7 +2027,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;
> }
>
> @@ -2029,7 +2040,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 4f76e28..c4e7124 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1077,7 +1077,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);
> @@ -1135,11 +1135,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 a48731d..553d843 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -161,8 +161,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 926f0a0..aa8b46f 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);
> }
> }
> }
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create
2013-11-02 12:39 ` Max Reitz
@ 2013-11-04 1:47 ` Wenchao Xia
2013-11-04 21:32 ` Max Reitz
0 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-11-04 1:47 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, pbonzini, stefanha
于 2013/11/2 20:39, Max Reitz 写道:
> On 14.10.2013 23:52, Wenchao Xia wrote:
>> 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 | 27 +++++++++++++++++++++++----
>> block/qcow2.h | 4 +++-
>> block/rbd.c | 21 ++++++++++++---------
>> block/sheepdog.c | 29 ++++++++++++++++++++---------
>> 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, 95 insertions(+), 47 deletions(-)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 3529c68..b373f9a 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 */
>> @@ -383,6 +386,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
>> if (l1_table_offset < 0) {
>> ret = l1_table_offset;
>> + error_setg(errp,
>> + "Failed in allocation of snapshot L1 table: %d (%s)",
>> + ret, strerror(-ret));
>
> You may want to use error_setg_errno instead (here and in many other
> places in this patch).
>
I forgot we have error_setg_errno(), will use it.
>> goto fail;
>> }
>>
>> @@ -397,12 +403,20 @@ 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(errp, "Failed in overlap check for snapshot L1 table at %"
>> + PRIu64 " with size %" PRIu64 ": %d (%s)",
>> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
>> + ret, strerror(-ret));
>
> s->l1_size * sizeof(uint64_t) is of the same type as uint64_t (PRIu64)
> only on 64 bit systems. On 32 bit systems, it probably is the same as
> uint32_t (because of a different size_t width).
I haven't a 32 bit system at hand to test, do you think
(int64_t)(s->l1_size * sizeof(uint64_t))
could work for both 32 bit and 64 bit system?
>
>> goto fail;
>> }
>>
>> ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>> s->l1_size * sizeof(uint64_t));
>> if (ret < 0) {
>> + error_setg(errp, "Failed in update of snapshot L1 table at %"
>> + PRIu64 " with size %" PRIu64 ": %d (%s)",
>> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
>> + ret, strerror(-ret));
>
> Same here.
>
>> goto fail;
>> }
>>
>> @@ -416,6 +430,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> */
>> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
>> if (ret < 0) {
>> + error_setg(errp, "Failed in update of refcount for snapshot at %"
>> + PRIu64 " with size %d: %d (%s)",
>> + s->l1_table_offset, s->l1_size, ret, strerror(-ret));
>> goto fail;
>> }
>>
>> @@ -431,6 +448,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 +471,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..6181999 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 need a valid name");
>
> *needs (or "requires")
>
will change.
>> + return; /* we need a name for rbd snapshots */
>> }
>>
>> /*
>> @@ -876,20 +878,21 @@ 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(errp,
>> + "Failed to create snapshot: %d (%s)",
>> + r, strerror(-r));
>> }
>> -
>> - return 0;
>> }
>>
>> static int qemu_rbd_snap_remove(BlockDriverState *bs,
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 5f81c93..67aaeb7 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -1961,7 +1961,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;
>> @@ -1974,10 +1976,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);
>> @@ -1995,21 +1997,27 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> fd = connect_to_sdog(s);
>> if (fd < 0) {
>> ret = fd;
>> + error_setg(errp, "Failed to connect: %d (%s)", ret, strerror(-ret));
>
> You may remove the "ret = fd" line before the error_setg(), if you want
> to; but then you'd have to replace the ret in the error_setg() by fd (it
> should be an error_setg_errno() again, anyway). Do as you please.
>
The function is return void now, it seems "ret = fd" can be
removed, will do that.
>> 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(errp,
>> + "Failed to write snapshot's inode: %d (%s)",
>> + ret, strerror(-ret));
>> goto cleanup;
>> }
>>
>> ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &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));
>
> This and the following hunk are actually places, where error_setg()
> seems better than error_setg_errno() (because it's unclear whether -ret
> or errno contains the true error value).
>
I guess change it as:
+ error_setg(errp,
+ "Failed to create inode for snapshot, return is %d",
+ ret);
?
> Max
>
>> goto cleanup;
>> }
>>
>> @@ -2019,7 +2027,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;
>> }
>>
>> @@ -2029,7 +2040,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 4f76e28..c4e7124 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1077,7 +1077,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);
>> @@ -1135,11 +1135,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 a48731d..553d843 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -161,8 +161,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 926f0a0..aa8b46f 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);
>> }
>> }
>> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create
2013-11-04 1:47 ` Wenchao Xia
@ 2013-11-04 21:32 ` Max Reitz
0 siblings, 0 replies; 21+ messages in thread
From: Max Reitz @ 2013-11-04 21:32 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, stefanha
On 04.11.2013 02:47, Wenchao Xia wrote:
> 于 2013/11/2 20:39, Max Reitz 写道:
>> On 14.10.2013 23:52, Wenchao Xia wrote:
>>> 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 | 27 +++++++++++++++++++++++----
>>> block/qcow2.h | 4 +++-
>>> block/rbd.c | 21 ++++++++++++---------
>>> block/sheepdog.c | 29 ++++++++++++++++++++---------
>>> 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, 95 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>>> index 3529c68..b373f9a 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 */
>>> @@ -383,6 +386,9 @@ int qcow2_snapshot_create(BlockDriverState *bs,
>>> QEMUSnapshotInfo *sn_info)
>>> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size *
>>> sizeof(uint64_t));
>>> if (l1_table_offset < 0) {
>>> ret = l1_table_offset;
>>> + error_setg(errp,
>>> + "Failed in allocation of snapshot L1 table: %d
>>> (%s)",
>>> + ret, strerror(-ret));
>>
>> You may want to use error_setg_errno instead (here and in many other
>> places in this patch).
>>
>
> I forgot we have error_setg_errno(), will use it.
>
>>> goto fail;
>>> }
>>>
>>> @@ -397,12 +403,20 @@ 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(errp, "Failed in overlap check for snapshot L1
>>> table at %"
>>> + PRIu64 " with size %" PRIu64 ": %d (%s)",
>>> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
>>> + ret, strerror(-ret));
>>
>> s->l1_size * sizeof(uint64_t) is of the same type as uint64_t (PRIu64)
>> only on 64 bit systems. On 32 bit systems, it probably is the same as
>> uint32_t (because of a different size_t width).
>
> I haven't a 32 bit system at hand to test, do you think
> (int64_t)(s->l1_size * sizeof(uint64_t))
> could work for both 32 bit and 64 bit system?
Yes, that should work (although uint64_t would be more appropriate for
PRIu64).
>
>>
>>> goto fail;
>>> }
>>>
>>> ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>>> s->l1_size * sizeof(uint64_t));
>>> if (ret < 0) {
>>> + error_setg(errp, "Failed in update of snapshot L1 table at %"
>>> + PRIu64 " with size %" PRIu64 ": %d (%s)",
>>> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
>>> + ret, strerror(-ret));
>>
>> Same here.
>>
>>> goto fail;
>>> }
>>>
>>> @@ -416,6 +430,9 @@ int qcow2_snapshot_create(BlockDriverState *bs,
>>> QEMUSnapshotInfo *sn_info)
>>> */
>>> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>>> s->l1_size, 1);
>>> if (ret < 0) {
>>> + error_setg(errp, "Failed in update of refcount for snapshot
>>> at %"
>>> + PRIu64 " with size %d: %d (%s)",
>>> + s->l1_table_offset, s->l1_size, ret,
>>> strerror(-ret));
>>> goto fail;
>>> }
>>>
>>> @@ -431,6 +448,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 +471,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..6181999 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 need a valid name");
>>
>> *needs (or "requires")
>>
>
> will change.
>
>>> + return; /* we need a name for rbd snapshots */
>>> }
>>>
>>> /*
>>> @@ -876,20 +878,21 @@ 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(errp,
>>> + "Failed to create snapshot: %d (%s)",
>>> + r, strerror(-r));
>>> }
>>> -
>>> - return 0;
>>> }
>>>
>>> static int qemu_rbd_snap_remove(BlockDriverState *bs,
>>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>>> index 5f81c93..67aaeb7 100644
>>> --- a/block/sheepdog.c
>>> +++ b/block/sheepdog.c
>>> @@ -1961,7 +1961,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;
>>> @@ -1974,10 +1976,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);
>>> @@ -1995,21 +1997,27 @@ static int
>>> sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>> fd = connect_to_sdog(s);
>>> if (fd < 0) {
>>> ret = fd;
>>> + error_setg(errp, "Failed to connect: %d (%s)", ret,
>>> strerror(-ret));
>>
>> You may remove the "ret = fd" line before the error_setg(), if you want
>> to; but then you'd have to replace the ret in the error_setg() by fd (it
>> should be an error_setg_errno() again, anyway). Do as you please.
>>
>
> The function is return void now, it seems "ret = fd" can be
> removed, will do that.
>
>>> 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(errp,
>>> + "Failed to write snapshot's inode: %d (%s)",
>>> + ret, strerror(-ret));
>>> goto cleanup;
>>> }
>>>
>>> ret = do_sd_create(s, s->name, s->inode.vdi_size,
>>> s->inode.vdi_id, &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));
>>
>> This and the following hunk are actually places, where error_setg()
>> seems better than error_setg_errno() (because it's unclear whether -ret
>> or errno contains the true error value).
>>
> I guess change it as:
> + error_setg(errp,
> + "Failed to create inode for snapshot, return is %d",
> + ret);
>
> ?
No, I think you should leave the hunk as it is. Apparently we don't know
whether the error value will be returned in ret or errno, so we need do
display both. I was just commenting on that you should indeed leave this
and the following hunk as they are and not replace them with
error_setg_errno(), since they actually have to display two error values.
Max
>
>> Max
>>
>>> goto cleanup;
>>> }
>>>
>>> @@ -2019,7 +2027,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;
>>> }
>>>
>>> @@ -2029,7 +2040,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 4f76e28..c4e7124 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1077,7 +1077,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);
>>> @@ -1135,11 +1135,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 a48731d..553d843 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -161,8 +161,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 926f0a0..aa8b46f 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);
>>> }
>>> }
>>> }
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots()
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create Wenchao Xia
@ 2013-10-14 21:52 ` Wenchao Xia
2013-11-02 12:52 ` Max Reitz
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots Wenchao Xia
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-10-14 21:52 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz, pbonzini, 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 | 48 ++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b373f9a..4bd494b 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,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
offset = snapshots_offset;
if (offset < 0) {
ret = offset;
+ error_setg(errp,
+ "Failed in allocation of cluster for snapshot list: "
+ "%d (%s)",
+ ret, strerror(-ret));
goto fail;
}
ret = bdrv_flush(bs);
if (ret < 0) {
+ error_setg(errp,
+ "Failed in flush after snapshot list cluster allocation: "
+ "%d (%s)",
+ ret, strerror(-ret));
goto fail;
}
@@ -194,6 +202,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(errp,
+ "Failed in overlap check for snapshot list cluster at %"
+ PRIi64 " with size %d: %d (%s)",
+ offset, snapshots_size, ret, strerror(-ret));
goto fail;
}
@@ -227,24 +239,40 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
if (ret < 0) {
+ error_setg(errp,
+ "Failed in write of snapshot header at %"
+ PRIi64 " with size %" PRIu64 ": %d (%s)",
+ offset, sizeof(h), ret, strerror(-ret));
goto fail;
}
offset += sizeof(h);
ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra));
if (ret < 0) {
+ error_setg(errp,
+ "Failed in write of extra snapshot data at %"
+ PRIi64 " with size %" PRIu64 ": %d (%s)",
+ offset, sizeof(extra), ret, strerror(-ret));
goto fail;
}
offset += sizeof(extra);
ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
if (ret < 0) {
+ error_setg(errp,
+ "Failed in write of snapshot id string at %"
+ PRIi64 " with size %d: %d (%s)",
+ offset, id_str_size, ret, strerror(-ret));
goto fail;
}
offset += id_str_size;
ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
if (ret < 0) {
+ error_setg(errp,
+ "Failed in write of snapshot name string at %"
+ PRIi64 " with size %d: %d (%s)",
+ offset, name_size, ret, strerror(-ret));
goto fail;
}
offset += name_size;
@@ -256,6 +284,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
*/
ret = bdrv_flush(bs);
if (ret < 0) {
+ error_setg(errp,
+ "Failed in flush after snapshot list update: %d (%s)",
+ ret, strerror(-ret));
goto fail;
}
@@ -268,6 +299,11 @@ 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(errp,
+ "Failed in update of image header at %"
+ PRIi64 " with size %" PRIu64 ":%d (%s)",
+ offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
+ ret, strerror(-ret));
goto fail;
}
@@ -283,6 +319,9 @@ fail:
qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
QCOW2_DISCARD_ALWAYS);
}
+ if (errp) {
+ g_assert(error_is_set(errp));
+ }
return ret;
}
@@ -446,10 +485,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--;
@@ -623,9 +660,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots()
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots() Wenchao Xia
@ 2013-11-02 12:52 ` Max Reitz
2013-11-04 1:48 ` Wenchao Xia
0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2013-11-02 12:52 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, stefanha
On 14.10.2013 23:52, Wenchao Xia wrote:
> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++------
> 1 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index b373f9a..4bd494b 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,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> offset = snapshots_offset;
> if (offset < 0) {
> ret = offset;
> + error_setg(errp,
> + "Failed in allocation of cluster for snapshot list: "
> + "%d (%s)",
> + ret, strerror(-ret));
Again, error_setg_errno() is your friend.
> goto fail;
> }
> ret = bdrv_flush(bs);
> if (ret < 0) {
> + error_setg(errp,
> + "Failed in flush after snapshot list cluster allocation: "
> + "%d (%s)",
> + ret, strerror(-ret));
> goto fail;
> }
>
> @@ -194,6 +202,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(errp,
> + "Failed in overlap check for snapshot list cluster at %"
> + PRIi64 " with size %d: %d (%s)",
> + offset, snapshots_size, ret, strerror(-ret));
> goto fail;
> }
>
> @@ -227,24 +239,40 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>
> ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
> if (ret < 0) {
> + error_setg(errp,
> + "Failed in write of snapshot header at %"
> + PRIi64 " with size %" PRIu64 ": %d (%s)",
> + offset, sizeof(h), ret, strerror(-ret));
Again, on 32 bit systems, sizeof(size_t) is generally 4 and not 8 (you
may want to cast sizeof(h) to int and just use %d).
> goto fail;
> }
> offset += sizeof(h);
>
> ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra));
> if (ret < 0) {
> + error_setg(errp,
> + "Failed in write of extra snapshot data at %"
> + PRIi64 " with size %" PRIu64 ": %d (%s)",
> + offset, sizeof(extra), ret, strerror(-ret));
Same here.
> goto fail;
> }
> offset += sizeof(extra);
>
> ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
> if (ret < 0) {
> + error_setg(errp,
> + "Failed in write of snapshot id string at %"
> + PRIi64 " with size %d: %d (%s)",
> + offset, id_str_size, ret, strerror(-ret));
> goto fail;
> }
> offset += id_str_size;
>
> ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
> if (ret < 0) {
> + error_setg(errp,
> + "Failed in write of snapshot name string at %"
> + PRIi64 " with size %d: %d (%s)",
> + offset, name_size, ret, strerror(-ret));
> goto fail;
> }
> offset += name_size;
> @@ -256,6 +284,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> */
> ret = bdrv_flush(bs);
> if (ret < 0) {
> + error_setg(errp,
> + "Failed in flush after snapshot list update: %d (%s)",
> + ret, strerror(-ret));
> goto fail;
> }
>
> @@ -268,6 +299,11 @@ 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(errp,
> + "Failed in update of image header at %"
> + PRIi64 " with size %" PRIu64 ":%d (%s)",
> + offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
> + ret, strerror(-ret));
And here (also applies to offsetof(), which returns size_t as well).
Also, you forgot a space after the colon (although you should be using
error_setg_errno() anyway).
Max
> goto fail;
> }
>
> @@ -283,6 +319,9 @@ fail:
> qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
> QCOW2_DISCARD_ALWAYS);
> }
> + if (errp) {
> + g_assert(error_is_set(errp));
> + }
> return ret;
> }
>
> @@ -446,10 +485,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--;
> @@ -623,9 +660,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;
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots()
2013-11-02 12:52 ` Max Reitz
@ 2013-11-04 1:48 ` Wenchao Xia
2013-11-04 19:46 ` Eric Blake
0 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-11-04 1:48 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, pbonzini, stefanha
> On 14.10.2013 23:52, Wenchao Xia wrote:
>> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++------
>> 1 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index b373f9a..4bd494b 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,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>> offset = snapshots_offset;
>> if (offset < 0) {
>> ret = offset;
>> + error_setg(errp,
>> + "Failed in allocation of cluster for snapshot list: "
>> + "%d (%s)",
>> + ret, strerror(-ret));
>
> Again, error_setg_errno() is your friend.
>
>> goto fail;
>> }
>> ret = bdrv_flush(bs);
>> if (ret < 0) {
>> + error_setg(errp,
>> + "Failed in flush after snapshot list cluster allocation: "
>> + "%d (%s)",
>> + ret, strerror(-ret));
>> goto fail;
>> }
>>
>> @@ -194,6 +202,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(errp,
>> + "Failed in overlap check for snapshot list cluster at %"
>> + PRIi64 " with size %d: %d (%s)",
>> + offset, snapshots_size, ret, strerror(-ret));
>> goto fail;
>> }
>>
>> @@ -227,24 +239,40 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>>
>> ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
>> if (ret < 0) {
>> + error_setg(errp,
>> + "Failed in write of snapshot header at %"
>> + PRIi64 " with size %" PRIu64 ": %d (%s)",
>> + offset, sizeof(h), ret, strerror(-ret));
>
> Again, on 32 bit systems, sizeof(size_t) is generally 4 and not 8 (you
> may want to cast sizeof(h) to int and just use %d).
>
Maybe caset as (int64_t)sizeof(h) to avoid possible incomplete value?
>> goto fail;
>> }
>> offset += sizeof(h);
>>
>> ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra));
>> if (ret < 0) {
>> + error_setg(errp,
>> + "Failed in write of extra snapshot data at %"
>> + PRIi64 " with size %" PRIu64 ": %d (%s)",
>> + offset, sizeof(extra), ret, strerror(-ret));
>
> Same here.
>
>> goto fail;
>> }
>> offset += sizeof(extra);
>>
>> ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
>> if (ret < 0) {
>> + error_setg(errp,
>> + "Failed in write of snapshot id string at %"
>> + PRIi64 " with size %d: %d (%s)",
>> + offset, id_str_size, ret, strerror(-ret));
>> goto fail;
>> }
>> offset += id_str_size;
>>
>> ret = bdrv_pwrite(bs->file, offset, sn->name, name_size);
>> if (ret < 0) {
>> + error_setg(errp,
>> + "Failed in write of snapshot name string at %"
>> + PRIi64 " with size %d: %d (%s)",
>> + offset, name_size, ret, strerror(-ret));
>> goto fail;
>> }
>> offset += name_size;
>> @@ -256,6 +284,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>> */
>> ret = bdrv_flush(bs);
>> if (ret < 0) {
>> + error_setg(errp,
>> + "Failed in flush after snapshot list update: %d (%s)",
>> + ret, strerror(-ret));
>> goto fail;
>> }
>>
>> @@ -268,6 +299,11 @@ 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(errp,
>> + "Failed in update of image header at %"
>> + PRIi64 " with size %" PRIu64 ":%d (%s)",
>> + offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
>> + ret, strerror(-ret));
>
> And here (also applies to offsetof(), which returns size_t as well).
> Also, you forgot a space after the colon (although you should be using
> error_setg_errno() anyway).
>
Will use error_setg_errno().
> Max
>
>> goto fail;
>> }
>>
>> @@ -283,6 +319,9 @@ fail:
>> qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
>> QCOW2_DISCARD_ALWAYS);
>> }
>> + if (errp) {
>> + g_assert(error_is_set(errp));
>> + }
>> return ret;
>> }
>>
>> @@ -446,10 +485,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--;
>> @@ -623,9 +660,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;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots()
2013-11-04 1:48 ` Wenchao Xia
@ 2013-11-04 19:46 ` Eric Blake
2013-11-05 2:19 ` Wenchao Xia
0 siblings, 1 reply; 21+ messages in thread
From: Eric Blake @ 2013-11-04 19:46 UTC (permalink / raw)
To: Wenchao Xia, Max Reitz, qemu-devel; +Cc: kwolf, pbonzini, stefanha
[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]
On 11/03/2013 06:48 PM, Wenchao Xia wrote:
>> On 14.10.2013 23:52, Wenchao Xia wrote:
>>> The function still returns int since qcow2_snapshot_delete() will
>>> return the number.
>>>
>>> @@ -227,24 +239,40 @@ static int
>>> qcow2_write_snapshots(BlockDriverState *bs)
>>>
>>> ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
>>> if (ret < 0) {
>>> + error_setg(errp,
>>> + "Failed in write of snapshot header at %"
>>> + PRIi64 " with size %" PRIu64 ": %d (%s)",
>>> + offset, sizeof(h), ret, strerror(-ret));
>>
>> Again, on 32 bit systems, sizeof(size_t) is generally 4 and not 8 (you
>> may want to cast sizeof(h) to int and just use %d).
>>
>
> Maybe caset as (int64_t)sizeof(h) to avoid possible incomplete value?
If we have objects larger than 2G (such that sizeof(h) would require
more than 31 bits), we have other issues to worry about. Casting to int
and using %d is safe enough.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots()
2013-11-04 19:46 ` Eric Blake
@ 2013-11-05 2:19 ` Wenchao Xia
0 siblings, 0 replies; 21+ messages in thread
From: Wenchao Xia @ 2013-11-05 2:19 UTC (permalink / raw)
To: Eric Blake, Max Reitz, qemu-devel; +Cc: kwolf, pbonzini, stefanha
于 2013/11/5 3:46, Eric Blake 写道:
> On 11/03/2013 06:48 PM, Wenchao Xia wrote:
>>> On 14.10.2013 23:52, Wenchao Xia wrote:
>>>> The function still returns int since qcow2_snapshot_delete() will
>>>> return the number.
>>>>
>
>
>>>> @@ -227,24 +239,40 @@ static int
>>>> qcow2_write_snapshots(BlockDriverState *bs)
>>>>
>>>> ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
>>>> if (ret < 0) {
>>>> + error_setg(errp,
>>>> + "Failed in write of snapshot header at %"
>>>> + PRIi64 " with size %" PRIu64 ": %d (%s)",
>>>> + offset, sizeof(h), ret, strerror(-ret));
>>>
>>> Again, on 32 bit systems, sizeof(size_t) is generally 4 and not 8 (you
>>> may want to cast sizeof(h) to int and just use %d).
>>>
>>
>> Maybe caset as (int64_t)sizeof(h) to avoid possible incomplete value?
>
> If we have objects larger than 2G (such that sizeof(h) would require
> more than 31 bits), we have other issues to worry about. Casting to int
> and using %d is safe enough.
>
OK, will use %d.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create Wenchao Xia
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots() Wenchao Xia
@ 2013-10-14 21:52 ` Wenchao Xia
2013-11-02 13:04 ` Max Reitz
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create() Wenchao Xia
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-10-14 21:52 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz, pbonzini, Wenchao Xia
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qcow2-snapshot.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4bd494b..c933b7f 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -304,6 +304,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
PRIi64 " with size %" PRIu64 ":%d (%s)",
offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
ret, strerror(-ret));
+ /*
+ * If the snapshot data part have 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 for safe.
+ */
+ snapshots_offset = 0;
goto fail;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots Wenchao Xia
@ 2013-11-02 13:04 ` Max Reitz
2013-11-02 13:56 ` Max Reitz
0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2013-11-02 13:04 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, stefanha
On 14.10.2013 23:52, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qcow2-snapshot.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 4bd494b..c933b7f 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -304,6 +304,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
> PRIi64 " with size %" PRIu64 ":%d (%s)",
> offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
> ret, strerror(-ret));
> + /*
> + * If the snapshot data part have been updated on disk, Then the
s/have/has/; s/Then/then/
> + * 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 for safe.
s/for/to be/
> + */
> + snapshots_offset = 0;
> goto fail;
> }
Other then that: Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots
2013-11-02 13:04 ` Max Reitz
@ 2013-11-02 13:56 ` Max Reitz
2013-11-04 1:51 ` Wenchao Xia
0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2013-11-02 13:56 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, stefanha
On 02.11.2013 14:04, Max Reitz wrote:
> On 14.10.2013 23:52, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> block/qcow2-snapshot.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 4bd494b..c933b7f 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -304,6 +304,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>> PRIi64 " with size %" PRIu64 ":%d (%s)",
>> offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
>> ret, strerror(-ret));
>> + /*
>> + * If the snapshot data part have been updated on disk, Then the
> s/have/has/; s/Then/then/
>
>> + * 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 for safe.
> s/for/to be/
>
>> + */
>> + snapshots_offset = 0;
>> goto fail;
>> }
> Other then that: Reviewed-by: Max Reitz <mreitz@redhat.com>
*than, of course; if I am already correcting other people's orthography,
I have to do the same for mine. ;-)
Max
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots
2013-11-02 13:56 ` Max Reitz
@ 2013-11-04 1:51 ` Wenchao Xia
0 siblings, 0 replies; 21+ messages in thread
From: Wenchao Xia @ 2013-11-04 1:51 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, pbonzini, stefanha
于 2013/11/2 21:56, Max Reitz 写道:
> On 02.11.2013 14:04, Max Reitz wrote:
>> On 14.10.2013 23:52, Wenchao Xia wrote:
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>> block/qcow2-snapshot.c | 7 +++++++
>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>>> index 4bd494b..c933b7f 100644
>>> --- a/block/qcow2-snapshot.c
>>> +++ b/block/qcow2-snapshot.c
>>> @@ -304,6 +304,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>>> PRIi64 " with size %" PRIu64 ":%d (%s)",
>>> offsetof(QCowHeader, nb_snapshots), sizeof(header_data),
>>> ret, strerror(-ret));
>>> + /*
>>> + * If the snapshot data part have been updated on disk, Then the
>> s/have/has/; s/Then/then/
>>
>>> + * 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 for safe.
>> s/for/to be/
>>
>>> + */
>>> + snapshots_offset = 0;
>>> goto fail;
>>> }
>> Other then that: Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> *than, of course; if I am already correcting other people's orthography,
> I have to do the same for mine. ;-)
>
> Max
>
Thanks for reviewing, I'll fix the issues you pointed out and respin.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH V4 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create()
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
` (2 preceding siblings ...)
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots Wenchao Xia
@ 2013-10-14 21:52 ` Wenchao Xia
2013-11-02 13:11 ` Max Reitz
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 5/6] blkdebug: add debug events for snapshot Wenchao Xia
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-10-14 21:52 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz, pbonzini, Wenchao Xia
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.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 c933b7f..6d0ffd4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -404,6 +404,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));
@@ -453,7 +454,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
PRIu64 " with size %" PRIu64 ": %d (%s)",
sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
ret, strerror(-ret));
- goto fail;
+ goto dealloc_cluster;
}
ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
@@ -463,7 +464,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
PRIu64 " with size %" PRIu64 ": %d (%s)",
sn->l1_table_offset, s->l1_size * sizeof(uint64_t),
ret, strerror(-ret));
- goto fail;
+ goto dealloc_cluster;
}
g_free(l1_table);
@@ -479,7 +480,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
error_setg(errp, "Failed in update of refcount for snapshot at %"
PRIu64 " with size %d: %d (%s)",
s->l1_table_offset, s->l1_size, ret, strerror(-ret));
- goto fail;
+ goto dealloc_cluster;
}
/* Append the new snapshot to the snapshot list */
@@ -497,7 +498,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);
@@ -517,6 +518,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH V4 5/6] blkdebug: add debug events for snapshot
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
` (3 preceding siblings ...)
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create() Wenchao Xia
@ 2013-10-14 21:52 ` Wenchao Xia
2013-11-02 13:20 ` Max Reitz
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 6/6] qemu-iotests: add test for qcow2 snapshot Wenchao Xia
2013-11-01 1:35 ` [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
6 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-10-14 21:52 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz, pbonzini, Wenchao Xia
Some code in qcow2-snapshot.c directly access bs->file, so in those
points error can't be injected by other events. Since the code in
qcow2-snapshot.c is qcow2's internal detail similar as L1 table,
so add some debug events.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/blkdebug.c | 4 ++++
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 6d0ffd4..4b313cc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -210,6 +210,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;
@@ -296,6 +297,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) {
@@ -457,6 +459,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 5/6] blkdebug: add debug events for snapshot
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 5/6] blkdebug: add debug events for snapshot Wenchao Xia
@ 2013-11-02 13:20 ` Max Reitz
0 siblings, 0 replies; 21+ messages in thread
From: Max Reitz @ 2013-11-02 13:20 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, stefanha
On 14.10.2013 23:52, Wenchao Xia wrote:
> Some code in qcow2-snapshot.c directly access bs->file, so in those
s/access/accesses/
> points error can't be injected by other events. Since the code in
Perhaps "places" instead of "points"? (And s/error/errors/)
> qcow2-snapshot.c is qcow2's internal detail similar as L1 table,
Maybe something like "qcow2-snapshot.c is similar to the other qcow2
internal code (in regards to e.g. the L1 table)" would be easier to
parse, although I'm not sure whether it has the same meaning you wanted
to convey.
> so add some debug events.
Maybe remove the "so", because you already had a "since" in the previous
subordinate clause?
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
Aside from that: Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH V4 6/6] qemu-iotests: add test for qcow2 snapshot
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
` (4 preceding siblings ...)
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 5/6] blkdebug: add debug events for snapshot Wenchao Xia
@ 2013-10-14 21:52 ` Wenchao Xia
2013-11-02 13:34 ` Max Reitz
2013-11-01 1:35 ` [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
6 siblings, 1 reply; 21+ messages in thread
From: Wenchao Xia @ 2013-10-14 21:52 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz, pbonzini, 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/068 | 214 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/068.out | 35 ++++++
tests/qemu-iotests/common.filter | 7 ++
tests/qemu-iotests/group | 1 +
4 files changed, 257 insertions(+), 0 deletions(-)
create mode 100755 tests/qemu-iotests/068
create mode 100644 tests/qemu-iotests/068.out
diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
new file mode 100755
index 0000000..37ada84
--- /dev/null
+++ b/tests/qemu-iotests/068
@@ -0,0 +1,214 @@
+#!/bin/bash
+#
+# qcow2 internal snapshot test
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+owner=xiawenc@linux.vnet.ibm.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm $TEST_DIR/blkdebug.conf
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# only test qcow2
+_supported_fmt qcow2
+_supported_proto generic
+# 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:$TEST_DIR/blkdebug.conf:$TEST_IMG"
+
+errno=5
+
+once=on
+
+imm=off
+
+
+# Start test, note that the injected errors are related to qcow2's snapshot
+# logic closely, see qcow2-snapshot.c for more details.
+
+# path 1: fail in L1 table allocation for snapshot
+echo
+echo "Path 1: fail in allocation of L1 table"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "cluster_alloc"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG
+$QEMU_IMG snapshot -l $TEST_IMG
+_check_test_img 2>&1
+
+
+# path 2: fail in update new L1 table
+echo
+echo "Path 2: fail in update new L1 table for snapshot"
+
+_make_test_img $SIZE
+
+cat > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "snapshot_l1_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG 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 > $TEST_DIR/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 > $TEST_DIR/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 > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "snapshot_list_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG 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 > $TEST_DIR/blkdebug.conf <<EOF
+[inject-error]
+event = "snapshot_header_update"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+$QEMU_IMG snapshot -c snap1 $BLKDBG_TEST_IMG 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/068.out b/tests/qemu-iotests/068.out
new file mode 100644
index 0000000..2e111c9
--- /dev/null
+++ b/tests/qemu-iotests/068.out
@@ -0,0 +1,35 @@
+QA output created by 068
+
+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: -5 (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: -5 (Input/output error)
+No errors were found on the image.
+
+Path 3: fail in update refcount block before write snapshot list
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+qemu-img: Could not create snapshot 'snap1': Failed in update of refcount for snapshot at X with size X: -5 (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: -5 (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: -5 (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 13c5500..3ca9cba 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -73,3 +73,4 @@
065 rw auto
066 rw auto
067 rw auto
+068 rw auto
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation
2013-10-14 21:52 [Qemu-devel] [PATCH V4 0/6] qcow2: rollback the modification on fail in snapshot creation Wenchao Xia
` (5 preceding siblings ...)
2013-10-14 21:52 ` [Qemu-devel] [PATCH V4 6/6] qemu-iotests: add test for qcow2 snapshot Wenchao Xia
@ 2013-11-01 1:35 ` Wenchao Xia
6 siblings, 0 replies; 21+ messages in thread
From: Wenchao Xia @ 2013-11-01 1:35 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: kwolf, pbonzini, mreitz, stefanha
于 2013/10/15 5:52, 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.
>
> 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 | 106 +++++++++++++++++--
> block/qcow2.h | 4 +-
> block/rbd.c | 21 ++--
> block/sheepdog.c | 29 ++++--
> 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/068 | 214 ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/068.out | 35 ++++++
> tests/qemu-iotests/common.filter | 7 ++
> tests/qemu-iotests/group | 1 +
> 16 files changed, 431 insertions(+), 55 deletions(-)
> create mode 100755 tests/qemu-iotests/068
> create mode 100644 tests/qemu-iotests/068.out
>
Hello, any comments?
^ permalink raw reply [flat|nested] 21+ messages in thread