* [Qemu-devel] [PATCH V5 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name()
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
@ 2013-07-11 5:46 ` Wenchao Xia
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
` (8 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:46 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.
Some code are modified based on Pavel's patch.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block/snapshot.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/snapshot.h | 6 ++++
2 files changed, 79 insertions(+), 0 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..481a3ee 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,79 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
return ret;
}
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
+{
+ QEMUSnapshotInfo *sn_tab, *sn;
+ int nb_sns, i;
+ bool ret = false;
+
+ assert(id || name);
+
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ if (nb_sns < 0) {
+ error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+ return false;
+ } else if (nb_sns == 0) {
+ return false;
+ }
+
+ if (id && name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ } else if (id) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ } else if (name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ }
+
+ g_free(sn_tab);
+ return ret;
+}
+
int bdrv_can_snapshot(BlockDriverState *bs)
{
BlockDriver *drv = bs->drv;
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index eaf61f0..9d06dc1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@
#define SNAPSHOT_H
#include "qemu-common.h"
+#include "qapi/error.h"
typedef struct QEMUSnapshotInfo {
char id_str[128]; /* unique snapshot id */
@@ -40,6 +41,11 @@ typedef struct QEMUSnapshotInfo {
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name);
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name() Wenchao Xia
@ 2013-07-11 5:46 ` Wenchao Xia
2013-07-18 11:07 ` Stefan Hajnoczi
2013-07-18 11:48 ` Kevin Wolf
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction Wenchao Xia
` (7 subsequent siblings)
9 siblings, 2 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:46 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Snapshot creation actually already distinguish id and name since it take
a structured parameter *sn, but delete can't. Later an accurate delete
is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
so change its prototype. Also *errp is added to tip error, but return
value is kepted to let caller check what kind of error happens. Existing
caller for it are savevm, delvm and qemu-img, they are not impacted by
check the return value and do the operations again.
Before this patch:
For qcow2, it search id first then name to find the one to delete.
For rbd, it search name.
For sheepdog, it does nothing.
After this patch:
For qcow2, logic is the same by call it twice in caller.
For rbd, it always fails in delete with id, but still search for name
in second try, no change for user.
Some code for *errp is based on Pavel's patch.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block/qcow2-snapshot.c | 66 ++++++++++++++++++++++++++++++++++-----------
block/qcow2.h | 5 +++-
block/rbd.c | 23 +++++++++++++++-
block/sheepdog.c | 5 +++-
block/snapshot.c | 36 ++++++++++++++++++++++--
include/block/block_int.h | 5 +++-
include/block/snapshot.h | 5 +++-
include/qemu-common.h | 3 ++
qemu-img.c | 5 +++-
savevm.c | 10 +++++-
10 files changed, 136 insertions(+), 27 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0caac90..47db6ad 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -288,31 +288,47 @@ static void find_new_snapshot_id(BlockDriverState *bs,
snprintf(id_str, id_str_size, "%d", id_max + 1);
}
-static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
+static int find_snapshot_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name)
{
BDRVQcowState *s = bs->opaque;
int i;
- for(i = 0; i < s->nb_snapshots; i++) {
- if (!strcmp(s->snapshots[i].id_str, id_str))
- return i;
+ if (id && name) {
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (!strcmp(s->snapshots[i].id_str, id) &&
+ !strcmp(s->snapshots[i].name, name)) {
+ return i;
+ }
+ }
+ } else if (id) {
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (!strcmp(s->snapshots[i].id_str, id)) {
+ return i;
+ }
+ }
+ } else if (name) {
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (!strcmp(s->snapshots[i].name, name)) {
+ return i;
+ }
+ }
}
+
return -1;
}
-static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
+static int find_snapshot_by_id_or_name(BlockDriverState *bs,
+ const char *id_or_name)
{
- BDRVQcowState *s = bs->opaque;
- int i, ret;
+ int ret;
- ret = find_snapshot_by_id(bs, name);
- if (ret >= 0)
+ ret = find_snapshot_by_id_and_name(bs, id_or_name, NULL);
+ if (ret >= 0) {
return ret;
- for(i = 0; i < s->nb_snapshots; i++) {
- if (!strcmp(s->snapshots[i].name, name))
- return i;
}
- return -1;
+ return find_snapshot_by_id_and_name(bs, NULL, id_or_name);
}
/* if no id is provided, a new one is constructed */
@@ -334,7 +350,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
}
/* Check that the ID is unique */
- if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+ if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
return -EEXIST;
}
@@ -531,15 +547,23 @@ fail:
return ret;
}
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot sn;
int snapshot_index, ret;
+ const char *device = bdrv_get_device_name(bs);
/* Search the snapshot */
- snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
+ snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
if (snapshot_index < 0) {
+ error_setg(errp,
+ "Can't find a snapshot with ID '%s' and name '%s' "
+ "on device '%s'",
+ STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
return -ENOENT;
}
sn = s->snapshots[snapshot_index];
@@ -551,6 +575,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
s->nb_snapshots--;
ret = qcow2_write_snapshots(bs);
if (ret < 0) {
+ error_setg(errp,
+ "Failed to remove snapshot with ID '%s' and name '%s' from "
+ "the snapshot list on device '%s'",
+ STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
return ret;
}
@@ -568,6 +596,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
sn.l1_size, -1);
if (ret < 0) {
+ error_setg(errp,
+ "Failed to free the cluster and L1 table on device '%s'",
+ device);
return ret;
}
qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t),
@@ -576,6 +607,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
/* must update the copied flag on the current cluster offsets */
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
if (ret < 0) {
+ error_setg(errp,
+ "Failed to update snapshot status in disk on device '%s'",
+ device);
return ret;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b2d5cd..2e6c471 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -411,7 +411,10 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
/* qcow2-snapshot.c functions */
int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp);
int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
diff --git a/block/rbd.c b/block/rbd.c
index cb71751..688b571 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,33 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
}
static int qemu_rbd_snap_remove(BlockDriverState *bs,
- const char *snapshot_name)
+ const char *snapshot_id,
+ const char *snapshot_name,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
+ const char *device = bdrv_get_device_name(bs);
+
+ if (snapshot_id && snapshot_id[0] != '\0') {
+ error_setg(errp,
+ "rbd do not support snapshot id on device '%s'", device);
+ return -EINVAL;
+ }
+ /* then snapshot_id == NULL or it contain an empty line, ignore it */
+
+ if (!snapshot_name) {
+ error_setg(errp,
+ "rbd need a valid snapshot name on device '%s'", device);
+ return -EINVAL;
+ }
r = rbd_snap_remove(s->image, snapshot_name);
+ if (r < 0) {
+ error_setg_errno(errp, -r,
+ "Failed to remove snapshot '%s' on device '%s'",
+ snapshot_name, device);
+ }
return r;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b397b5b..7fdae71 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2089,7 +2089,10 @@ out:
return ret;
}
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
/* FIXME: Delete specified snapshot id. */
return 0;
diff --git a/block/snapshot.c b/block/snapshot.c
index 481a3ee..cf2f4ca 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -182,18 +182,48 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return -ENOTSUP;
}
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+/**
+ * Delete an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, delete the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, delete the first one with id
+ * @snapshot_id.
+ * If only @name is specified, delete the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If can't find one matching @id and @name, return -ENOENT.
+ * If @bs did not support internal snapshot, return -ENOTSUP. If @bs do not
+ * support parameter @snapshot_id or @name, return -EINVAL.
+ */
+int bdrv_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
if (!drv) {
+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
return -ENOMEDIUM;
}
+ if (!snapshot_id && !name) {
+ error_setg(errp, "snapshot_id and name are both NULL");
+ return -EINVAL;
+ }
if (drv->bdrv_snapshot_delete) {
- return drv->bdrv_snapshot_delete(bs, snapshot_id);
+ return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
}
if (bs->file) {
- return bdrv_snapshot_delete(bs->file, snapshot_id);
+ return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
}
+ error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+ drv->format_name, bdrv_get_device_name(bs),
+ "internal snapshot");
return -ENOTSUP;
}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..4499b8b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -166,7 +166,10 @@ struct BlockDriver {
QEMUSnapshotInfo *sn_info);
int (*bdrv_snapshot_goto)(BlockDriverState *bs,
const char *snapshot_id);
- int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
+ int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp);
int (*bdrv_snapshot_list)(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9d06dc1..e47e78d 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -51,7 +51,10 @@ int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
int bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp);
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f439738..06c777f 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
int64_t strtosz_suffix_unit(const char *nptr, char **end,
const char default_suffix, int64_t unit);
+/* used to print char* safely */
+#define STR_PRINT_CHAR(str) ((str) ? (str) : "null")
+
/* path.c */
void init_paths(const char *prefix);
const char *path(const char *pathname);
diff --git a/qemu-img.c b/qemu-img.c
index f8c97d3..05564d3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
- ret = bdrv_snapshot_delete(bs, snapshot_name);
+ ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
+ if (ret == -ENOENT || ret == -EINVAL) {
+ ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
+ }
if (ret) {
error_report("Could not delete snapshot '%s': %d (%s)",
snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index e0491e7..56afebb 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0)
{
- ret = bdrv_snapshot_delete(bs, name);
+ ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
+ if (ret == -ENOENT || ret == -EINVAL) {
+ ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
+ }
if (ret < 0) {
monitor_printf(mon,
"Error while deleting snapshot on '%s'\n",
@@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
bs1 = NULL;
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1)) {
- ret = bdrv_snapshot_delete(bs1, name);
+ ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
+ if (ret == -ENOENT || ret == -EINVAL) {
+ ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
+ }
if (ret < 0) {
if (ret == -ENOTSUP)
monitor_printf(mon,
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
@ 2013-07-18 11:07 ` Stefan Hajnoczi
2013-07-19 9:12 ` Wenchao Xia
2013-07-18 11:48 ` Kevin Wolf
1 sibling, 1 reply; 20+ messages in thread
From: Stefan Hajnoczi @ 2013-07-18 11:07 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
On Thu, Jul 11, 2013 at 01:46:58PM +0800, Wenchao Xia wrote:
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index f439738..06c777f 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
> int64_t strtosz_suffix_unit(const char *nptr, char **end,
> const char default_suffix, int64_t unit);
>
> +/* used to print char* safely */
> +#define STR_PRINT_CHAR(str) ((str) ? (str) : "null")
When I saw the name I thought it would filter out non-printable
characters. Maybe STR_OR_NULL() is a better name?
BTW the evil gcc shortcut is pretty quick to type: str ?: "null".
Besides this I'm pretty happy with this version.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete
2013-07-18 11:07 ` Stefan Hajnoczi
@ 2013-07-19 9:12 ` Wenchao Xia
0 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-19 9:12 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, phrdina, famz, qemu-devel, armbru, lcapitulino, stefanha,
pbonzini, dietmar
于 2013-7-18 19:07, Stefan Hajnoczi 写道:
> On Thu, Jul 11, 2013 at 01:46:58PM +0800, Wenchao Xia wrote:
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index f439738..06c777f 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
>> int64_t strtosz_suffix_unit(const char *nptr, char **end,
>> const char default_suffix, int64_t unit);
>>
>> +/* used to print char* safely */
>> +#define STR_PRINT_CHAR(str) ((str) ? (str) : "null")
>
> When I saw the name I thought it would filter out non-printable
> characters. Maybe STR_OR_NULL() is a better name?
>
I'll use STR_OR_NULL(), looks better.
> BTW the evil gcc shortcut is pretty quick to type: str ?: "null".
>
> Besides this I'm pretty happy with this version.
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
2013-07-18 11:07 ` Stefan Hajnoczi
@ 2013-07-18 11:48 ` Kevin Wolf
2013-07-19 9:13 ` Wenchao Xia
1 sibling, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2013-07-18 11:48 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> Snapshot creation actually already distinguish id and name since it take
> a structured parameter *sn, but delete can't. Later an accurate delete
> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
> so change its prototype. Also *errp is added to tip error, but return
> value is kepted to let caller check what kind of error happens. Existing
> caller for it are savevm, delvm and qemu-img, they are not impacted by
> check the return value and do the operations again.
>
> Before this patch:
> For qcow2, it search id first then name to find the one to delete.
> For rbd, it search name.
> For sheepdog, it does nothing.
>
> After this patch:
> For qcow2, logic is the same by call it twice in caller.
> For rbd, it always fails in delete with id, but still search for name
> in second try, no change for user.
>
> Some code for *errp is based on Pavel's patch.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block/qcow2-snapshot.c | 66 ++++++++++++++++++++++++++++++++++-----------
> block/qcow2.h | 5 +++-
> block/rbd.c | 23 +++++++++++++++-
> block/sheepdog.c | 5 +++-
> block/snapshot.c | 36 ++++++++++++++++++++++--
> include/block/block_int.h | 5 +++-
> include/block/snapshot.h | 5 +++-
> include/qemu-common.h | 3 ++
> qemu-img.c | 5 +++-
> savevm.c | 10 +++++-
> 10 files changed, 136 insertions(+), 27 deletions(-)
> @@ -531,15 +547,23 @@ fail:
> return ret;
> }
>
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + const char *name,
> + Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot sn;
> int snapshot_index, ret;
> + const char *device = bdrv_get_device_name(bs);
>
> /* Search the snapshot */
> - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> + snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
> if (snapshot_index < 0) {
> + error_setg(errp,
> + "Can't find a snapshot with ID '%s' and name '%s' "
> + "on device '%s'",
> + STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
At least the "on device '%s'" part should be removed from the error
messages. It is something the caller can add if there is any ambiguity.
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
> break;
>
> case SNAPSHOT_DELETE:
> - ret = bdrv_snapshot_delete(bs, snapshot_name);
> + ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
> + if (ret == -ENOENT || ret == -EINVAL) {
> + ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
> + }
> if (ret) {
> error_report("Could not delete snapshot '%s': %d (%s)",
> snapshot_name, ret, strerror(-ret));
Why don't you use the new error messages?
> diff --git a/savevm.c b/savevm.c
> index e0491e7..56afebb 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> if (bdrv_can_snapshot(bs) &&
> bdrv_snapshot_find(bs, snapshot, name) >= 0)
> {
> - ret = bdrv_snapshot_delete(bs, name);
> + ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
> + if (ret == -ENOENT || ret == -EINVAL) {
> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
> + }
> if (ret < 0) {
> monitor_printf(mon,
> "Error while deleting snapshot on '%s'\n",
> @@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
> bs1 = NULL;
> while ((bs1 = bdrv_next(bs1))) {
> if (bdrv_can_snapshot(bs1)) {
> - ret = bdrv_snapshot_delete(bs1, name);
> + ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
> + if (ret == -ENOENT || ret == -EINVAL) {
> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
> + }
> if (ret < 0) {
> if (ret == -ENOTSUP)
> monitor_printf(mon,
Same thing here.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete
2013-07-18 11:48 ` Kevin Wolf
@ 2013-07-19 9:13 ` Wenchao Xia
0 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-19 9:13 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
于 2013-7-18 19:48, Kevin Wolf 写道:
> Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
>> Snapshot creation actually already distinguish id and name since it take
>> a structured parameter *sn, but delete can't. Later an accurate delete
>> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
>> so change its prototype. Also *errp is added to tip error, but return
>> value is kepted to let caller check what kind of error happens. Existing
>> caller for it are savevm, delvm and qemu-img, they are not impacted by
>> check the return value and do the operations again.
>>
>> Before this patch:
>> For qcow2, it search id first then name to find the one to delete.
>> For rbd, it search name.
>> For sheepdog, it does nothing.
>>
>> After this patch:
>> For qcow2, logic is the same by call it twice in caller.
>> For rbd, it always fails in delete with id, but still search for name
>> in second try, no change for user.
>>
>> Some code for *errp is based on Pavel's patch.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> block/qcow2-snapshot.c | 66 ++++++++++++++++++++++++++++++++++-----------
>> block/qcow2.h | 5 +++-
>> block/rbd.c | 23 +++++++++++++++-
>> block/sheepdog.c | 5 +++-
>> block/snapshot.c | 36 ++++++++++++++++++++++--
>> include/block/block_int.h | 5 +++-
>> include/block/snapshot.h | 5 +++-
>> include/qemu-common.h | 3 ++
>> qemu-img.c | 5 +++-
>> savevm.c | 10 +++++-
>> 10 files changed, 136 insertions(+), 27 deletions(-)
>
>> @@ -531,15 +547,23 @@ fail:
>> return ret;
>> }
>>
>> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>> +int qcow2_snapshot_delete(BlockDriverState *bs,
>> + const char *snapshot_id,
>> + const char *name,
>> + Error **errp)
>> {
>> BDRVQcowState *s = bs->opaque;
>> QCowSnapshot sn;
>> int snapshot_index, ret;
>> + const char *device = bdrv_get_device_name(bs);
>>
>> /* Search the snapshot */
>> - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
>> + snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
>> if (snapshot_index < 0) {
>> + error_setg(errp,
>> + "Can't find a snapshot with ID '%s' and name '%s' "
>> + "on device '%s'",
>> + STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
>
> At least the "on device '%s'" part should be removed from the error
> messages. It is something the caller can add if there is any ambiguity.
>
will remove.
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
>> break;
>>
>> case SNAPSHOT_DELETE:
>> - ret = bdrv_snapshot_delete(bs, snapshot_name);
>> + ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
>> + if (ret == -ENOENT || ret == -EINVAL) {
>> + ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
>> + }
>> if (ret) {
>> error_report("Could not delete snapshot '%s': %d (%s)",
>> snapshot_name, ret, strerror(-ret));
>
> Why don't you use the new error messages?
>
>> diff --git a/savevm.c b/savevm.c
>> index e0491e7..56afebb 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>> if (bdrv_can_snapshot(bs) &&
>> bdrv_snapshot_find(bs, snapshot, name) >= 0)
>> {
>> - ret = bdrv_snapshot_delete(bs, name);
>> + ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
>> + if (ret == -ENOENT || ret == -EINVAL) {
>> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
>> + }
>> if (ret < 0) {
>> monitor_printf(mon,
>> "Error while deleting snapshot on '%s'\n",
>> @@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
>> bs1 = NULL;
>> while ((bs1 = bdrv_next(bs1))) {
>> if (bdrv_can_snapshot(bs1)) {
>> - ret = bdrv_snapshot_delete(bs1, name);
>> + ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
>> + if (ret == -ENOENT || ret == -EINVAL) {
>> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
>> + }
>> if (ret < 0) {
>> if (ret == -ENOTSUP)
>> monitor_printf(mon,
>
> Same thing here.
>
> Kevin
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name() Wenchao Xia
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
@ 2013-07-11 5:46 ` Wenchao Xia
2013-07-18 12:22 ` Kevin Wolf
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 4/8] qmp: add interface blockdev-snapshot-internal-sync Wenchao Xia
` (6 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:46 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot.
Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case. Request with
empty name will be rejected.
Snapshot ID can't be specified in this interface.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 18 ++++++++-
qmp-commands.hx | 34 ++++++++++++----
3 files changed, 160 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b3a57e0..6554768 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -808,6 +808,118 @@ struct BlkTransactionState {
QSIMPLEQ_ENTRY(BlkTransactionState) entry;
};
+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+ BlkTransactionState common;
+ BlockDriverState *bs;
+ QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ const char *device;
+ const char *name;
+ BlockDriverState *bs;
+ QEMUSnapshotInfo sn, *sn1;
+ bool ret;
+ qemu_timeval tv;
+ BlockdevSnapshotInternal *internal;
+ InternalSnapshotState *state;
+ int ret1;
+
+ g_assert(common->action->kind ==
+ TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+ internal = common->action->blockdev_snapshot_internal_sync;
+ state = DO_UPCAST(InternalSnapshotState, common, common);
+
+ /* 1. parse input */
+ device = internal->device;
+ name = internal->name;
+
+ /* 2. check for validation */
+ bs = bdrv_find(device);
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return;
+ }
+
+ if (!bdrv_is_inserted(bs)) {
+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+ return;
+ }
+
+ if (bdrv_is_read_only(bs)) {
+ error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+ return;
+ }
+
+ if (!bdrv_can_snapshot(bs)) {
+ error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+ bs->drv->format_name, device, "internal snapshot");
+ return;
+ }
+
+ if (!strlen(name)) {
+ error_setg(errp, "Name is empty on device '%s'", device);
+ return;
+ }
+
+ /* check whether a snapshot with name exist */
+ ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
+ if (error_is_set(errp)) {
+ return;
+ }
+ if (ret) {
+ error_setg(errp,
+ "Snapshot with name '%s' already exists on device '%s'",
+ name, device);
+ return;
+ }
+
+ /* 3. take the snapshot */
+ sn1 = &state->sn;
+ pstrcpy(sn1->name, sizeof(sn1->name), name);
+ qemu_gettimeofday(&tv);
+ sn1->date_sec = tv.tv_sec;
+ sn1->date_nsec = tv.tv_usec * 1000;
+ sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+ ret1 = bdrv_snapshot_create(bs, sn1);
+ if (ret1 < 0) {
+ error_setg_errno(errp, -ret1,
+ "Failed to create snapshot '%s' on device '%s'",
+ name, device);
+ return;
+ }
+
+ /* 4. succeed, mark a snapshot is created */
+ state->bs = bs;
+}
+
+static void internal_snapshot_abort(BlkTransactionState *common)
+{
+ InternalSnapshotState *state =
+ DO_UPCAST(InternalSnapshotState, common, common);
+ BlockDriverState *bs = state->bs;
+ QEMUSnapshotInfo *sn = &state->sn;
+ Error *local_error = NULL;
+
+ if (!bs) {
+ return;
+ }
+
+ if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
+ error_report("Failed to delete snapshot with id '%s' and name '%s' on "
+ "device '%s' in abort, reason is: '%s'",
+ sn->id_str,
+ sn->name,
+ bdrv_get_device_name(bs),
+ error_get_pretty(local_error));
+ error_free(local_error);
+ }
+}
+
/* external snapshot private data */
typedef struct ExternalSnapshotState {
BlkTransactionState common;
@@ -990,6 +1102,11 @@ static const BdrvActionOps actions[] = {
.prepare = abort_prepare,
.commit = abort_commit,
},
+ [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
+ .instance_size = sizeof(InternalSnapshotState),
+ .prepare = internal_snapshot_prepare,
+ .abort = internal_snapshot_abort,
+ },
};
/*
diff --git a/qapi-schema.json b/qapi-schema.json
index b251d28..5ec55d6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1621,6 +1621,21 @@
{ 'type': 'BlockdevSnapshot',
'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
'*mode': 'NewImageMode' } }
+##
+# @BlockdevSnapshotInternal
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: the name of the internal snapshot to be created
+#
+# Notes: In transaction, if @name is empty, or any snapshot matching @name
+# exists, the operation will fail. Only some image formats support it,
+# for example, qcow2, rbd, and sheepdog.
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevSnapshotInternal',
+ 'data': { 'device': 'str', 'name': 'str' } }
##
# @DriveBackup
@@ -1679,7 +1694,8 @@
'data': {
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
- 'abort': 'Abort'
+ 'abort': 'Abort',
+ 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
} }
##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 362f0e1..0277f7e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -993,14 +993,15 @@ SQMP
transaction
-----------
-Atomically operate on one or more block devices. The only supported
-operation for now is snapshotting. If there is any failure performing
-any of the operations, all snapshots for the group are abandoned, and
-the original disks pre-snapshot attempt are used.
+Atomically operate on one or more block devices. The only supported operations
+for now are drive-backup, internal and external snapshotting. A list of
+dictionaries is accepted, that contains the actions to be performed.
+If there is any failure performing any of the operations, all operations
+for the group are abandoned.
-A list of dictionaries is accepted, that contains the actions to be performed.
-For snapshots this is the device, the file to use for the new snapshot,
-and the format. The default format, if not specified, is qcow2.
+For external snapshots, the dictionary contains the device, the file to use for
+the new snapshot, and the format. The default format, if not specified, is
+qcow2.
Each new snapshot defaults to being created by QEMU (wiping any
contents if the file already exists), but it is also possible to reuse
@@ -1009,6 +1010,17 @@ the new image file has the same contents as the current one; QEMU cannot
perform any meaningful check. Typically this is achieved by using the
current image file as the backing file for the new image.
+On failure, the original disks pre-snapshot attempt will be used.
+
+For internal snapshots, the dictionary contains the device and the snapshot's
+name. If an internal snapshot matching name already exists, the request will
+be rejected. Only some image formats support it, for example, qcow2, rbd,
+and sheepdog.
+
+On failure, qemu will try delete the newly created internal snapshot in the
+transaction. When an I/O error occurs during deletion, the user needs to fix
+it later with qemu-img or other command.
+
Arguments:
actions array:
@@ -1021,6 +1033,9 @@ actions array:
- "format": format of new image (json-string, optional)
- "mode": whether and how QEMU should create the snapshot file
(NewImageMode, optional, default "absolute-paths")
+ When "type" is "blockdev-snapshot-internal-sync":
+ - "device": device name to snapshot (json-string)
+ - "name": name of the new snapshot (json-string)
Example:
@@ -1032,7 +1047,10 @@ Example:
{ 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
"snapshot-file": "/some/place/my-image2",
"mode": "existing",
- "format": "qcow2" } } ] } }
+ "format": "qcow2" } },
+ { 'type': 'blockdev-snapshot-internal-sync', 'data' : {
+ "device": "ide-hd2",
+ "name": "snapshot0" } } ] } }
<- { "return": {} }
EQMP
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction Wenchao Xia
@ 2013-07-18 12:22 ` Kevin Wolf
2013-07-19 9:19 ` Wenchao Xia
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2013-07-18 12:22 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> Unlike savevm, the qmp_transaction interface will not generate
> snapshot name automatically, saving trouble to return information
> of the new created snapshot.
>
> Although qcow2 support storing multiple snapshots with same name
> but different ID, here it will fail when an snapshot with that name
> already exist before the operation. Format such as rbd do not support
> ID at all, and in most case, it means trouble to user when he faces
> multiple snapshots with same name, so ban that case. Request with
> empty name will be rejected.
>
> Snapshot ID can't be specified in this interface.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qapi-schema.json | 18 ++++++++-
> qmp-commands.hx | 34 ++++++++++++----
> 3 files changed, 160 insertions(+), 9 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index b3a57e0..6554768 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -808,6 +808,118 @@ struct BlkTransactionState {
> QSIMPLEQ_ENTRY(BlkTransactionState) entry;
> };
>
> +/* internal snapshot private data */
> +typedef struct InternalSnapshotState {
> + BlkTransactionState common;
> + BlockDriverState *bs;
> + QEMUSnapshotInfo sn;
> +} InternalSnapshotState;
> +
> +static void internal_snapshot_prepare(BlkTransactionState *common,
> + Error **errp)
> +{
> + const char *device;
> + const char *name;
> + BlockDriverState *bs;
> + QEMUSnapshotInfo sn, *sn1;
> + bool ret;
> + qemu_timeval tv;
> + BlockdevSnapshotInternal *internal;
> + InternalSnapshotState *state;
> + int ret1;
> +
> + g_assert(common->action->kind ==
> + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
> + internal = common->action->blockdev_snapshot_internal_sync;
> + state = DO_UPCAST(InternalSnapshotState, common, common);
> +
> + /* 1. parse input */
> + device = internal->device;
> + name = internal->name;
> +
> + /* 2. check for validation */
> + bs = bdrv_find(device);
> + if (!bs) {
> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + return;
> + }
> +
> + if (!bdrv_is_inserted(bs)) {
> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> + return;
> + }
> +
> + if (bdrv_is_read_only(bs)) {
> + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
> + return;
> + }
> +
> + if (!bdrv_can_snapshot(bs)) {
> + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> + bs->drv->format_name, device, "internal snapshot");
> + return;
> + }
> +
> + if (!strlen(name)) {
> + error_setg(errp, "Name is empty on device '%s'", device);
Name is empty. This has nothing to do with the device.
> + return;
> + }
> +
> + /* check whether a snapshot with name exist */
> + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
> + if (error_is_set(errp)) {
> + return;
> + }
> + if (ret) {
Save one line with '} else if {' ?
> + error_setg(errp,
> + "Snapshot with name '%s' already exists on device '%s'",
> + name, device);
> + return;
> + }
> +
> + /* 3. take the snapshot */
> + sn1 = &state->sn;
do_savevm() has a memset() to clear all of sn1 before it starts filling
it in. Should we do the same here? For example sn1.vm_state_size looks
undefined.
It also stops the VM before saving the snapshot. I don't think this is
necessary here because we don't save the VM state, but do we need at
least the bdrv_flush/bdrv_drain_all part of it?
> + pstrcpy(sn1->name, sizeof(sn1->name), name);
> + qemu_gettimeofday(&tv);
> + sn1->date_sec = tv.tv_sec;
> + sn1->date_nsec = tv.tv_usec * 1000;
> + sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
> +
> + ret1 = bdrv_snapshot_create(bs, sn1);
> + if (ret1 < 0) {
> + error_setg_errno(errp, -ret1,
> + "Failed to create snapshot '%s' on device '%s'",
> + name, device);
> + return;
> + }
> +
> + /* 4. succeed, mark a snapshot is created */
> + state->bs = bs;
> +}
> +
> +static void internal_snapshot_abort(BlkTransactionState *common)
> +{
> + InternalSnapshotState *state =
> + DO_UPCAST(InternalSnapshotState, common, common);
> + BlockDriverState *bs = state->bs;
> + QEMUSnapshotInfo *sn = &state->sn;
> + Error *local_error = NULL;
> +
> + if (!bs) {
> + return;
> + }
> +
> + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
> + error_report("Failed to delete snapshot with id '%s' and name '%s' on "
> + "device '%s' in abort, reason is: '%s'",
> + sn->id_str,
> + sn->name,
> + bdrv_get_device_name(bs),
> + error_get_pretty(local_error));
> + error_free(local_error);
See, here you're doing the right thing if bdrv_snapshot_delete() returns
simple errors like "Failed to remove from snapshot list". With the
changes the earlier patch made to qcow2, you end up with this, though:
Failed to delete snapshot with id 'uninitialised' and name 'test' on
device 'ide0-hd0' in abort, reason is: 'Failed to remove snapshot
with ID 'uninitialised' and name 'test' from the snapshot list on
device 'ide0-hd0''
We need to standardise on the minimal error information that makes the
error unambiguous in order to avoid such duplication.
To sum up: Leave this code as it is, but change qcow2 etc. to remove ID,
name and device from their messages.
> + }
> +}
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction
2013-07-18 12:22 ` Kevin Wolf
@ 2013-07-19 9:19 ` Wenchao Xia
2013-07-19 10:13 ` Kevin Wolf
0 siblings, 1 reply; 20+ messages in thread
From: Wenchao Xia @ 2013-07-19 9:19 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
于 2013-7-18 20:22, Kevin Wolf 写道:
> Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
>> Unlike savevm, the qmp_transaction interface will not generate
>> snapshot name automatically, saving trouble to return information
>> of the new created snapshot.
>>
>> Although qcow2 support storing multiple snapshots with same name
>> but different ID, here it will fail when an snapshot with that name
>> already exist before the operation. Format such as rbd do not support
>> ID at all, and in most case, it means trouble to user when he faces
>> multiple snapshots with same name, so ban that case. Request with
>> empty name will be rejected.
>>
>> Snapshot ID can't be specified in this interface.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> qapi-schema.json | 18 ++++++++-
>> qmp-commands.hx | 34 ++++++++++++----
>> 3 files changed, 160 insertions(+), 9 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b3a57e0..6554768 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -808,6 +808,118 @@ struct BlkTransactionState {
>> QSIMPLEQ_ENTRY(BlkTransactionState) entry;
>> };
>>
>> +/* internal snapshot private data */
>> +typedef struct InternalSnapshotState {
>> + BlkTransactionState common;
>> + BlockDriverState *bs;
>> + QEMUSnapshotInfo sn;
>> +} InternalSnapshotState;
>> +
>> +static void internal_snapshot_prepare(BlkTransactionState *common,
>> + Error **errp)
>> +{
>> + const char *device;
>> + const char *name;
>> + BlockDriverState *bs;
>> + QEMUSnapshotInfo sn, *sn1;
>> + bool ret;
>> + qemu_timeval tv;
>> + BlockdevSnapshotInternal *internal;
>> + InternalSnapshotState *state;
>> + int ret1;
>> +
>> + g_assert(common->action->kind ==
>> + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
>> + internal = common->action->blockdev_snapshot_internal_sync;
>> + state = DO_UPCAST(InternalSnapshotState, common, common);
>> +
>> + /* 1. parse input */
>> + device = internal->device;
>> + name = internal->name;
>> +
>> + /* 2. check for validation */
>> + bs = bdrv_find(device);
>> + if (!bs) {
>> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> + return;
>> + }
>> +
>> + if (!bdrv_is_inserted(bs)) {
>> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>> + return;
>> + }
>> +
>> + if (bdrv_is_read_only(bs)) {
>> + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
>> + return;
>> + }
>> +
>> + if (!bdrv_can_snapshot(bs)) {
>> + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>> + bs->drv->format_name, device, "internal snapshot");
>> + return;
>> + }
>> +
>> + if (!strlen(name)) {
>> + error_setg(errp, "Name is empty on device '%s'", device);
>
> Name is empty. This has nothing to do with the device.
>
OK.
>> + return;
>> + }
>> +
>> + /* check whether a snapshot with name exist */
>> + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
>> + if (error_is_set(errp)) {
>> + return;
>> + }
>> + if (ret) {
>
> Save one line with '} else if {' ?
>
will change, thanks.
>> + error_setg(errp,
>> + "Snapshot with name '%s' already exists on device '%s'",
>> + name, device);
>> + return;
>> + }
>> +
>> + /* 3. take the snapshot */
>> + sn1 = &state->sn;
>
> do_savevm() has a memset() to clear all of sn1 before it starts filling
> it in. Should we do the same here? For example sn1.vm_state_size looks
> undefined.
>
I think state->sn is set to zero by
qmp_transaction():
state = g_malloc0(ops->instance_size);
> It also stops the VM before saving the snapshot. I don't think this is
> necessary here because we don't save the VM state, but do we need at
> least the bdrv_flush/bdrv_drain_all part of it?
>
bdrv_drain_all() is called already in qmp_transaction(). But I plan
add an explicit API later for bdrv_flush/bdrv_drain, which ensure data
is going to underlining storage.
>> + pstrcpy(sn1->name, sizeof(sn1->name), name);
>> + qemu_gettimeofday(&tv);
>> + sn1->date_sec = tv.tv_sec;
>> + sn1->date_nsec = tv.tv_usec * 1000;
>> + sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>> +
>> + ret1 = bdrv_snapshot_create(bs, sn1);
>> + if (ret1 < 0) {
>> + error_setg_errno(errp, -ret1,
>> + "Failed to create snapshot '%s' on device '%s'",
>> + name, device);
>> + return;
>> + }
>> +
>> + /* 4. succeed, mark a snapshot is created */
>> + state->bs = bs;
>> +}
>> +
>> +static void internal_snapshot_abort(BlkTransactionState *common)
>> +{
>> + InternalSnapshotState *state =
>> + DO_UPCAST(InternalSnapshotState, common, common);
>> + BlockDriverState *bs = state->bs;
>> + QEMUSnapshotInfo *sn = &state->sn;
>> + Error *local_error = NULL;
>> +
>> + if (!bs) {
>> + return;
>> + }
>> +
>> + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
>> + error_report("Failed to delete snapshot with id '%s' and name '%s' on "
>> + "device '%s' in abort, reason is: '%s'",
>> + sn->id_str,
>> + sn->name,
>> + bdrv_get_device_name(bs),
>> + error_get_pretty(local_error));
>> + error_free(local_error);
>
> See, here you're doing the right thing if bdrv_snapshot_delete() returns
> simple errors like "Failed to remove from snapshot list". With the
> changes the earlier patch made to qcow2, you end up with this, though:
>
> Failed to delete snapshot with id 'uninitialised' and name 'test' on
> device 'ide0-hd0' in abort, reason is: 'Failed to remove snapshot
> with ID 'uninitialised' and name 'test' from the snapshot list on
> device 'ide0-hd0''
>
> We need to standardise on the minimal error information that makes the
> error unambiguous in order to avoid such duplication.
>
> To sum up: Leave this code as it is, but change qcow2 etc. to remove ID,
> name and device from their messages.
>
OK, will remove them in the implementions.
>> + }
>> +}
>
> Kevin
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction
2013-07-19 9:19 ` Wenchao Xia
@ 2013-07-19 10:13 ` Kevin Wolf
2013-07-20 0:09 ` Wenchao Xia
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2013-07-19 10:13 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 19.07.2013 um 11:19 hat Wenchao Xia geschrieben:
> 于 2013-7-18 20:22, Kevin Wolf 写道:
> >Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> >>Unlike savevm, the qmp_transaction interface will not generate
> >>snapshot name automatically, saving trouble to return information
> >>of the new created snapshot.
> >>
> >>Although qcow2 support storing multiple snapshots with same name
> >>but different ID, here it will fail when an snapshot with that name
> >>already exist before the operation. Format such as rbd do not support
> >>ID at all, and in most case, it means trouble to user when he faces
> >>multiple snapshots with same name, so ban that case. Request with
> >>empty name will be rejected.
> >>
> >>Snapshot ID can't be specified in this interface.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>---
> >> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> qapi-schema.json | 18 ++++++++-
> >> qmp-commands.hx | 34 ++++++++++++----
> >> 3 files changed, 160 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/blockdev.c b/blockdev.c
> >>index b3a57e0..6554768 100644
> >>--- a/blockdev.c
> >>+++ b/blockdev.c
> >>@@ -808,6 +808,118 @@ struct BlkTransactionState {
> >> QSIMPLEQ_ENTRY(BlkTransactionState) entry;
> >> };
> >>
> >>+/* internal snapshot private data */
> >>+typedef struct InternalSnapshotState {
> >>+ BlkTransactionState common;
> >>+ BlockDriverState *bs;
> >>+ QEMUSnapshotInfo sn;
> >>+} InternalSnapshotState;
> >>+
> >>+static void internal_snapshot_prepare(BlkTransactionState *common,
> >>+ Error **errp)
> >>+{
> >>+ const char *device;
> >>+ const char *name;
> >>+ BlockDriverState *bs;
> >>+ QEMUSnapshotInfo sn, *sn1;
> >>+ bool ret;
> >>+ qemu_timeval tv;
> >>+ BlockdevSnapshotInternal *internal;
> >>+ InternalSnapshotState *state;
> >>+ int ret1;
> >>+
> >>+ g_assert(common->action->kind ==
> >>+ TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
> >>+ internal = common->action->blockdev_snapshot_internal_sync;
> >>+ state = DO_UPCAST(InternalSnapshotState, common, common);
> >>+
> >>+ /* 1. parse input */
> >>+ device = internal->device;
> >>+ name = internal->name;
> >>+
> >>+ /* 2. check for validation */
> >>+ bs = bdrv_find(device);
> >>+ if (!bs) {
> >>+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >>+ return;
> >>+ }
> >>+
> >>+ if (!bdrv_is_inserted(bs)) {
> >>+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> >>+ return;
> >>+ }
> >>+
> >>+ if (bdrv_is_read_only(bs)) {
> >>+ error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
> >>+ return;
> >>+ }
> >>+
> >>+ if (!bdrv_can_snapshot(bs)) {
> >>+ error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> >>+ bs->drv->format_name, device, "internal snapshot");
> >>+ return;
> >>+ }
> >>+
> >>+ if (!strlen(name)) {
> >>+ error_setg(errp, "Name is empty on device '%s'", device);
> >
> >Name is empty. This has nothing to do with the device.
> >
> OK.
>
> >>+ return;
> >>+ }
> >>+
> >>+ /* check whether a snapshot with name exist */
> >>+ ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
> >>+ if (error_is_set(errp)) {
> >>+ return;
> >>+ }
> >>+ if (ret) {
> >
> >Save one line with '} else if {' ?
> >
> will change, thanks.
>
> >>+ error_setg(errp,
> >>+ "Snapshot with name '%s' already exists on device '%s'",
> >>+ name, device);
> >>+ return;
> >>+ }
> >>+
> >>+ /* 3. take the snapshot */
> >>+ sn1 = &state->sn;
> >
> >do_savevm() has a memset() to clear all of sn1 before it starts filling
> >it in. Should we do the same here? For example sn1.vm_state_size looks
> >undefined.
> >
> I think state->sn is set to zero by
> qmp_transaction():
> state = g_malloc0(ops->instance_size);
Oh, yes. I was confused by the fact that there is a local sn, which
isn't related to sn1 at all. Perhaps some renaming to make things
clearer?
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction
2013-07-19 10:13 ` Kevin Wolf
@ 2013-07-20 0:09 ` Wenchao Xia
0 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-20 0:09 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, qemu-devel, armbru, lcapitulino, stefanha,
pbonzini, dietmar
于 2013-7-19 18:13, Kevin Wolf 写道:
> Am 19.07.2013 um 11:19 hat Wenchao Xia geschrieben:
>> 于 2013-7-18 20:22, Kevin Wolf 写道:
>>> Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
>>>> Unlike savevm, the qmp_transaction interface will not generate
>>>> snapshot name automatically, saving trouble to return information
>>>> of the new created snapshot.
>>>>
>>>> Although qcow2 support storing multiple snapshots with same name
>>>> but different ID, here it will fail when an snapshot with that name
>>>> already exist before the operation. Format such as rbd do not support
>>>> ID at all, and in most case, it means trouble to user when he faces
>>>> multiple snapshots with same name, so ban that case. Request with
>>>> empty name will be rejected.
>>>>
>>>> Snapshot ID can't be specified in this interface.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> qapi-schema.json | 18 ++++++++-
>>>> qmp-commands.hx | 34 ++++++++++++----
>>>> 3 files changed, 160 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index b3a57e0..6554768 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -808,6 +808,118 @@ struct BlkTransactionState {
>>>> QSIMPLEQ_ENTRY(BlkTransactionState) entry;
>>>> };
>>>>
>>>> +/* internal snapshot private data */
>>>> +typedef struct InternalSnapshotState {
>>>> + BlkTransactionState common;
>>>> + BlockDriverState *bs;
>>>> + QEMUSnapshotInfo sn;
>>>> +} InternalSnapshotState;
>>>> +
>>>> +static void internal_snapshot_prepare(BlkTransactionState *common,
>>>> + Error **errp)
>>>> +{
>>>> + const char *device;
>>>> + const char *name;
>>>> + BlockDriverState *bs;
>>>> + QEMUSnapshotInfo sn, *sn1;
>>>> + bool ret;
>>>> + qemu_timeval tv;
>>>> + BlockdevSnapshotInternal *internal;
>>>> + InternalSnapshotState *state;
>>>> + int ret1;
>>>> +
>>>> + g_assert(common->action->kind ==
>>>> + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
>>>> + internal = common->action->blockdev_snapshot_internal_sync;
>>>> + state = DO_UPCAST(InternalSnapshotState, common, common);
>>>> +
>>>> + /* 1. parse input */
>>>> + device = internal->device;
>>>> + name = internal->name;
>>>> +
>>>> + /* 2. check for validation */
>>>> + bs = bdrv_find(device);
>>>> + if (!bs) {
>>>> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>>>> + return;
>>>> + }
>>>> +
>>>> + if (!bdrv_is_inserted(bs)) {
>>>> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>>>> + return;
>>>> + }
>>>> +
>>>> + if (bdrv_is_read_only(bs)) {
>>>> + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
>>>> + return;
>>>> + }
>>>> +
>>>> + if (!bdrv_can_snapshot(bs)) {
>>>> + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>>>> + bs->drv->format_name, device, "internal snapshot");
>>>> + return;
>>>> + }
>>>> +
>>>> + if (!strlen(name)) {
>>>> + error_setg(errp, "Name is empty on device '%s'", device);
>>>
>>> Name is empty. This has nothing to do with the device.
>>>
>> OK.
>>
>>>> + return;
>>>> + }
>>>> +
>>>> + /* check whether a snapshot with name exist */
>>>> + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
>>>> + if (error_is_set(errp)) {
>>>> + return;
>>>> + }
>>>> + if (ret) {
>>>
>>> Save one line with '} else if {' ?
>>>
>> will change, thanks.
>>
>>>> + error_setg(errp,
>>>> + "Snapshot with name '%s' already exists on device '%s'",
>>>> + name, device);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* 3. take the snapshot */
>>>> + sn1 = &state->sn;
>>>
>>> do_savevm() has a memset() to clear all of sn1 before it starts filling
>>> it in. Should we do the same here? For example sn1.vm_state_size looks
>>> undefined.
>>>
>> I think state->sn is set to zero by
>> qmp_transaction():
>> state = g_malloc0(ops->instance_size);
>
> Oh, yes. I was confused by the fact that there is a local sn, which
> isn't related to sn1 at all. Perhaps some renaming to make things
> clearer?
>
> Kevin
>
sure, will rename local sn to sn_old.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH V5 4/8] qmp: add interface blockdev-snapshot-internal-sync
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (2 preceding siblings ...)
2013-07-11 5:46 ` [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction Wenchao Xia
@ 2013-07-11 5:47 ` Wenchao Xia
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync Wenchao Xia
` (5 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Snapshot ID can't be specified in this interface.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
blockdev.c | 13 +++++++++++++
qapi-schema.json | 20 ++++++++++++++++++++
qmp-commands.hx | 29 +++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 6554768..c97299b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -777,6 +777,19 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
&snapshot, errp);
}
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+ const char *name,
+ Error **errp)
+{
+ BlockdevSnapshotInternal snapshot = {
+ .device = (char *) device,
+ .name = (char *) name
+ };
+
+ blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+ &snapshot, errp);
+}
+
/* New and old BlockDriverState structs for group snapshots */
diff --git a/qapi-schema.json b/qapi-schema.json
index 5ec55d6..55641a3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1736,6 +1736,26 @@
'data': 'BlockdevSnapshot' }
##
+# @blockdev-snapshot-internal-sync
+#
+# Synchronously take an internal snapshot of a block device, when the format
+# of the image used supports it.
+#
+# For the arguments, see the documentation of BlockdevSnapshotInternal.
+#
+# Returns: nothing on success
+# If @device is not a valid block device, DeviceNotFound
+# If any snapshot matching @name exists, or @name is empty,
+# GenericError
+# If the format of the image used does not support it,
+# BlockFormatFeatureNotSupported
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+ 'data': 'BlockdevSnapshotInternal' }
+
+##
# @human-monitor-command:
#
# Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0277f7e..643be17 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1090,6 +1090,35 @@ Example:
EQMP
{
+ .name = "blockdev-snapshot-internal-sync",
+ .args_type = "device:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+ },
+
+SQMP
+blockdev-snapshot-internal-sync
+-------------------------------
+
+Synchronously take an internal snapshot of a block device when the format of
+image used supports it. If the name is an empty string, or a snapshot with
+name already exists, the operation will fail.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "name": name of the new snapshot (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-internal-sync",
+ "arguments": { "device": "ide-hd0",
+ "name": "snapshot0" }
+ }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "drive-mirror",
.args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
"on-source-error:s?,on-target-error:s?,"
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH V5 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (3 preceding siblings ...)
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 4/8] qmp: add interface blockdev-snapshot-internal-sync Wenchao Xia
@ 2013-07-11 5:47 ` Wenchao Xia
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 6/8] hmp: add interface hmp_snapshot_blkdev_internal Wenchao Xia
` (4 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
This interface use id and name as optional parameters, to handle the
case that one image contain multiple snapshots with same name which
may be '', but with different id.
Adding parameter id is for historical compatiability reason, and
that case is not possible in qemu's new interface for internal
snapshot at block device level, but still possible in qemu-img.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 27 +++++++++++++++++++++++
qmp-commands.hx | 41 ++++++++++++++++++++++++++++++++++++
3 files changed, 129 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index c97299b..d390a94 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -790,6 +790,67 @@ void qmp_blockdev_snapshot_internal_sync(const char *device,
&snapshot, errp);
}
+SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
+ bool has_id,
+ const char *id,
+ bool has_name,
+ const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs = bdrv_find(device);
+ QEMUSnapshotInfo sn;
+ Error *local_err = NULL;
+ SnapshotInfo *info = NULL;
+ int ret;
+
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return NULL;
+ }
+
+ if (!has_id) {
+ id = NULL;
+ }
+
+ if (!has_name) {
+ name = NULL;
+ }
+
+ if (!id && !name) {
+ error_setg(errp, "Name or id must be provided");
+ return NULL;
+ }
+
+ ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+ if (!ret) {
+ error_setg(errp,
+ "Snapshot with id '%s' and name '%s' does not exist on "
+ "device '%s'",
+ STR_PRINT_CHAR(id), STR_PRINT_CHAR(name), device);
+ return NULL;
+ }
+
+ bdrv_snapshot_delete(bs, id, name, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ info = g_malloc0(sizeof(SnapshotInfo));
+ info->id = g_strdup(sn.id_str);
+ info->name = g_strdup(sn.name);
+ info->date_nsec = sn.date_nsec;
+ info->date_sec = sn.date_sec;
+ info->vm_state_size = sn.vm_state_size;
+ info->vm_clock_nsec = sn.vm_clock_nsec % 1000000000;
+ info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
+
+ return info;
+}
/* New and old BlockDriverState structs for group snapshots */
diff --git a/qapi-schema.json b/qapi-schema.json
index 55641a3..5c84f2e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1756,6 +1756,33 @@
'data': 'BlockdevSnapshotInternal' }
##
+# @blockdev-snapshot-delete-internal-sync
+#
+# Synchronously delete an internal snapshot of a block device, when the format
+# of the image used support it. The snapshot is identified by name or id or
+# both. One of the name or id is required. Return SnapshotInfo for the
+# successfully deleted snapshot.
+#
+# @device: the name of the device to delete the snapshot from
+#
+# @id: optional the snapshot's ID to be deleted
+#
+# @name: optional the snapshot's name to be deleted
+#
+# Returns: SnapshotInfo on success
+# If @device is not a valid block device, DeviceNotFound
+# If snapshot not found, GenericError
+# If the format of the image used does not support it,
+# BlockFormatFeatureNotSupported
+# If @id and @name are both not specified, GenericError
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-delete-internal-sync',
+ 'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
+ 'returns': 'SnapshotInfo' }
+
+##
# @human-monitor-command:
#
# Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 643be17..c87649e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1119,6 +1119,47 @@ Example:
EQMP
{
+ .name = "blockdev-snapshot-delete-internal-sync",
+ .args_type = "device:B,id:s?,name:s?",
+ .mhandler.cmd_new =
+ qmp_marshal_input_blockdev_snapshot_delete_internal_sync,
+ },
+
+SQMP
+blockdev-snapshot-delete-internal-sync
+--------------------------------------
+
+Synchronously delete an internal snapshot of a block device when the format of
+image used supports it. The snapshot is identified by name or id or both. One
+of name or id is required. If the snapshot is not found, the operation will
+fail.
+
+Arguments:
+
+- "device": device name (json-string)
+- "id": ID of the snapshot (json-string, optional)
+- "name": name of the snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-delete-internal-sync",
+ "arguments": { "device": "ide-hd0",
+ "name": "snapshot0" }
+ }
+<- { "return": {
+ "id": "1",
+ "name": "snapshot0",
+ "vm-state-size": 0,
+ "date-sec": 1000012,
+ "date-nsec": 10,
+ "vm-clock-sec": 100,
+ "vm-clock-nsec": 20
+ }
+ }
+
+EQMP
+
+ {
.name = "drive-mirror",
.args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
"on-source-error:s?,on-target-error:s?,"
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH V5 6/8] hmp: add interface hmp_snapshot_blkdev_internal
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (4 preceding siblings ...)
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync Wenchao Xia
@ 2013-07-11 5:47 ` Wenchao Xia
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal Wenchao Xia
` (3 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp-commands.hx | 19 +++++++++++++++++--
hmp.c | 10 ++++++++++
hmp.h | 1 +
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d1cdcfb..ec613e8 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1023,8 +1023,7 @@ ETEXI
"of device. If a new image file is specified, the\n\t\t\t"
"new image file will become the new root image.\n\t\t\t"
"If format is specified, the snapshot file will\n\t\t\t"
- "be created in that format. Otherwise the\n\t\t\t"
- "snapshot will be internal! (currently unsupported).\n\t\t\t"
+ "be created in that format.\n\t\t\t"
"The default format is qcow2. The -n flag requests QEMU\n\t\t\t"
"to reuse the image found in new-image-file, instead of\n\t\t\t"
"recreating it from scratch.",
@@ -1038,6 +1037,22 @@ Snapshot device, using snapshot file as target if provided
ETEXI
{
+ .name = "snapshot_blkdev_internal",
+ .args_type = "device:B,name:s",
+ .params = "device name",
+ .help = "take an internal snapshot of device.\n\t\t\t"
+ "The format of the image used by device must\n\t\t\t"
+ "support it, such as qcow2.\n\t\t\t",
+ .mhandler.cmd = hmp_snapshot_blkdev_internal,
+ },
+
+STEXI
+@item snapshot_blkdev_internal
+@findex snapshot_blkdev_internal
+Take an internal snapshot on device if it support
+ETEXI
+
+ {
.name = "drive_mirror",
.args_type = "reuse:-n,full:-f,device:B,target:s,format:s?",
.params = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index 2daed43..d300366 100644
--- a/hmp.c
+++ b/hmp.c
@@ -930,6 +930,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &errp);
}
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+ const char *device = qdict_get_str(qdict, "device");
+ const char *name = qdict_get_str(qdict, "name");
+ Error *errp = NULL;
+
+ qmp_blockdev_snapshot_internal_sync(device, name, &errp);
+ hmp_handle_error(mon, &errp);
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 56d2e92..fddf3a9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -54,6 +54,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict);
void hmp_balloon(Monitor *mon, const QDict *qdict);
void hmp_block_resize(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH V5 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (5 preceding siblings ...)
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 6/8] hmp: add interface hmp_snapshot_blkdev_internal Wenchao Xia
@ 2013-07-11 5:47 ` Wenchao Xia
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 8/8] qemu-iotests: add 056 internal snapshot for block device test case Wenchao Xia
` (2 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
It is hard to make both id and name optional in hmp console as qmp
interface, so this interface require user to specify name.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp-commands.hx | 18 ++++++++++++++++++
hmp.c | 12 ++++++++++++
hmp.h | 1 +
3 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index ec613e8..7098c11 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1053,6 +1053,24 @@ Take an internal snapshot on device if it support
ETEXI
{
+ .name = "snapshot_delete_blkdev_internal",
+ .args_type = "device:B,name:s,id:s?",
+ .params = "device name [id]",
+ .help = "delete an internal snapshot of device.\n\t\t\t"
+ "If id is specified, qemu will try delete\n\t\t\t"
+ "the snapshot matching both id and name.\n\t\t\t"
+ "The format of the image used by device must\n\t\t\t"
+ "support it, such as qcow2.\n\t\t\t",
+ .mhandler.cmd = hmp_snapshot_delete_blkdev_internal,
+ },
+
+STEXI
+@item snapshot_delete_blkdev_internal
+@findex snapshot_delete_blkdev_internal
+Delete an internal snapshot on device if it support
+ETEXI
+
+ {
.name = "drive_mirror",
.args_type = "reuse:-n,full:-f,device:B,target:s,format:s?",
.params = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index d300366..9f9f908 100644
--- a/hmp.c
+++ b/hmp.c
@@ -940,6 +940,18 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &errp);
}
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+ const char *device = qdict_get_str(qdict, "device");
+ const char *name = qdict_get_str(qdict, "name");
+ const char *id = qdict_get_try_str(qdict, "id");
+ Error *errp = NULL;
+
+ qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
+ true, name, &errp);
+ hmp_handle_error(mon, &errp);
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index fddf3a9..fa0e5d9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -55,6 +55,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict);
void hmp_block_resize(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH V5 8/8] qemu-iotests: add 056 internal snapshot for block device test case
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (6 preceding siblings ...)
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal Wenchao Xia
@ 2013-07-11 5:47 ` Wenchao Xia
2013-07-18 11:09 ` [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Stefan Hajnoczi
2013-07-18 12:34 ` Kevin Wolf
9 siblings, 0 replies; 20+ messages in thread
From: Wenchao Xia @ 2013-07-11 5:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Create in transaction and deletion in single command will be tested.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
tests/qemu-iotests/056 | 259 ++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/056.out | 5 +
tests/qemu-iotests/group | 1 +
3 files changed, 265 insertions(+), 0 deletions(-)
create mode 100755 tests/qemu-iotests/056
create mode 100644 tests/qemu-iotests/056.out
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
new file mode 100755
index 0000000..9cdd582
--- /dev/null
+++ b/tests/qemu-iotests/056
@@ -0,0 +1,259 @@
+#!/usr/bin/env python
+#
+# Tests for internal snapshot.
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# Based on 055.
+#
+# 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/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_drv_base_name = 'drive'
+
+class ImageSnapshotTestCase(iotests.QMPTestCase):
+ image_len = 120 * 1024 * 1024 # MB
+
+ def __init__(self, *args):
+ self.expect = []
+ super(ImageSnapshotTestCase, self).__init__(*args)
+
+ def _setUp(self, test_img_base_name, image_num):
+ self.vm = iotests.VM()
+ for i in range(0, image_num):
+ filename = '%s%d' % (test_img_base_name, i)
+ img = os.path.join(iotests.test_dir, filename)
+ device = '%s%d' % (test_drv_base_name, i)
+ qemu_img('create', '-f', iotests.imgfmt, img, str(self.image_len))
+ self.vm.add_drive(img)
+ self.expect.append({'image': img, 'device': device,
+ 'snapshots': [],
+ 'snapshots_name_counter': 0})
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for dev_expect in self.expect:
+ os.remove(dev_expect['image'])
+
+ def createSnapshotInTransaction(self, snapshot_num, abort = False):
+ actions = []
+ for dev_expect in self.expect:
+ num = dev_expect['snapshots_name_counter']
+ for j in range(0, snapshot_num):
+ name = '%s_sn%d' % (dev_expect['device'], num)
+ num = num + 1
+ if abort == False:
+ dev_expect['snapshots'].append({'name': name})
+ dev_expect['snapshots_name_counter'] = num
+ actions.append({
+ 'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': dev_expect['device'],
+ 'name': name },
+ })
+
+ if abort == True:
+ actions.append({
+ 'type': 'abort',
+ 'data': {},
+ })
+
+ result = self.vm.qmp('transaction', actions = actions)
+
+ if abort == True:
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ else:
+ self.assert_qmp(result, 'return', {})
+
+ def verifySnapshotInfo(self):
+ result = self.vm.qmp('query-block')
+
+ # Verify each expected result
+ for dev_expect in self.expect:
+ # 1. Find the returned image value and snapshot info
+ image_result = None
+ for device in result['return']:
+ if device['device'] == dev_expect['device']:
+ image_result = device['inserted']['image']
+ break
+ self.assertTrue(image_result != None)
+ # Do not consider zero snapshot case now
+ sn_list_result = image_result['snapshots']
+ sn_list_expect = dev_expect['snapshots']
+
+ # 2. Verify it with expect
+ self.assertTrue(len(sn_list_result) == len(sn_list_expect))
+
+ for sn_expect in sn_list_expect:
+ sn_result = None
+ for sn in sn_list_result:
+ if sn_expect['name'] == sn['name']:
+ sn_result = sn
+ break
+ self.assertTrue(sn_result != None)
+ # Fill in the detail info
+ sn_expect.update(sn_result)
+
+ def deleteSnapshot(self, device, id = None, name = None):
+ sn_list_expect = None
+ sn_expect = None
+
+ self.assertTrue(id != None or name != None)
+
+ # Fill in the detail info include ID
+ self.verifySnapshotInfo()
+
+ #find the expected snapshot list
+ for dev_expect in self.expect:
+ if dev_expect['device'] == device:
+ sn_list_expect = dev_expect['snapshots']
+ break
+ self.assertTrue(sn_list_expect != None)
+
+ if id != None and name != None:
+ for sn in sn_list_expect:
+ if sn['id'] == id and sn['name'] == name:
+ sn_expect = sn
+ result = \
+ self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = device,
+ id = id,
+ name = name)
+ break
+ elif id != None:
+ for sn in sn_list_expect:
+ if sn['id'] == id:
+ sn_expect = sn
+ result = \
+ self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = device,
+ id = id)
+ break
+ else:
+ for sn in sn_list_expect:
+ if sn['name'] == name:
+ sn_expect = sn
+ result = \
+ self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = device,
+ name = name)
+ break
+
+ self.assertTrue(sn_expect != None)
+
+ self.assert_qmp(result, 'return', sn_expect)
+ sn_list_expect.remove(sn_expect)
+
+class TestSingleTransaction(ImageSnapshotTestCase):
+ def setUp(self):
+ self._setUp('test_a.img', 1)
+
+ def test_create(self):
+ self.createSnapshotInTransaction(1)
+ self.verifySnapshotInfo()
+
+ def test_error_name_empty(self):
+ actions = [{'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': self.expect[0]['device'],
+ 'name': '' },
+ }]
+ result = self.vm.qmp('transaction', actions = actions)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_error_device(self):
+ actions = [{'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': 'drive_error',
+ 'name': 'a' },
+ }]
+ result = self.vm.qmp('transaction', actions = actions)
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ def test_error_exist(self):
+ self.createSnapshotInTransaction(1)
+ self.verifySnapshotInfo()
+ actions = [{'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': self.expect[0]['device'],
+ 'name': self.expect[0]['snapshots'][0] },
+ }]
+ result = self.vm.qmp('transaction', actions = actions)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+class TestMultipleTransaction(ImageSnapshotTestCase):
+ def setUp(self):
+ self._setUp('test_b.img', 2)
+
+ def test_create(self):
+ self.createSnapshotInTransaction(3)
+ self.verifySnapshotInfo()
+
+ def test_abort(self):
+ self.createSnapshotInTransaction(2)
+ self.verifySnapshotInfo()
+ self.createSnapshotInTransaction(3, abort = True)
+ self.verifySnapshotInfo()
+
+class TestSnapshotDelete(ImageSnapshotTestCase):
+ def setUp(self):
+ self._setUp('test_c.img', 1)
+
+ def test_delete_with_id(self):
+ self.createSnapshotInTransaction(2)
+ self.verifySnapshotInfo()
+ self.deleteSnapshot(self.expect[0]['device'],
+ id = self.expect[0]['snapshots'][0]['id'])
+ self.verifySnapshotInfo()
+
+ def test_delete_with_name(self):
+ self.createSnapshotInTransaction(3)
+ self.verifySnapshotInfo()
+ self.deleteSnapshot(self.expect[0]['device'],
+ name = self.expect[0]['snapshots'][1]['name'])
+ self.verifySnapshotInfo()
+
+ def test_delete_with_id_and_name(self):
+ self.createSnapshotInTransaction(4)
+ self.verifySnapshotInfo()
+ self.deleteSnapshot(self.expect[0]['device'],
+ id = self.expect[0]['snapshots'][2]['id'],
+ name = self.expect[0]['snapshots'][2]['name'])
+ self.verifySnapshotInfo()
+
+
+ def test_error_device(self):
+ result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = 'drive_error',
+ id = '0')
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ def test_error_no_id_and_name(self):
+ result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = self.expect[0]['device'])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_error_snapshot_not_exist(self):
+ self.createSnapshotInTransaction(2)
+ self.verifySnapshotInfo()
+ result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = self.expect[0]['device'],
+ id = self.expect[0]['snapshots'][0]['id'],
+ name = self.expect[0]['snapshots'][1]['name'])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
new file mode 100644
index 0000000..281b69e
--- /dev/null
+++ b/tests/qemu-iotests/056.out
@@ -0,0 +1,5 @@
+............
+----------------------------------------------------------------------
+Ran 12 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index fdc6ed1..5f5009b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -62,3 +62,4 @@
053 rw auto
054 rw auto
055 rw auto
+056 rw auto
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (7 preceding siblings ...)
2013-07-11 5:47 ` [Qemu-devel] [PATCH V5 8/8] qemu-iotests: add 056 internal snapshot for block device test case Wenchao Xia
@ 2013-07-18 11:09 ` Stefan Hajnoczi
2013-07-18 12:34 ` Kevin Wolf
9 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2013-07-18 11:09 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, dietmar, qemu-devel, armbru, lcapitulino, stefanha,
pbonzini, Wenchao Xia
On Thu, Jul 11, 2013 at 01:46:56PM +0800, Wenchao Xia wrote:
> This series brings internal snapshot support at block devices level, now we
> have two three methods to do block snapshot lively: 1) backing chain,
> 2) internal one and 3) drive-back up approach.
>
> Comparation:
> Advantages: Disadvantages:
> 1) delta data, taken fast, export, size performance, delete slow.
> 2) taken fast, delete fast, performance, size delta data, format
> 3) performance, export, format taken slow, delta data, size
>
> I think in most case, saving vmstate in an standalone file is better than
> saving it inside qcow2, So suggest treat internal snapshot as block level
> methods and not encourage user to savevm in qcow2 any more.
>
> Implemention details:
> To avoid trouble, this serial have hide ID in create interfaces, this make
> sure no chaos of ID and name will be introduced by these interfaces.
> There is one patch may be common to Pavel's savvm transaction, patch 1/11,
> others are not quite related. Patch 1/11 will not set errp when no snapshot
> find, since patch 3/11 need to distinguish real error case.
>
> Next steps to better full VM snapshot:
> Improve internal snapshot's export capability.
> Better vmstate saving.
>
> Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
> version comes drom Dietmar Maurer.
>
> v3:
> General:
> Rebased after Stenfan's driver-backup patch V6.
>
> Address Eric's comments:
> 4/9: grammar fix and better doc.
> 5/9: parameter name is mandatory now. grammar fix.
> 6/9: redesiged interface: take both id and name as optional parameter, return
> the deleted snapshot's info.
>
> Address Stefan's comments:
> 4/9: add '' around %s in message. drop code comments about vm_clock.
> 9/9: better doc, refined the code and add more test case.
>
> v4:
> Address Stefan's comments:
> 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(),
> spell fix and better doc.
> 5/9: better doc.
> 6/9: remove spurious ';' in code, spell fix and better doc.
>
> v5:
> Address Kevin's comments:
> 3/8, 4/8, 8/8: remove the limit of numeric snapshot name.
> General change:
> 4/8: use existing type as parameter in qapi schema.
>
> Wenchao Xia (8):
> 1 snapshot: new function bdrv_snapshot_find_by_id_and_name()
> 2 snapshot: distinguish id and name in snapshot delete
> 3 qmp: add internal snapshot support in qmp_transaction
> 4 qmp: add interface blockdev-snapshot-internal-sync
> 5 qmp: add interface blockdev-snapshot-delete-internal-sync
> 6 hmp: add interface hmp_snapshot_blkdev_internal
> 7 hmp: add interface hmp_snapshot_delete_blkdev_internal
> 8 qemu-iotests: add 056 internal snapshot for block device test case
>
> block/qcow2-snapshot.c | 66 +++++++++---
> block/qcow2.h | 5 +-
> block/rbd.c | 23 ++++-
> block/sheepdog.c | 5 +-
> block/snapshot.c | 109 ++++++++++++++++++-
> blockdev.c | 191 ++++++++++++++++++++++++++++++++
> hmp-commands.hx | 37 ++++++-
> hmp.c | 22 ++++
> hmp.h | 2 +
> include/block/block_int.h | 5 +-
> include/block/snapshot.h | 11 ++-
> include/qemu-common.h | 3 +
> qapi-schema.json | 65 +++++++++++-
> qemu-img.c | 5 +-
> qmp-commands.hx | 104 ++++++++++++++++--
> savevm.c | 10 ++-
> tests/qemu-iotests/056 | 259 ++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/056.out | 5 +
> tests/qemu-iotests/group | 1 +
> 19 files changed, 890 insertions(+), 38 deletions(-)
> create mode 100755 tests/qemu-iotests/056
> create mode 100644 tests/qemu-iotests/056.out
Kevin: Are you happy with the issue you pointed out last revision?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level
2013-07-11 5:46 [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Wenchao Xia
` (8 preceding siblings ...)
2013-07-18 11:09 ` [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level Stefan Hajnoczi
@ 2013-07-18 12:34 ` Kevin Wolf
2013-07-19 5:32 ` Stefan Hajnoczi
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2013-07-18 12:34 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> This series brings internal snapshot support at block devices level, now we
> have two three methods to do block snapshot lively: 1) backing chain,
> 2) internal one and 3) drive-back up approach.
>
> Comparation:
> Advantages: Disadvantages:
> 1) delta data, taken fast, export, size performance, delete slow.
> 2) taken fast, delete fast, performance, size delta data, format
> 3) performance, export, format taken slow, delta data, size
>
> I think in most case, saving vmstate in an standalone file is better than
> saving it inside qcow2, So suggest treat internal snapshot as block level
> methods and not encourage user to savevm in qcow2 any more.
>
> Implemention details:
> To avoid trouble, this serial have hide ID in create interfaces, this make
> sure no chaos of ID and name will be introduced by these interfaces.
> There is one patch may be common to Pavel's savvm transaction, patch 1/11,
> others are not quite related. Patch 1/11 will not set errp when no snapshot
> find, since patch 3/11 need to distinguish real error case.
>
> Next steps to better full VM snapshot:
> Improve internal snapshot's export capability.
> Better vmstate saving.
>
> Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
> version comes drom Dietmar Maurer.
>
> v3:
> General:
> Rebased after Stenfan's driver-backup patch V6.
>
> Address Eric's comments:
> 4/9: grammar fix and better doc.
> 5/9: parameter name is mandatory now. grammar fix.
> 6/9: redesiged interface: take both id and name as optional parameter, return
> the deleted snapshot's info.
>
> Address Stefan's comments:
> 4/9: add '' around %s in message. drop code comments about vm_clock.
> 9/9: better doc, refined the code and add more test case.
>
> v4:
> Address Stefan's comments:
> 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(),
> spell fix and better doc.
> 5/9: better doc.
> 6/9: remove spurious ';' in code, spell fix and better doc.
>
> v5:
> Address Kevin's comments:
> 3/8, 4/8, 8/8: remove the limit of numeric snapshot name.
> General change:
> 4/8: use existing type as parameter in qapi schema.
>
> Wenchao Xia (8):
> 1 snapshot: new function bdrv_snapshot_find_by_id_and_name()
> 2 snapshot: distinguish id and name in snapshot delete
> 3 qmp: add internal snapshot support in qmp_transaction
> 4 qmp: add interface blockdev-snapshot-internal-sync
> 5 qmp: add interface blockdev-snapshot-delete-internal-sync
> 6 hmp: add interface hmp_snapshot_blkdev_internal
> 7 hmp: add interface hmp_snapshot_delete_blkdev_internal
> 8 qemu-iotests: add 056 internal snapshot for block device test case
Okay, reviewed the whole series. I had some comments on two or three
patches.
One thing that I want to add is that in order to provide transactional
behaviour (i.e. snapshotting multiple device in one transaction should
result in a consistent snapshot), I think we may in fact need to stop
the VM while taking the snapshots.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/8] add internal snapshot support at block device level
2013-07-18 12:34 ` Kevin Wolf
@ 2013-07-19 5:32 ` Stefan Hajnoczi
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2013-07-19 5:32 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, dietmar, qemu-devel, armbru, lcapitulino, pbonzini,
Wenchao Xia
On Thu, Jul 18, 2013 at 02:34:52PM +0200, Kevin Wolf wrote:
> Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> > This series brings internal snapshot support at block devices level, now we
> > have two three methods to do block snapshot lively: 1) backing chain,
> > 2) internal one and 3) drive-back up approach.
> >
> > Comparation:
> > Advantages: Disadvantages:
> > 1) delta data, taken fast, export, size performance, delete slow.
> > 2) taken fast, delete fast, performance, size delta data, format
> > 3) performance, export, format taken slow, delta data, size
> >
> > I think in most case, saving vmstate in an standalone file is better than
> > saving it inside qcow2, So suggest treat internal snapshot as block level
> > methods and not encourage user to savevm in qcow2 any more.
> >
> > Implemention details:
> > To avoid trouble, this serial have hide ID in create interfaces, this make
> > sure no chaos of ID and name will be introduced by these interfaces.
> > There is one patch may be common to Pavel's savvm transaction, patch 1/11,
> > others are not quite related. Patch 1/11 will not set errp when no snapshot
> > find, since patch 3/11 need to distinguish real error case.
> >
> > Next steps to better full VM snapshot:
> > Improve internal snapshot's export capability.
> > Better vmstate saving.
> >
> > Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
> > version comes drom Dietmar Maurer.
> >
> > v3:
> > General:
> > Rebased after Stenfan's driver-backup patch V6.
> >
> > Address Eric's comments:
> > 4/9: grammar fix and better doc.
> > 5/9: parameter name is mandatory now. grammar fix.
> > 6/9: redesiged interface: take both id and name as optional parameter, return
> > the deleted snapshot's info.
> >
> > Address Stefan's comments:
> > 4/9: add '' around %s in message. drop code comments about vm_clock.
> > 9/9: better doc, refined the code and add more test case.
> >
> > v4:
> > Address Stefan's comments:
> > 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(),
> > spell fix and better doc.
> > 5/9: better doc.
> > 6/9: remove spurious ';' in code, spell fix and better doc.
> >
> > v5:
> > Address Kevin's comments:
> > 3/8, 4/8, 8/8: remove the limit of numeric snapshot name.
> > General change:
> > 4/8: use existing type as parameter in qapi schema.
> >
> > Wenchao Xia (8):
> > 1 snapshot: new function bdrv_snapshot_find_by_id_and_name()
> > 2 snapshot: distinguish id and name in snapshot delete
> > 3 qmp: add internal snapshot support in qmp_transaction
> > 4 qmp: add interface blockdev-snapshot-internal-sync
> > 5 qmp: add interface blockdev-snapshot-delete-internal-sync
> > 6 hmp: add interface hmp_snapshot_blkdev_internal
> > 7 hmp: add interface hmp_snapshot_delete_blkdev_internal
> > 8 qemu-iotests: add 056 internal snapshot for block device test case
>
> Okay, reviewed the whole series. I had some comments on two or three
> patches.
>
> One thing that I want to add is that in order to provide transactional
> behaviour (i.e. snapshotting multiple device in one transaction should
> result in a consistent snapshot), I think we may in fact need to stop
> the VM while taking the snapshots.
If we are doing guest memory snapshots we definitely need to stop vcpus.
For disk-only snapshots I do not think it's necessary to stop the guest
because:
1. Guest I/O can only be processed from the QEMU global mutex. But we
currently hold it so no one else can make progress.
2. In the dataplane case the drain callback that I added stops the
dataplane thread temporarily. It cannot be restarted until the main
loop iterates again (see #1).
Therefore I think stopping vcpus is not necessary for disk-only
snapshots.
Stefan
^ permalink raw reply [flat|nested] 20+ messages in thread