* [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error()
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
@ 2013-04-24 15:31 ` Pavel Hrdina
2013-04-24 16:44 ` Eric Blake
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions Pavel Hrdina
` (12 subsequent siblings)
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Later in the patch series we will use this function a few times.
This will avoid duplicating the code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
qemu-img.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index cd096a1..ab83fbe 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -322,6 +322,16 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
return 0;
}
+static int qemu_img_handle_error(Error *err)
+{
+ if (error_is_set(&err)) {
+ error_report("%s", error_get_pretty(err));
+ error_free(err);
+ return 1;
+ }
+ return 0;
+}
+
static int img_create(int argc, char **argv)
{
int c;
@@ -400,13 +410,8 @@ static int img_create(int argc, char **argv)
bdrv_img_create(filename, fmt, base_filename, base_fmt,
options, img_size, BDRV_O_FLAGS, &local_err, quiet);
- if (error_is_set(&local_err)) {
- error_report("%s", error_get_pretty(local_err));
- error_free(local_err);
- return 1;
- }
- return 0;
+ return qemu_img_handle_error(local_err);
}
static void dump_json_image_check(ImageCheck *check, bool quiet)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error()
2013-04-24 15:31 ` [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error() Pavel Hrdina
@ 2013-04-24 16:44 ` Eric Blake
2013-04-25 2:53 ` Wenchao Xia
0 siblings, 1 reply; 44+ messages in thread
From: Eric Blake @ 2013-04-24 16:44 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 940 bytes --]
On 04/24/2013 09:31 AM, Pavel Hrdina wrote:
> Later in the patch series we will use this function a few times.
> This will avoid duplicating the code.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> qemu-img.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> +static int qemu_img_handle_error(Error *err)
> +{
> + if (error_is_set(&err)) {
> + error_report("%s", error_get_pretty(err));
> + error_free(err);
> + return 1;
> + }
> + return 0;
Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
instead of 0/1 is a bit nicer at expressing why we chose a positive
value; but that would be a separate cleanup to all of qemu-img.c.
Hence, I have no problems giving:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error()
2013-04-24 16:44 ` Eric Blake
@ 2013-04-25 2:53 ` Wenchao Xia
2013-04-25 3:00 ` Eric Blake
0 siblings, 1 reply; 44+ messages in thread
From: Wenchao Xia @ 2013-04-25 2:53 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, armbru, Pavel Hrdina, qemu-devel, lcapitulino
于 2013-4-25 0:44, Eric Blake 写道:
> On 04/24/2013 09:31 AM, Pavel Hrdina wrote:
>> Later in the patch series we will use this function a few times.
>> This will avoid duplicating the code.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> qemu-img.c | 17 +++++++++++------
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>
>
>>
>> +static int qemu_img_handle_error(Error *err)
>> +{
>> + if (error_is_set(&err)) {
>> + error_report("%s", error_get_pretty(err));
>> + error_free(err);
>> + return 1;
>> + }
>> + return 0;
>
> Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
> instead of 0/1 is a bit nicer at expressing why we chose a positive
> value; but that would be a separate cleanup to all of qemu-img.c.
> Hence, I have no problems giving:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Maybe an incode comments like:
+/* Returns 1 on error. */
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error()
2013-04-25 2:53 ` Wenchao Xia
@ 2013-04-25 3:00 ` Eric Blake
0 siblings, 0 replies; 44+ messages in thread
From: Eric Blake @ 2013-04-25 3:00 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, Pavel Hrdina, qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]
On 04/24/2013 08:53 PM, Wenchao Xia wrote:
>>>
>>> +static int qemu_img_handle_error(Error *err)
>>> +{
>>> + if (error_is_set(&err)) {
>>> + error_report("%s", error_get_pretty(err));
>>> + error_free(err);
>>> + return 1;
>>> + }
>>> + return 0;
>>
>> Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
>> instead of 0/1 is a bit nicer at expressing why we chose a positive
>> value; but that would be a separate cleanup to all of qemu-img.c.
>> Hence, I have no problems giving:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> Maybe an incode comments like:
> +/* Returns 1 on error. */
That would also help. My main concern was that +1 on error is unusual
compared to most of qemu that returns <0 on error. The _reason_ it is a
positive number is because we are really returning EXIT_FAILURE (a
well-defined constant from system headers) - but calling the number by
its name is smarter than just using a magic number without explanation.
But as I already said, that's a bigger problem for a different series.
>
> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
2013-04-24 15:31 ` [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error() Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-04-24 20:05 ` Eric Blake
` (3 more replies)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter Pavel Hrdina
` (11 subsequent siblings)
13 siblings, 4 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 21 +++++++++++++--------
block/qcow2-snapshot.c | 19 +++++++++++++------
block/qcow2.h | 4 +++-
block/rbd.c | 10 +++++++---
block/sheepdog.c | 6 ++++--
include/block/block.h | 4 +++-
include/block/block_int.h | 4 +++-
qemu-img.c | 8 +++-----
savevm.c | 32 ++++++++++++++++----------------
9 files changed, 65 insertions(+), 43 deletions(-)
diff --git a/block.c b/block.c
index aa9a533..3f7ee38 100644
--- a/block.c
+++ b/block.c
@@ -3438,16 +3438,21 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return -ENOTSUP;
}
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+void bdrv_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_delete)
- return drv->bdrv_snapshot_delete(bs, snapshot_id);
- if (bs->file)
- return bdrv_snapshot_delete(bs->file, snapshot_id);
- return -ENOTSUP;
+
+ if (!drv) {
+ error_setg(errp, "Device has no medium");
+ } else if (drv->bdrv_snapshot_delete) {
+ drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
+ } else if (bs->file) {
+ bdrv_snapshot_delete(bs->file, snapshot_id, errp);
+ } else {
+ error_setg(errp, "Snapshots are not supported");
+ }
}
int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 992a5c8..fd0e231 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -530,7 +530,9 @@ fail:
return ret;
}
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot sn;
@@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
/* Search the snapshot */
snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
if (snapshot_index < 0) {
- return -ENOENT;
+ error_setg(errp, "Failed to find snapshot '%s' by id or name",
+ snapshot_id);
+ return;
}
sn = s->snapshots[snapshot_index];
@@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
s->nb_snapshots--;
ret = qcow2_write_snapshots(bs);
if (ret < 0) {
- return ret;
+ error_setg_errno(errp, -ret, "Failed to remove snapshot '%s' from "
+ "snapshot list", sn.name);
+ return;
}
/*
@@ -567,14 +573,16 @@ 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) {
- return ret;
+ error_setg_errno(errp, -ret, "Failed to update refcounts");
+ return;
}
qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
/* 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) {
- return ret;
+ error_setg_errno(errp, -ret, "Failed to update cluster flags");
+ return;
}
#ifdef DEBUG_ALLOC
@@ -583,7 +591,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
qcow2_check_refcounts(bs, &result, 0);
}
#endif
- return 0;
}
int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
diff --git a/block/qcow2.h b/block/qcow2.h
index 9421843..dbd332d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -384,7 +384,9 @@ 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);
+void qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ 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 1826411..1e54c55 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -899,14 +899,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
return 0;
}
-static int qemu_rbd_snap_remove(BlockDriverState *bs,
- const char *snapshot_name)
+static void qemu_rbd_snap_remove(BlockDriverState *bs,
+ const char *snapshot_name,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
r = rbd_snap_remove(s->image, snapshot_name);
- return r;
+ if (r < 0) {
+ error_setg_errno(errp, -r, "Failed to remove snapshot '%s'",
+ snapshot_name);
+ }
}
static int qemu_rbd_snap_rollback(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 20b5d06..7e0610f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1937,10 +1937,12 @@ out:
return ret;
}
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static void sd_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
/* FIXME: Delete specified snapshot id. */
- return 0;
+ error_setg(errp, "Deleting snapshot is not supported");
}
static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
diff --git a/include/block/block.h b/include/block/block.h
index 1251c5c..abb636d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -337,7 +337,9 @@ 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);
+void bdrv_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp);
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6078dd3..c3f67c3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -156,7 +156,9 @@ 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);
+ void (*bdrv_snapshot_delete)(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp);
int (*bdrv_snapshot_list)(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index ab83fbe..9cf3467 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1932,6 +1932,7 @@ static int img_snapshot(int argc, char **argv)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn;
+ Error *local_err = NULL;
char *filename, *snapshot_name = NULL;
int c, ret = 0, bdrv_oflags;
int action = 0;
@@ -2029,11 +2030,8 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
- ret = bdrv_snapshot_delete(bs, snapshot_name);
- if (ret) {
- error_report("Could not delete snapshot '%s': %d (%s)",
- snapshot_name, ret, strerror(-ret));
- }
+ bdrv_snapshot_delete(bs, snapshot_name, &local_err);
+ ret = qemu_img_handle_error(local_err);
break;
}
diff --git a/savevm.c b/savevm.c
index 31dcce9..ba97c41 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2291,18 +2291,20 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
- int ret;
+ Error *local_err = NULL;
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0)
{
- ret = bdrv_snapshot_delete(bs, name);
- if (ret < 0) {
- monitor_printf(mon,
- "Error while deleting snapshot on '%s'\n",
- bdrv_get_device_name(bs));
+ bdrv_snapshot_delete(bs, name, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
+ "old snapshot on device '%s': %s",
+ bdrv_get_device_name(bs),
+ error_get_pretty(local_err));
+ error_free(local_err);
return -1;
}
}
@@ -2516,7 +2518,7 @@ int load_vmstate(const char *name)
void do_delvm(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
- int ret;
+ Error *local_err = NULL;
const char *name = qdict_get_str(qdict, "name");
bs = bdrv_snapshots();
@@ -2528,15 +2530,13 @@ 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);
- if (ret < 0) {
- if (ret == -ENOTSUP)
- monitor_printf(mon,
- "Snapshots not supported on device '%s'\n",
- bdrv_get_device_name(bs1));
- else
- monitor_printf(mon, "Error %d while deleting snapshot on "
- "'%s'\n", ret, bdrv_get_device_name(bs1));
+ bdrv_snapshot_delete(bs1, name, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
+ "old snapshot on device '%s': %s",
+ bdrv_get_device_name(bs),
+ error_get_pretty(local_err));
+ error_free(local_err);
}
}
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions Pavel Hrdina
@ 2013-04-24 20:05 ` Eric Blake
2013-04-25 3:19 ` Wenchao Xia
` (2 subsequent siblings)
3 siblings, 0 replies; 44+ messages in thread
From: Eric Blake @ 2013-04-24 20:05 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 21 +++++++++++++--------
> block/qcow2-snapshot.c | 19 +++++++++++++------
> block/qcow2.h | 4 +++-
> block/rbd.c | 10 +++++++---
> block/sheepdog.c | 6 ++++--
> include/block/block.h | 4 +++-
> include/block/block_int.h | 4 +++-
> qemu-img.c | 8 +++-----
> savevm.c | 32 ++++++++++++++++----------------
> 9 files changed, 65 insertions(+), 43 deletions(-)
>
> +++ b/block/sheepdog.c
> @@ -1937,10 +1937,12 @@ out:
> return ret;
> }
>
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> /* FIXME: Delete specified snapshot id. */
> - return 0;
> + error_setg(errp, "Deleting snapshot is not supported");
Commit message should mention this bug fix.
But the code looked okay to me, so:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions Pavel Hrdina
2013-04-24 20:05 ` Eric Blake
@ 2013-04-25 3:19 ` Wenchao Xia
2013-04-25 13:42 ` Stefan Hajnoczi
2013-05-03 9:53 ` Kevin Wolf
3 siblings, 0 replies; 44+ messages in thread
From: Wenchao Xia @ 2013-04-25 3:19 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, lcapitulino, qemu-devel, armbru
bdrv_snapshot_delete() do not return error number now, and I checked
original caller code, they used it only for error reporting, so it is
safe to change.
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 21 +++++++++++++--------
> block/qcow2-snapshot.c | 19 +++++++++++++------
> block/qcow2.h | 4 +++-
> block/rbd.c | 10 +++++++---
> block/sheepdog.c | 6 ++++--
> include/block/block.h | 4 +++-
> include/block/block_int.h | 4 +++-
> qemu-img.c | 8 +++-----
> savevm.c | 32 ++++++++++++++++----------------
> 9 files changed, 65 insertions(+), 43 deletions(-)
>
> diff --git a/block.c b/block.c
> index aa9a533..3f7ee38 100644
> --- a/block.c
> +++ b/block.c
> @@ -3438,16 +3438,21 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> return -ENOTSUP;
> }
>
> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +void bdrv_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> BlockDriver *drv = bs->drv;
> - if (!drv)
> - return -ENOMEDIUM;
> - if (drv->bdrv_snapshot_delete)
> - return drv->bdrv_snapshot_delete(bs, snapshot_id);
> - if (bs->file)
> - return bdrv_snapshot_delete(bs->file, snapshot_id);
> - return -ENOTSUP;
> +
> + if (!drv) {
> + error_setg(errp, "Device has no medium");
> + } else if (drv->bdrv_snapshot_delete) {
> + drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
> + } else if (bs->file) {
> + bdrv_snapshot_delete(bs->file, snapshot_id, errp);
> + } else {
> + error_setg(errp, "Snapshots are not supported");
> + }
> }
>
> int bdrv_snapshot_list(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 992a5c8..fd0e231 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -530,7 +530,9 @@ fail:
> return ret;
> }
>
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +void qcow2_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot sn;
> @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> /* Search the snapshot */
> snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> if (snapshot_index < 0) {
> - return -ENOENT;
> + error_setg(errp, "Failed to find snapshot '%s' by id or name",
> + snapshot_id);
> + return;
> }
> sn = s->snapshots[snapshot_index];
>
> @@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> s->nb_snapshots--;
> ret = qcow2_write_snapshots(bs);
> if (ret < 0) {
> - return ret;
> + error_setg_errno(errp, -ret, "Failed to remove snapshot '%s' from "
> + "snapshot list", sn.name);
> + return;
> }
>
> /*
> @@ -567,14 +573,16 @@ 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) {
> - return ret;
> + error_setg_errno(errp, -ret, "Failed to update refcounts");
> + return;
> }
> qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
>
> /* 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) {
> - return ret;
> + error_setg_errno(errp, -ret, "Failed to update cluster flags");
> + return;
> }
>
> #ifdef DEBUG_ALLOC
> @@ -583,7 +591,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> qcow2_check_refcounts(bs, &result, 0);
> }
> #endif
> - return 0;
> }
>
> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 9421843..dbd332d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -384,7 +384,9 @@ 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);
> +void qcow2_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + 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 1826411..1e54c55 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -899,14 +899,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
> return 0;
> }
>
> -static int qemu_rbd_snap_remove(BlockDriverState *bs,
> - const char *snapshot_name)
> +static void qemu_rbd_snap_remove(BlockDriverState *bs,
> + const char *snapshot_name,
> + Error **errp)
> {
> BDRVRBDState *s = bs->opaque;
> int r;
>
> r = rbd_snap_remove(s->image, snapshot_name);
> - return r;
> + if (r < 0) {
> + error_setg_errno(errp, -r, "Failed to remove snapshot '%s'",
> + snapshot_name);
> + }
> }
>
> static int qemu_rbd_snap_rollback(BlockDriverState *bs,
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 20b5d06..7e0610f 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1937,10 +1937,12 @@ out:
> return ret;
> }
>
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> /* FIXME: Delete specified snapshot id. */
> - return 0;
> + error_setg(errp, "Deleting snapshot is not supported");
> }
>
> static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> diff --git a/include/block/block.h b/include/block/block.h
> index 1251c5c..abb636d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -337,7 +337,9 @@ 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);
> +void bdrv_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp);
> int bdrv_snapshot_list(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_info);
> int bdrv_snapshot_load_tmp(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 6078dd3..c3f67c3 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -156,7 +156,9 @@ 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);
> + void (*bdrv_snapshot_delete)(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp);
> int (*bdrv_snapshot_list)(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_info);
> int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
> diff --git a/qemu-img.c b/qemu-img.c
> index ab83fbe..9cf3467 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1932,6 +1932,7 @@ static int img_snapshot(int argc, char **argv)
> {
> BlockDriverState *bs;
> QEMUSnapshotInfo sn;
> + Error *local_err = NULL;
> char *filename, *snapshot_name = NULL;
> int c, ret = 0, bdrv_oflags;
> int action = 0;
> @@ -2029,11 +2030,8 @@ static int img_snapshot(int argc, char **argv)
> break;
>
> case SNAPSHOT_DELETE:
> - ret = bdrv_snapshot_delete(bs, snapshot_name);
> - if (ret) {
> - error_report("Could not delete snapshot '%s': %d (%s)",
> - snapshot_name, ret, strerror(-ret));
> - }
> + bdrv_snapshot_delete(bs, snapshot_name, &local_err);
> + ret = qemu_img_handle_error(local_err);
> break;
> }
>
> diff --git a/savevm.c b/savevm.c
> index 31dcce9..ba97c41 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2291,18 +2291,20 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> {
> BlockDriverState *bs;
> QEMUSnapshotInfo sn1, *snapshot = &sn1;
> - int ret;
> + Error *local_err = NULL;
>
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs) &&
> bdrv_snapshot_find(bs, snapshot, name) >= 0)
> {
> - ret = bdrv_snapshot_delete(bs, name);
> - if (ret < 0) {
> - monitor_printf(mon,
> - "Error while deleting snapshot on '%s'\n",
> - bdrv_get_device_name(bs));
> + bdrv_snapshot_delete(bs, name, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
> + "old snapshot on device '%s': %s",
> + bdrv_get_device_name(bs),
> + error_get_pretty(local_err));
> + error_free(local_err);
> return -1;
> }
> }
> @@ -2516,7 +2518,7 @@ int load_vmstate(const char *name)
> void do_delvm(Monitor *mon, const QDict *qdict)
> {
> BlockDriverState *bs, *bs1;
> - int ret;
> + Error *local_err = NULL;
> const char *name = qdict_get_str(qdict, "name");
>
> bs = bdrv_snapshots();
> @@ -2528,15 +2530,13 @@ 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);
> - if (ret < 0) {
> - if (ret == -ENOTSUP)
> - monitor_printf(mon,
> - "Snapshots not supported on device '%s'\n",
> - bdrv_get_device_name(bs1));
> - else
> - monitor_printf(mon, "Error %d while deleting snapshot on "
> - "'%s'\n", ret, bdrv_get_device_name(bs1));
> + bdrv_snapshot_delete(bs1, name, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
> + "old snapshot on device '%s': %s",
> + bdrv_get_device_name(bs),
> + error_get_pretty(local_err));
> + error_free(local_err);
> }
> }
> }
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions Pavel Hrdina
2013-04-24 20:05 ` Eric Blake
2013-04-25 3:19 ` Wenchao Xia
@ 2013-04-25 13:42 ` Stefan Hajnoczi
2013-05-03 9:53 ` Kevin Wolf
3 siblings, 0 replies; 44+ messages in thread
From: Stefan Hajnoczi @ 2013-04-25 13:42 UTC (permalink / raw)
To: Pavel Hrdina
Cc: kwolf, armbru, qemu-devel, lcapitulino, xiawenc, MORITA Kazutaka
On Wed, Apr 24, 2013 at 05:32:00PM +0200, Pavel Hrdina wrote:
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 20b5d06..7e0610f 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1937,10 +1937,12 @@ out:
> return ret;
> }
>
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> /* FIXME: Delete specified snapshot id. */
> - return 0;
> + error_setg(errp, "Deleting snapshot is not supported");
> }
Careful, this could break existing tests or applications that expect
snapshot delete to lie. I suggest *not* setting the error for now,
unless Kazutaka agrees it's okay to start erroring now.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions Pavel Hrdina
` (2 preceding siblings ...)
2013-04-25 13:42 ` Stefan Hajnoczi
@ 2013-05-03 9:53 ` Kevin Wolf
3 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 9:53 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> @@ -2528,15 +2530,13 @@ 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);
> - if (ret < 0) {
> - if (ret == -ENOTSUP)
> - monitor_printf(mon,
> - "Snapshots not supported on device '%s'\n",
> - bdrv_get_device_name(bs1));
> - else
> - monitor_printf(mon, "Error %d while deleting snapshot on "
> - "'%s'\n", ret, bdrv_get_device_name(bs1));
> + bdrv_snapshot_delete(bs1, name, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
> + "old snapshot on device '%s': %s",
Here in do_delvm() it doesn't make sense to talk about an "old" snapshot.
Probably some unchanged copy & paste from above?
> + bdrv_get_device_name(bs),
> + error_get_pretty(local_err));
> + error_free(local_err);
> }
> }
> }
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
2013-04-24 15:31 ` [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error() Pavel Hrdina
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 02/12] block: update error reporting for bdrv_snapshot_delete() and related functions Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-04-24 21:26 ` Eric Blake
` (2 more replies)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm Pavel Hrdina
` (10 subsequent siblings)
13 siblings, 3 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Finding snapshot by a name which could also be an id isn't best way
how to do it. There will be rewrite of savevm, loadvm and delvm to
improve the behavior of these commands. The savevm and loadvm will
have their own patch series.
Now bdrv_snapshot_find takes more parameters. The name parameter will
be matched only against the name of the snapshot and the same applies
to id parameter.
There is one exception. If you set the last parameter, the name parameter
will be matched against the name or the id of a snapshot. This exception
is only for backward compatibility for other commands and it will be
dropped after all commands will be rewritten.
We only need to know if that snapshot exists or not. We don't care
about any error message. If snapshot exists it returns TRUE otherwise
it returns FALSE.
There is also new Error parameter which will containt error messeage if
something goes wrong.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 67 insertions(+), 26 deletions(-)
diff --git a/savevm.c b/savevm.c
index ba97c41..1622c55 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2262,26 +2262,66 @@ out:
return ret;
}
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name)
+static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+ const char *name, const char *id, Error **errp,
+ bool old_match)
{
QEMUSnapshotInfo *sn_tab, *sn;
- int nb_sns, i, ret;
+ int nb_sns, i;
+ bool found = false;
+
+ assert(name || id);
- ret = -ENOENT;
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- if (nb_sns < 0)
- return ret;
- for(i = 0; i < nb_sns; i++) {
+ if (nb_sns < 0) {
+ error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+ return found;
+ }
+
+ if (nb_sns == 0) {
+ error_setg(errp, "Device has no snapshots");
+ return found;
+ }
+
+ for (i = 0; i < nb_sns; i++) {
sn = &sn_tab[i];
- if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
- *sn_info = *sn;
- ret = 0;
- break;
+ if (name && id) {
+ if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ found = true;
+ break;
+ }
+ } else if (name) {
+ /* for compatibility for old bdrv_snapshot_find call
+ * will be removed */
+ if (old_match) {
+ if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ found = true;
+ break;
+ }
+ } else {
+ if (!strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ found = true;
+ break;
+ }
+ }
+ } else if (id) {
+ if (!strcmp(sn->id_str, id)) {
+ *sn_info = *sn;
+ found = true;
+ break;
+ }
}
}
+
+ if (!found) {
+ error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
+ }
+
g_free(sn_tab);
- return ret;
+ return found;
}
/*
@@ -2296,7 +2336,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs) &&
- bdrv_snapshot_find(bs, snapshot, name) >= 0)
+ bdrv_snapshot_find(bs, snapshot, name, NULL, NULL, true))
{
bdrv_snapshot_delete(bs, name, &local_err);
if (error_is_set(&local_err)) {
@@ -2358,8 +2398,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
if (name) {
- ret = bdrv_snapshot_find(bs, old_sn, name);
- if (ret >= 0) {
+ if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
} else {
@@ -2440,6 +2479,7 @@ int load_vmstate(const char *name)
BlockDriverState *bs, *bs_vm_state;
QEMUSnapshotInfo sn;
QEMUFile *f;
+ Error *local_err = NULL;
int ret;
bs_vm_state = bdrv_snapshots();
@@ -2449,9 +2489,10 @@ int load_vmstate(const char *name)
}
/* Don't even try to load empty VM states */
- ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
- if (ret < 0) {
- return ret;
+ if (!bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, &local_err, true)) {
+ error_report("Snapshot doesn't exist: %s", error_get_pretty(local_err));
+ error_free(local_err);
+ return -ENOENT;
} else if (sn.vm_state_size == 0) {
error_report("This is a disk-only snapshot. Revert to it offline "
"using qemu-img.");
@@ -2473,11 +2514,11 @@ int load_vmstate(const char *name)
return -ENOTSUP;
}
- ret = bdrv_snapshot_find(bs, &sn, name);
- if (ret < 0) {
- error_report("Device '%s' does not have the requested snapshot '%s'",
- bdrv_get_device_name(bs), name);
- return ret;
+ if (!bdrv_snapshot_find(bs, &sn, name, NULL, &local_err, true)) {
+ error_report("Snapshot doesn't exist for device '%s': %s",
+ bdrv_get_device_name(bs), error_get_pretty(local_err));
+ error_free(local_err);
+ return -ENOENT;
}
}
@@ -2546,7 +2587,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
- int nb_sns, i, ret, available;
+ int nb_sns, i, available;
int total;
int *available_snapshots;
char buf[256];
@@ -2577,8 +2618,8 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
- if (ret < 0) {
+ if (!bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL, NULL,
+ true)) {
available = 0;
break;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter Pavel Hrdina
@ 2013-04-24 21:26 ` Eric Blake
2013-04-25 6:46 ` Pavel Hrdina
2013-04-25 6:31 ` Wenchao Xia
2013-05-03 10:24 ` Kevin Wolf
2 siblings, 1 reply; 44+ messages in thread
From: Eric Blake @ 2013-04-24 21:26 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 5036 bytes --]
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> Finding snapshot by a name which could also be an id isn't best way
> how to do it. There will be rewrite of savevm, loadvm and delvm to
> improve the behavior of these commands. The savevm and loadvm will
> have their own patch series.
>
> Now bdrv_snapshot_find takes more parameters. The name parameter will
> be matched only against the name of the snapshot and the same applies
> to id parameter.
>
> There is one exception. If you set the last parameter, the name parameter
> will be matched against the name or the id of a snapshot. This exception
> is only for backward compatibility for other commands and it will be
> dropped after all commands will be rewritten.
>
> We only need to know if that snapshot exists or not. We don't care
> about any error message. If snapshot exists it returns TRUE otherwise
> it returns FALSE.
>
> There is also new Error parameter which will containt error messeage if
double-typo and grammar:
s/also/also a/
s/containt error messeage/contain the error message/
> something goes wrong.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 67 insertions(+), 26 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index ba97c41..1622c55 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2262,26 +2262,66 @@ out:
> return ret;
> }
>
> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> - const char *name)
> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> + const char *name, const char *id, Error **errp,
> + bool old_match)
I'd add a FIXME comment here documenting that you intend to remove the
old_match parameter after all callers have been updated to the new
semantics.
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> - int nb_sns, i, ret;
> + int nb_sns, i;
> + bool found = false;
Bikeshedding: I don't think you need this variable, if you would instead
do...
> +
> + assert(name || id);
>
> - ret = -ENOENT;
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> - if (nb_sns < 0)
> - return ret;
> - for(i = 0; i < nb_sns; i++) {
> + if (nb_sns < 0) {
> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> + return found;
return false;
> + }
> +
> + if (nb_sns == 0) {
> + error_setg(errp, "Device has no snapshots");
> + return found;
return false;
> + }
*sn_info = NULL;
> +
> + for (i = 0; i < nb_sns; i++) {
> sn = &sn_tab[i];
> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> - *sn_info = *sn;
> - ret = 0;
> - break;
> + if (name && id) {
> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
Drop this assignment and others like it...
> + break;
> + }
> + } else if (name) {
> + /* for compatibility for old bdrv_snapshot_find call
> + * will be removed */
> + if (old_match) {
> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + } else {
> + if (!strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + }
> + } else if (id) {
> + if (!strcmp(sn->id_str, id)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> }
> }
> +
> + if (!found) {
use 'if (*sn_info)'
> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
> + }
> +
> g_free(sn_tab);
> - return ret;
> + return found;
return *sn_info != NULL;
> }
If you _do_ decide to keep the boolean variable instead of hard-coding a
false return and avoiding redundancy by using other variables to
determine the result, then at least s/found/ret/, because I find 'return
found' as a way to intentionally fail rather odd-looking.
At any rate, I can live with this logic, and all the conversions of
existing call sites properly passed the given name, NULL id, and true
for old_match semantics; along with optional deciding whether to pass
NULL or a local error based on whether it would ignore lookup failure or
propagate it as a failure of the higher-level operation that needed a
lookup.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-24 21:26 ` Eric Blake
@ 2013-04-25 6:46 ` Pavel Hrdina
2013-04-25 8:18 ` Pavel Hrdina
0 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-25 6:46 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
On 24.4.2013 23:26, Eric Blake wrote:
> On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
>> Finding snapshot by a name which could also be an id isn't best way
>> how to do it. There will be rewrite of savevm, loadvm and delvm to
>> improve the behavior of these commands. The savevm and loadvm will
>> have their own patch series.
>>
>> Now bdrv_snapshot_find takes more parameters. The name parameter will
>> be matched only against the name of the snapshot and the same applies
>> to id parameter.
>>
>> There is one exception. If you set the last parameter, the name parameter
>> will be matched against the name or the id of a snapshot. This exception
>> is only for backward compatibility for other commands and it will be
>> dropped after all commands will be rewritten.
>>
>> We only need to know if that snapshot exists or not. We don't care
>> about any error message. If snapshot exists it returns TRUE otherwise
>> it returns FALSE.
>>
>> There is also new Error parameter which will containt error messeage if
>
> double-typo and grammar:
> s/also/also a/
> s/containt error messeage/contain the error message/
>
>> something goes wrong.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 67 insertions(+), 26 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index ba97c41..1622c55 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2262,26 +2262,66 @@ out:
>> return ret;
>> }
>>
>> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> - const char *name)
>> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> + const char *name, const char *id, Error **errp,
>> + bool old_match)
>
> I'd add a FIXME comment here documenting that you intend to remove the
> old_match parameter after all callers have been updated to the new
> semantics.
>
>> {
>> QEMUSnapshotInfo *sn_tab, *sn;
>> - int nb_sns, i, ret;
>> + int nb_sns, i;
>> + bool found = false;
>
> Bikeshedding: I don't think you need this variable, if you would instead
> do...
>
>> +
>> + assert(name || id);
>>
>> - ret = -ENOENT;
>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> - if (nb_sns < 0)
>> - return ret;
>> - for(i = 0; i < nb_sns; i++) {
>> + if (nb_sns < 0) {
>> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>> + return found;
>
> return false;
>
>> + }
>> +
>> + if (nb_sns == 0) {
>> + error_setg(errp, "Device has no snapshots");
>> + return found;
>
> return false;
>
>> + }
>
> *sn_info = NULL;
You cannot do that. It should be sn_info = NULL, but if you look at the
usage of the bdrv_snapshot_find you will see that you cannot do that
too. And the same applies ...
>
>> +
>> + for (i = 0; i < nb_sns; i++) {
>> sn = &sn_tab[i];
>> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>> - *sn_info = *sn;
>> - ret = 0;
>> - break;
>> + if (name && id) {
>> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>
> Drop this assignment and others like it...
>
>> + break;
>> + }
>> + } else if (name) {
>> + /* for compatibility for old bdrv_snapshot_find call
>> + * will be removed */
>> + if (old_match) {
>> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + } else {
>> + if (!strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + }
>> + } else if (id) {
>> + if (!strcmp(sn->id_str, id)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> }
>> }
>> +
>> + if (!found) {
>
> use 'if (*sn_info)'
to this usage and ...
>
>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
>> + }
>> +
>> g_free(sn_tab);
>> - return ret;
>> + return found;
>
> return *sn_info != NULL;
this one.
>
>> }
>
> If you _do_ decide to keep the boolean variable instead of hard-coding a
> false return and avoiding redundancy by using other variables to
> determine the result, then at least s/found/ret/, because I find 'return
> found' as a way to intentionally fail rather odd-looking.
>
> At any rate, I can live with this logic, and all the conversions of
> existing call sites properly passed the given name, NULL id, and true
> for old_match semantics; along with optional deciding whether to pass
> NULL or a local error based on whether it would ignore lookup failure or
> propagate it as a failure of the higher-level operation that needed a
> lookup.
>
You are right that avoiding redundancy is better and I just think up a
new solution.
static QEMUSnapshotInfo *bdrv_snapshot_find(BlockDriverState *bs,
const char *name,
const char *id,
Error **errp,
bool old_match /*FIXME*/)
And the bdrv_snapshot_find will return QEMUSnapshotInfo* on success and
on error it will set the error message and also return NULL.
Pavel
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-25 6:46 ` Pavel Hrdina
@ 2013-04-25 8:18 ` Pavel Hrdina
0 siblings, 0 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-25 8:18 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, armbru, qemu-devel, xiawenc, lcapitulino
On 25.4.2013 08:46, Pavel Hrdina wrote:
> On 24.4.2013 23:26, Eric Blake wrote:
>> On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
>>> Finding snapshot by a name which could also be an id isn't best way
>>> how to do it. There will be rewrite of savevm, loadvm and delvm to
>>> improve the behavior of these commands. The savevm and loadvm will
>>> have their own patch series.
>>>
>>> Now bdrv_snapshot_find takes more parameters. The name parameter will
>>> be matched only against the name of the snapshot and the same applies
>>> to id parameter.
>>>
>>> There is one exception. If you set the last parameter, the name
>>> parameter
>>> will be matched against the name or the id of a snapshot. This exception
>>> is only for backward compatibility for other commands and it will be
>>> dropped after all commands will be rewritten.
>>>
>>> We only need to know if that snapshot exists or not. We don't care
>>> about any error message. If snapshot exists it returns TRUE otherwise
>>> it returns FALSE.
>>>
>>> There is also new Error parameter which will containt error messeage if
>>
>> double-typo and grammar:
>> s/also/also a/
>> s/containt error messeage/contain the error message/
>>
>>> something goes wrong.
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>> savevm.c | 93
>>> ++++++++++++++++++++++++++++++++++++++++++++++------------------
>>> 1 file changed, 67 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/savevm.c b/savevm.c
>>> index ba97c41..1622c55 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -2262,26 +2262,66 @@ out:
>>> return ret;
>>> }
>>>
>>> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo
>>> *sn_info,
>>> - const char *name)
>>> +static bool bdrv_snapshot_find(BlockDriverState *bs,
>>> QEMUSnapshotInfo *sn_info,
>>> + const char *name, const char *id,
>>> Error **errp,
>>> + bool old_match)
>>
>> I'd add a FIXME comment here documenting that you intend to remove the
>> old_match parameter after all callers have been updated to the new
>> semantics.
>>
>>> {
>>> QEMUSnapshotInfo *sn_tab, *sn;
>>> - int nb_sns, i, ret;
>>> + int nb_sns, i;
>>> + bool found = false;
>>
>> Bikeshedding: I don't think you need this variable, if you would instead
>> do...
>>
>>> +
>>> + assert(name || id);
>>>
>>> - ret = -ENOENT;
>>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>>> - if (nb_sns < 0)
>>> - return ret;
>>> - for(i = 0; i < nb_sns; i++) {
>>> + if (nb_sns < 0) {
>>> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot
>>> list");
>>> + return found;
>>
>> return false;
>>
>>> + }
>>> +
>>> + if (nb_sns == 0) {
>>> + error_setg(errp, "Device has no snapshots");
>>> + return found;
>>
>> return false;
>>
>>> + }
>>
>> *sn_info = NULL;
>
> You cannot do that. It should be sn_info = NULL, but if you look at the
> usage of the bdrv_snapshot_find you will see that you cannot do that
> too. And the same applies ...
>
>>
>>> +
>>> + for (i = 0; i < nb_sns; i++) {
>>> sn = &sn_tab[i];
>>> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>> - *sn_info = *sn;
>>> - ret = 0;
>>> - break;
>>> + if (name && id) {
>>> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>>> + *sn_info = *sn;
>>> + found = true;
>>
>> Drop this assignment and others like it...
>>
>>> + break;
>>> + }
>>> + } else if (name) {
>>> + /* for compatibility for old bdrv_snapshot_find call
>>> + * will be removed */
>>> + if (old_match) {
>>> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name,
>>> name)) {
>>> + *sn_info = *sn;
>>> + found = true;
>>> + break;
>>> + }
>>> + } else {
>>> + if (!strcmp(sn->name, name)) {
>>> + *sn_info = *sn;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> + } else if (id) {
>>> + if (!strcmp(sn->id_str, id)) {
>>> + *sn_info = *sn;
>>> + found = true;
>>> + break;
>>> + }
>>> }
>>> }
>>> +
>>> + if (!found) {
>>
>> use 'if (*sn_info)'
>
> to this usage and ...
>
>>
>>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name
>>> : id);
>>> + }
>>> +
>>> g_free(sn_tab);
>>> - return ret;
>>> + return found;
>>
>> return *sn_info != NULL;
>
> this one.
>
>>
>>> }
>>
>> If you _do_ decide to keep the boolean variable instead of hard-coding a
>> false return and avoiding redundancy by using other variables to
>> determine the result, then at least s/found/ret/, because I find 'return
>> found' as a way to intentionally fail rather odd-looking.
>>
>> At any rate, I can live with this logic, and all the conversions of
>> existing call sites properly passed the given name, NULL id, and true
>> for old_match semantics; along with optional deciding whether to pass
>> NULL or a local error based on whether it would ignore lookup failure or
>> propagate it as a failure of the higher-level operation that needed a
>> lookup.
>>
>
> You are right that avoiding redundancy is better and I just think up a
> new solution.
>
> static QEMUSnapshotInfo *bdrv_snapshot_find(BlockDriverState *bs,
> const char *name,
> const char *id,
> Error **errp,
> bool old_match /*FIXME*/)
>
> And the bdrv_snapshot_find will return QEMUSnapshotInfo* on success and
> on error it will set the error message and also return NULL.
>
> Pavel
>
Well, this doesn't work too. I'll stick with the bool return value and
the bool ret variable.
Pavel
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter Pavel Hrdina
2013-04-24 21:26 ` Eric Blake
@ 2013-04-25 6:31 ` Wenchao Xia
2013-04-25 6:52 ` Pavel Hrdina
2013-04-25 12:16 ` Eric Blake
2013-05-03 10:24 ` Kevin Wolf
2 siblings, 2 replies; 44+ messages in thread
From: Wenchao Xia @ 2013-04-25 6:31 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, lcapitulino, qemu-devel, armbru
> Finding snapshot by a name which could also be an id isn't best way
> how to do it. There will be rewrite of savevm, loadvm and delvm to
> improve the behavior of these commands. The savevm and loadvm will
> have their own patch series.
>
> Now bdrv_snapshot_find takes more parameters. The name parameter will
> be matched only against the name of the snapshot and the same applies
> to id parameter.
>
> There is one exception. If you set the last parameter, the name parameter
> will be matched against the name or the id of a snapshot. This exception
> is only for backward compatibility for other commands and it will be
> dropped after all commands will be rewritten.
>
> We only need to know if that snapshot exists or not. We don't care
> about any error message. If snapshot exists it returns TRUE otherwise
> it returns FALSE.
>
> There is also new Error parameter which will containt error messeage if
> something goes wrong.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 67 insertions(+), 26 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index ba97c41..1622c55 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2262,26 +2262,66 @@ out:
> return ret;
> }
>
> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> - const char *name)
> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> + const char *name, const char *id, Error **errp,
> + bool old_match)
suggest directly drop old_match parameter here, or squash patch 12
into this one, mark the change in commit message, then the logic will
be clearer.
Personally hope to place parameter *id before *name.
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> - int nb_sns, i, ret;
> + int nb_sns, i;
> + bool found = false;
> +
> + assert(name || id);
>
> - ret = -ENOENT;
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> - if (nb_sns < 0)
> - return ret;
> - for(i = 0; i < nb_sns; i++) {
> + if (nb_sns < 0) {
> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> + return found;
> + }
> +
> + if (nb_sns == 0) {
> + error_setg(errp, "Device has no snapshots");
> + return found;
> + }
suggest not set errp here, which can be used to tip exception, but
having not found one is a normal case.
> +
> + for (i = 0; i < nb_sns; i++) {
> sn = &sn_tab[i];
> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> - *sn_info = *sn;
> - ret = 0;
> - break;
> + if (name && id) {
> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + } else if (name) {
> + /* for compatibility for old bdrv_snapshot_find call
> + * will be removed */
> + if (old_match) {
> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + } else {
> + if (!strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + }
> + } else if (id) {
> + if (!strcmp(sn->id_str, id)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> }
> }
suggest change the sequence:
if (name && id) {
for () {
}
} else if (name){
for () {
}
} else if (id) {
for () {
}
}
slightly faster, and make logic more clear.
> +
> + if (!found) {
> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
suggest not to set error, since it is a normal case.
> + }
> +
> g_free(sn_tab);
> - return ret;
> + return found;
> }
>
> /*
> @@ -2296,7 +2336,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs) &&
> - bdrv_snapshot_find(bs, snapshot, name) >= 0)
> + bdrv_snapshot_find(bs, snapshot, name, NULL, NULL, true))
> {
> bdrv_snapshot_delete(bs, name, &local_err);
> if (error_is_set(&local_err)) {
> @@ -2358,8 +2398,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>
> if (name) {
> - ret = bdrv_snapshot_find(bs, old_sn, name);
> - if (ret >= 0) {
> + if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
> pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> } else {
> @@ -2440,6 +2479,7 @@ int load_vmstate(const char *name)
> BlockDriverState *bs, *bs_vm_state;
> QEMUSnapshotInfo sn;
> QEMUFile *f;
> + Error *local_err = NULL;
> int ret;
>
> bs_vm_state = bdrv_snapshots();
> @@ -2449,9 +2489,10 @@ int load_vmstate(const char *name)
> }
>
> /* Don't even try to load empty VM states */
> - ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> - if (ret < 0) {
> - return ret;
> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, &local_err, true)) {
> + error_report("Snapshot doesn't exist: %s", error_get_pretty(local_err));
> + error_free(local_err);
> + return -ENOENT;
> } else if (sn.vm_state_size == 0) {
> error_report("This is a disk-only snapshot. Revert to it offline "
> "using qemu-img.");
> @@ -2473,11 +2514,11 @@ int load_vmstate(const char *name)
> return -ENOTSUP;
> }
>
> - ret = bdrv_snapshot_find(bs, &sn, name);
> - if (ret < 0) {
> - error_report("Device '%s' does not have the requested snapshot '%s'",
> - bdrv_get_device_name(bs), name);
> - return ret;
> + if (!bdrv_snapshot_find(bs, &sn, name, NULL, &local_err, true)) {
> + error_report("Snapshot doesn't exist for device '%s': %s",
> + bdrv_get_device_name(bs), error_get_pretty(local_err));
> + error_free(local_err);
> + return -ENOENT;
> }
> }
>
> @@ -2546,7 +2587,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> {
> BlockDriverState *bs, *bs1;
> QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
> - int nb_sns, i, ret, available;
> + int nb_sns, i, available;
> int total;
> int *available_snapshots;
> char buf[256];
> @@ -2577,8 +2618,8 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>
> while ((bs1 = bdrv_next(bs1))) {
> if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> - if (ret < 0) {
> + if (!bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL, NULL,
> + true)) {
> available = 0;
> break;
> }
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-25 6:31 ` Wenchao Xia
@ 2013-04-25 6:52 ` Pavel Hrdina
2013-04-25 12:16 ` Eric Blake
1 sibling, 0 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-25 6:52 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, armbru
On 25.4.2013 08:31, Wenchao Xia wrote:
>> Finding snapshot by a name which could also be an id isn't best way
>> how to do it. There will be rewrite of savevm, loadvm and delvm to
>> improve the behavior of these commands. The savevm and loadvm will
>> have their own patch series.
>>
>> Now bdrv_snapshot_find takes more parameters. The name parameter will
>> be matched only against the name of the snapshot and the same applies
>> to id parameter.
>>
>> There is one exception. If you set the last parameter, the name parameter
>> will be matched against the name or the id of a snapshot. This exception
>> is only for backward compatibility for other commands and it will be
>> dropped after all commands will be rewritten.
>>
>> We only need to know if that snapshot exists or not. We don't care
>> about any error message. If snapshot exists it returns TRUE otherwise
>> it returns FALSE.
>>
>> There is also new Error parameter which will containt error messeage if
>> something goes wrong.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 67 insertions(+), 26 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index ba97c41..1622c55 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2262,26 +2262,66 @@ out:
>> return ret;
>> }
>>
>> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> - const char *name)
>> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> + const char *name, const char *id, Error **errp,
>> + bool old_match)
> suggest directly drop old_match parameter here, or squash patch 12
> into this one, mark the change in commit message, then the logic will
> be clearer.
> Personally hope to place parameter *id before *name.
That make sense, I'll change it.
>
>> {
>> QEMUSnapshotInfo *sn_tab, *sn;
>> - int nb_sns, i, ret;
>> + int nb_sns, i;
>> + bool found = false;
>> +
>> + assert(name || id);
>>
>> - ret = -ENOENT;
>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> - if (nb_sns < 0)
>> - return ret;
>> - for(i = 0; i < nb_sns; i++) {
>> + if (nb_sns < 0) {
>> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>> + return found;
>> + }
>> +
>> + if (nb_sns == 0) {
>> + error_setg(errp, "Device has no snapshots");
>> + return found;
>> + }
> suggest not set errp here, which can be used to tip exception, but
> having not found one is a normal case.
You don't have to use the error message. It is set as the error message
but actually is more like an announcement.
>
>> +
>> + for (i = 0; i < nb_sns; i++) {
>> sn = &sn_tab[i];
>> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>> - *sn_info = *sn;
>> - ret = 0;
>> - break;
>> + if (name && id) {
>> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + } else if (name) {
>> + /* for compatibility for old bdrv_snapshot_find call
>> + * will be removed */
>> + if (old_match) {
>> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + } else {
>> + if (!strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + }
>> + } else if (id) {
>> + if (!strcmp(sn->id_str, id)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> }
>> }
> suggest change the sequence:
>
> if (name && id) {
> for () {
> }
> } else if (name){
> for () {
> }
> } else if (id) {
> for () {
> }
> }
> slightly faster, and make logic more clear.
Thanks for suggest, I'll change the logic.
>
>> +
>> + if (!found) {
>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
> suggest not to set error, since it is a normal case.
The same comment as for "Device has no snapshots".
Pavel
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-25 6:31 ` Wenchao Xia
2013-04-25 6:52 ` Pavel Hrdina
@ 2013-04-25 12:16 ` Eric Blake
2013-04-26 2:37 ` Wenchao Xia
1 sibling, 1 reply; 44+ messages in thread
From: Eric Blake @ 2013-04-25 12:16 UTC (permalink / raw)
To: Wenchao Xia; +Cc: lcapitulino, kwolf, Pavel Hrdina, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]
On 04/25/2013 12:31 AM, Wenchao Xia wrote:
>
>> +
>> + if (!found) {
>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
> suggest not to set error, since it is a normal case.
The way I understand it, failure to find a snapshot might need to be
treated as an error - it's up to the caller's needs. Also, there pretty
much is only one failure mode - the requested snapshot was not found -
even if there are multiple ways that we can fail to find a requested
snapshot, so I'm fine with treating all 'false' returns as an error path.
Thus, a caller that wants to probe for a snapshot existence but not set
an error calls:
bdrv_snapshot_find(bs, snapshot, name, id, NULL, false);
while a caller that wants to report a missing snapshot as an error calls:
bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);
and then propagates local_err on upwards.
Or are you worried about a possible third case, where a caller cares
about failure during bdrv_snapshot_list(), differently than failure to
find a snapshot? What callers have that semantics? If that is a real
concern, then maybe returning a bool is the wrong approach, and we
should instead return an int. A return < 0 is a fatal error
(bdrv_snapshot_list failed to even look up snapshots); a return of 0
means our lookup attempt hit no fatal errors but the snapshot was not
found, and a return of 1 means the snapshot was found. Then there would
be three calling styles:
Probe for existence, with no error reporting:
if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) {
// exists
}
Probe for existence but with error reporting on fatal errors:
exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);
if (exist < 0) {
// propagate local_err
} else if (exist) {
// exists
}
Probe for snapshot, with error reporting even for failed lookup:
if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) {
// propagate local_err
}
But I don't know what the existing callers need, to make a decision on
whether a signature change is warranted. Again, more reason to defer
this series to 1.6.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-25 12:16 ` Eric Blake
@ 2013-04-26 2:37 ` Wenchao Xia
0 siblings, 0 replies; 44+ messages in thread
From: Wenchao Xia @ 2013-04-26 2:37 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, armbru, Pavel Hrdina, qemu-devel, lcapitulino
于 2013-4-25 20:16, Eric Blake 写道:
> On 04/25/2013 12:31 AM, Wenchao Xia wrote:
>>
>>> +
>>> + if (!found) {
>>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
>> suggest not to set error, since it is a normal case.
>
> The way I understand it, failure to find a snapshot might need to be
> treated as an error - it's up to the caller's needs. Also, there pretty
> much is only one failure mode - the requested snapshot was not found -
> even if there are multiple ways that we can fail to find a requested
> snapshot, so I'm fine with treating all 'false' returns as an error path.
>
> Thus, a caller that wants to probe for a snapshot existence but not set
> an error calls:
> bdrv_snapshot_find(bs, snapshot, name, id, NULL, false);
>
> while a caller that wants to report a missing snapshot as an error calls:
> bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);
> and then propagates local_err on upwards.
>
>
> Or are you worried about a possible third case, where a caller cares
> about failure during bdrv_snapshot_list(), differently than failure to
> find a snapshot? What callers have that semantics? If that is a real
> concern, then maybe returning a bool is the wrong approach, and we
> should instead return an int. A return < 0 is a fatal error
> (bdrv_snapshot_list failed to even look up snapshots); a return of 0
> means our lookup attempt hit no fatal errors but the snapshot was not
> found, and a return of 1 means the snapshot was found. Then there would
> be three calling styles:
>
> Probe for existence, with no error reporting:
> if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) {
> // exists
> }
> Probe for existence but with error reporting on fatal errors:
> exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);
> if (exist < 0) {
> // propagate local_err
> } else if (exist) {
> // exists
> }
> Probe for snapshot, with error reporting even for failed lookup:
> if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) {
> // propagate local_err
> }
>
> But I don't know what the existing callers need, to make a decision on
> whether a signature change is warranted. Again, more reason to defer
> this series to 1.6.
>
Personally I prefer internal layer have clean meaning, setting error
only for exception. But I am not strongly against it, if caller can
make easier use of it, a document for this function is also OK.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter Pavel Hrdina
2013-04-24 21:26 ` Eric Blake
2013-04-25 6:31 ` Wenchao Xia
@ 2013-05-03 10:24 ` Kevin Wolf
2 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 10:24 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Finding snapshot by a name which could also be an id isn't best way
> how to do it. There will be rewrite of savevm, loadvm and delvm to
> improve the behavior of these commands. The savevm and loadvm will
> have their own patch series.
>
> Now bdrv_snapshot_find takes more parameters. The name parameter will
> be matched only against the name of the snapshot and the same applies
> to id parameter.
>
> There is one exception. If you set the last parameter, the name parameter
> will be matched against the name or the id of a snapshot. This exception
> is only for backward compatibility for other commands and it will be
> dropped after all commands will be rewritten.
>
> We only need to know if that snapshot exists or not. We don't care
> about any error message. If snapshot exists it returns TRUE otherwise
> it returns FALSE.
>
> There is also new Error parameter which will containt error messeage if
> something goes wrong.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 67 insertions(+), 26 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index ba97c41..1622c55 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2262,26 +2262,66 @@ out:
> return ret;
> }
>
> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> - const char *name)
> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> + const char *name, const char *id, Error **errp,
> + bool old_match)
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> - int nb_sns, i, ret;
> + int nb_sns, i;
> + bool found = false;
> +
> + assert(name || id);
>
> - ret = -ENOENT;
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> - if (nb_sns < 0)
> - return ret;
> - for(i = 0; i < nb_sns; i++) {
> + if (nb_sns < 0) {
> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> + return found;
> + }
> +
> + if (nb_sns == 0) {
> + error_setg(errp, "Device has no snapshots");
This is not an error case, please remove the error_setg(). If the caller
indeed needs to have a snapshot, it can generate an error by himself. We
cannot assume here that every caller needs it.
> + return found;
> + }
> +
> + for (i = 0; i < nb_sns; i++) {
> sn = &sn_tab[i];
> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> - *sn_info = *sn;
> - ret = 0;
> - break;
> + if (name && id) {
> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + } else if (name) {
> + /* for compatibility for old bdrv_snapshot_find call
> + * will be removed */
> + if (old_match) {
> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + } else {
> + if (!strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> + }
> + } else if (id) {
> + if (!strcmp(sn->id_str, id)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> + }
> }
> }
> +
> + if (!found) {
> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
> + }
Same here.
> +
> g_free(sn_tab);
> - return ret;
> + return found;
> }
>
> /*
> @@ -2296,7 +2336,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs) &&
> - bdrv_snapshot_find(bs, snapshot, name) >= 0)
> + bdrv_snapshot_find(bs, snapshot, name, NULL, NULL, true))
> {
> bdrv_snapshot_delete(bs, name, &local_err);
> if (error_is_set(&local_err)) {
> @@ -2358,8 +2398,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>
> if (name) {
> - ret = bdrv_snapshot_find(bs, old_sn, name);
> - if (ret >= 0) {
> + if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
> pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> } else {
> @@ -2440,6 +2479,7 @@ int load_vmstate(const char *name)
> BlockDriverState *bs, *bs_vm_state;
> QEMUSnapshotInfo sn;
> QEMUFile *f;
> + Error *local_err = NULL;
> int ret;
>
> bs_vm_state = bdrv_snapshots();
> @@ -2449,9 +2489,10 @@ int load_vmstate(const char *name)
> }
>
> /* Don't even try to load empty VM states */
> - ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> - if (ret < 0) {
> - return ret;
> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, &local_err, true)) {
> + error_report("Snapshot doesn't exist: %s", error_get_pretty(local_err));
"Could not find snapshot" (it could just be an I/O error in reading the
snapshot list)
Or actually, this message is fine for the case where you don't find the
snapshot, but have no error in bdrv_snapshot_find().
Should the device name of bs_vm_state be mentioned in the error message?
> + error_free(local_err);
> + return -ENOENT;
> } else if (sn.vm_state_size == 0) {
> error_report("This is a disk-only snapshot. Revert to it offline "
> "using qemu-img.");
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (2 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-04-24 22:54 ` Eric Blake
2013-05-03 10:50 ` Kevin Wolf
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions Pavel Hrdina
` (9 subsequent siblings)
13 siblings, 2 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
QMP command vm-snapshot-delete takes two parameters: name and id. They are
optional, but one of the name or id must be provided. If both are provided they
will match only the snapshot with the same name and id. The command returns
SnapshotInfo only if the snapshot exists, otherwise it returns an error message.
HMP command delvm now uses the new vm-snapshot-delete, but behave slightly
different. The delvm takes one optional flag -i and one parameter name. If you
provide only the name parameter, it will match only snapshots with that name.
If you also provide the flag, it will match only snapshots with name as id.
Information about successfully deleted snapshot will be printed, otherwise
an error message will be printed.
These improves behavior of the command to be more strict on selecting snapshots
because actual behavior is wrong. Now if you want to delete snapshot with name '2'
but there is no snapshot with that name it could delete snapshot with id '2' and
that isn't what you want.
There is also small hack to ensure that in each block device with different
driver type the correct snapshot is deleted. The 'qcow2' and 'sheepdog' internally
search at first for id but 'rbd' has only name and therefore search only for name.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 14 +++++-----
hmp.c | 33 +++++++++++++++++++++++
hmp.h | 1 +
include/sysemu/sysemu.h | 1 -
qapi-schema.json | 17 ++++++++++++
qmp-commands.hx | 38 +++++++++++++++++++++++++++
savevm.c | 69 +++++++++++++++++++++++++++++++++++++++----------
7 files changed, 153 insertions(+), 20 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index df44906..32f79d9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -339,16 +339,18 @@ ETEXI
{
.name = "delvm",
- .args_type = "name:s",
- .params = "tag|id",
- .help = "delete a VM snapshot from its tag or id",
- .mhandler.cmd = do_delvm,
+ .args_type = "id:-i,name:s",
+ .params = "[-i] name",
+ .help = "delete a VM snapshot by name as tag or with -i flag by name as id",
+ .mhandler.cmd = hmp_vm_snapshot_delete,
},
STEXI
-@item delvm @var{tag}|@var{id}
+@item delvm [-i] @var{name}
@findex delvm
-Delete the snapshot identified by @var{tag} or @var{id}.
+Delete a snapshot identified by @var{name} as tag. If flag -i is provided,
+delete a snapshot identified by @var{name} as id.
+
ETEXI
{
diff --git a/hmp.c b/hmp.c
index 4fb76ec..fea1cee 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1425,3 +1425,36 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err);
hmp_handle_error(mon, &local_err);
}
+
+void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict)
+{
+ const char *name = qdict_get_try_str(qdict, "name");
+ const bool id = qdict_get_try_bool(qdict, "id", false);
+ Error *local_err = NULL;
+ SnapshotInfo *info;
+
+ if (id) {
+ info = qmp_vm_snapshot_delete(false, NULL, true, name, &local_err);
+ } else {
+ info = qmp_vm_snapshot_delete(true, name, false, NULL, &local_err);
+ }
+
+ if (info) {
+ char buf[256];
+ QEMUSnapshotInfo sn = {
+ .vm_state_size = info->vm_state_size,
+ .date_sec = info->date_sec,
+ .date_nsec = info->date_nsec,
+ .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
+ info->vm_clock_nsec,
+ };
+ pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
+ pstrcpy(sn.name, sizeof(sn.name), info->name);
+ monitor_printf(mon, "Deleted snapshot info:\n");
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ }
+
+ qapi_free_SnapshotInfo(info);
+ hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index 95fe76e..b0667b3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -85,5 +85,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
void hmp_chardev_add(Monitor *mon, const QDict *qdict);
void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict);
#endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6578782..f46f9d2 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -67,7 +67,6 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
void do_savevm(Monitor *mon, const QDict *qdict);
int load_vmstate(const char *name);
-void do_delvm(Monitor *mon, const QDict *qdict);
void do_info_snapshots(Monitor *mon, const QDict *qdict);
void qemu_announce_self(void);
diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..0bd61b8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3505,3 +3505,20 @@
'*asl_compiler_rev': 'uint32',
'*file': 'str',
'*data': 'str' }}
+
+##
+# @vm-snapshot-delete:
+#
+# Delete a snapshot identified by name or id or both. One of the name or id
+# is required. It will returns SnapshotInfo of successfully deleted snapshot.
+#
+# @name: #optional tag of an existing snapshot
+#
+# @id: #optional id of an existing snapshot
+#
+# Returns: SnapshotInfo on success
+#
+# Since: 1.5
+##
+{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
+ 'returns': 'SnapshotInfo' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4d65422..9b15cb4 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1423,6 +1423,44 @@ Example:
EQMP
{
+ .name = "vm-snapshot-delete",
+ .args_type = "name:s?,id:s?",
+ .params = "[tag] [id]",
+ .help = "delete a VM snapshot from its tag or id",
+ .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_delete
+ },
+
+SQMP
+vm-snapshot-delete
+------
+
+Delete a snapshot identified by name or id or both. One of the name or id
+is required. It will returns SnapshotInfo of successfully deleted snapshot.
+
+Arguments:
+
+- "name": tag of an existing snapshot (json-string, optional)
+
+- "id": id of an existing snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "vm-snapshot-delete", "arguments": { "name": "my_snapshot" } }
+<- {
+ "return": {
+ "id": "1",
+ "name": "my_snapshot",
+ "date-sec": 1364480534,
+ "date-nsec": 978215000,
+ "vm-clock-sec": 5,
+ "vm-clock-nsec": 153620449,
+ "vm-state-size": 5709953
+ }
+ }
+
+
+EQMP
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/savevm.c b/savevm.c
index 1622c55..1b4aea8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -40,6 +40,7 @@
#include "trace.h"
#include "qemu/bitops.h"
#include "qemu/iov.h"
+#include "block/block_int.h"
#define SELF_ANNOUNCE_ROUNDS 5
@@ -2556,31 +2557,73 @@ int load_vmstate(const char *name)
return 0;
}
-void do_delvm(Monitor *mon, const QDict *qdict)
+SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
+ const bool has_id, const char *id,
+ Error **errp)
{
- BlockDriverState *bs, *bs1;
+ BlockDriverState *bs;
+ SnapshotInfo *info = NULL;
+ QEMUSnapshotInfo sn;
Error *local_err = NULL;
- const char *name = qdict_get_str(qdict, "name");
+
+ if (!has_name && !has_id) {
+ error_setg(errp, "Name or id must be provided");
+ return NULL;
+ }
+
+ if (!has_name) {
+ name = NULL;
+ }
+ if (!has_id) {
+ id = NULL;
+ }
bs = bdrv_snapshots();
if (!bs) {
- monitor_printf(mon, "No block device supports snapshots\n");
- return;
+ error_setg(errp, "No block device supports snapshots");
+ return NULL;
}
- bs1 = NULL;
- while ((bs1 = bdrv_next(bs1))) {
- if (bdrv_can_snapshot(bs1)) {
- bdrv_snapshot_delete(bs1, name, &local_err);
+ if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
+ 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;
+
+ bs = NULL;
+ while ((bs = bdrv_next(bs))) {
+ if (bdrv_can_snapshot(bs) &&
+ bdrv_snapshot_find(bs, &sn, name, id, NULL, false)) {
+ /* Small hack to ensure that proper snapshot is deleted for every
+ * block driver. This will be fixed soon. */
+ if (!strcmp(bs->drv->format_name, "rbd")) {
+ bdrv_snapshot_delete(bs, sn.name, &local_err);
+ } else {
+ bdrv_snapshot_delete(bs, sn.id_str, &local_err);
+ }
if (error_is_set(&local_err)) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
- "old snapshot on device '%s': %s",
- bdrv_get_device_name(bs),
- error_get_pretty(local_err));
+ error_setg(errp, "Failed to delete old snapshot on "
+ "device '%s': %s", bdrv_get_device_name(bs),
+ error_get_pretty(local_err));
error_free(local_err);
}
}
}
+
+ if (error_is_set(errp)) {
+ g_free(info);
+ return NULL;
+ }
+
+ return info;
}
void do_info_snapshots(Monitor *mon, const QDict *qdict)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm Pavel Hrdina
@ 2013-04-24 22:54 ` Eric Blake
2013-04-25 6:58 ` Wenchao Xia
2013-05-03 10:50 ` Kevin Wolf
1 sibling, 1 reply; 44+ messages in thread
From: Eric Blake @ 2013-04-24 22:54 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> QMP command vm-snapshot-delete takes two parameters: name and id. They are
> optional, but one of the name or id must be provided. If both are provided they
> will match only the snapshot with the same name and id. The command returns
> SnapshotInfo only if the snapshot exists, otherwise it returns an error message.
>
> HMP command delvm now uses the new vm-snapshot-delete, but behave slightly
s/but behave/and behaves/
> different. The delvm takes one optional flag -i and one parameter name. If you
> provide only the name parameter, it will match only snapshots with that name.
> If you also provide the flag, it will match only snapshots with name as id.
> Information about successfully deleted snapshot will be printed, otherwise
> an error message will be printed.
>
> These improves behavior of the command to be more strict on selecting snapshots
> because actual behavior is wrong. Now if you want to delete snapshot with name '2'
> but there is no snapshot with that name it could delete snapshot with id '2' and
> that isn't what you want.
>
> There is also small hack to ensure that in each block device with different
> driver type the correct snapshot is deleted. The 'qcow2' and 'sheepdog' internally
> search at first for id but 'rbd' has only name and therefore search only for name.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> hmp-commands.hx | 14 +++++-----
> hmp.c | 33 +++++++++++++++++++++++
> hmp.h | 1 +
> include/sysemu/sysemu.h | 1 -
> qapi-schema.json | 17 ++++++++++++
> qmp-commands.hx | 38 +++++++++++++++++++++++++++
> savevm.c | 69 +++++++++++++++++++++++++++++++++++++++----------
> 7 files changed, 153 insertions(+), 20 deletions(-)
>
> +void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict)
> +{
> + const char *name = qdict_get_try_str(qdict, "name");
> + const bool id = qdict_get_try_bool(qdict, "id", false);
Don't know that the 'const' is really needed here, but doesn't hurt.
> + Error *local_err = NULL;
> + SnapshotInfo *info;
> +
> + if (id) {
> + info = qmp_vm_snapshot_delete(false, NULL, true, name, &local_err);
> + } else {
> + info = qmp_vm_snapshot_delete(true, name, false, NULL, &local_err);
> + }
> +
> + if (info) {
> + char buf[256];
I know this fixed-size buffer is just a copy-and-paste from other code
that displays snapshot information, but I really hate it. On the other
hand, I can tolerate if we have it as an intermediate step between two
series that both land in the same release.
If your series goes in first, Wenchao's series that cleans up the
fixed-size buffer will need to be rebased to tweak this additional spot.
If Wenchao's patches go in first, then you will have a bit of rebase
work to do. Since we are already deferring this series into 1.6, I
think it would be nice to post a unified series of the best of both
authors, rather than continuing to waffle on what should go in first.
[And if I keep saying that often enough, I may end up getting my hands
dirty and becoming the person that posts such a unified patch, although
generally I don't like forcefully taking over someone else's initial work.]
> +++ b/qapi-schema.json
> @@ -3505,3 +3505,20 @@
> '*asl_compiler_rev': 'uint32',
> '*file': 'str',
> '*data': 'str' }}
> +
> +##
> +# @vm-snapshot-delete:
> +#
> +# Delete a snapshot identified by name or id or both. One of the name or id
> +# is required. It will returns SnapshotInfo of successfully deleted snapshot.
s/returns/return/
> +#
> +# @name: #optional tag of an existing snapshot
> +#
> +# @id: #optional id of an existing snapshot
> +#
> +# Returns: SnapshotInfo on success
> +#
> +# Since: 1.5
1.6, now that we are deferring.
> +##
> +{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
> + 'returns': 'SnapshotInfo' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..9b15cb4 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1423,6 +1423,44 @@ Example:
>
> EQMP
> {
> + .name = "vm-snapshot-delete",
> + .args_type = "name:s?,id:s?",
> + .params = "[tag] [id]",
> + .help = "delete a VM snapshot from its tag or id",
Sounds slightly better if you do s/from/based on/
> + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_delete
> + },
> +
> +SQMP
> +vm-snapshot-delete
> +------
> +
> +Delete a snapshot identified by name or id or both. One of the name or id
> +is required. It will returns SnapshotInfo of successfully deleted snapshot.
s/returns/return/
> @@ -2556,31 +2557,73 @@ int load_vmstate(const char *name)
> return 0;
> }
>
> -void do_delvm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> + const bool has_id, const char *id,
> + Error **errp)
Actual function looks right to me.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
2013-04-24 22:54 ` Eric Blake
@ 2013-04-25 6:58 ` Wenchao Xia
2013-04-25 12:21 ` Eric Blake
0 siblings, 1 reply; 44+ messages in thread
From: Wenchao Xia @ 2013-04-25 6:58 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, kwolf, Pavel Hrdina, qemu-devel, armbru
>> + char buf[256];
>
> I know this fixed-size buffer is just a copy-and-paste from other code
> that displays snapshot information, but I really hate it. On the other
> hand, I can tolerate if we have it as an intermediate step between two
> series that both land in the same release.
>
> If your series goes in first, Wenchao's series that cleans up the
> fixed-size buffer will need to be rebased to tweak this additional spot.
> If Wenchao's patches go in first, then you will have a bit of rebase
> work to do. Since we are already deferring this series into 1.6, I
> think it would be nice to post a unified series of the best of both
> authors, rather than continuing to waffle on what should go in first.
That would be a very long serial, taking time to rebase for any code
change in it, that is why I haven't consider it before.
> [And if I keep saying that often enough, I may end up getting my hands
> dirty and becoming the person that posts such a unified patch, although
Pls don't, I guess it would not be a good experience working in a
long serial which may need modification later.
> generally I don't like forcefully taking over someone else's initial work.]
>
My serial serves mainly for block image's info querying, different
with Pavel, one serial fixing all is not easy to make.
Instead, I'll send out small serial change the common part:
1 better bdrv_snapshot_find().
2 hmp/qemu-img dumping info code().
Then we rebase on it, as two serial, do you think it is OK?
>> +++ b/qapi-schema.json
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
2013-04-25 6:58 ` Wenchao Xia
@ 2013-04-25 12:21 ` Eric Blake
2013-04-26 2:39 ` Wenchao Xia
0 siblings, 1 reply; 44+ messages in thread
From: Eric Blake @ 2013-04-25 12:21 UTC (permalink / raw)
To: Wenchao Xia; +Cc: lcapitulino, kwolf, Pavel Hrdina, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 2646 bytes --]
On 04/25/2013 12:58 AM, Wenchao Xia wrote:
>
>>> + char buf[256];
>>
>> I know this fixed-size buffer is just a copy-and-paste from other code
>> that displays snapshot information, but I really hate it. On the other
>> hand, I can tolerate if we have it as an intermediate step between two
>> series that both land in the same release.
>>
>> If your series goes in first, Wenchao's series that cleans up the
>> fixed-size buffer will need to be rebased to tweak this additional spot.
>> If Wenchao's patches go in first, then you will have a bit of rebase
>> work to do. Since we are already deferring this series into 1.6, I
>> think it would be nice to post a unified series of the best of both
>> authors, rather than continuing to waffle on what should go in first.
> That would be a very long serial, taking time to rebase for any code
> change in it, that is why I haven't consider it before.
s/serial/series/
But it would at least have the end goal in mind, instead of trying to
debate what the end goal is between two different series.
>
>> [And if I keep saying that often enough, I may end up getting my hands
>> dirty and becoming the person that posts such a unified patch, although
> Pls don't, I guess it would not be a good experience working in a
> long serial which may need modification later.
Sometimes, letting an additional author join in on attempting to post
patches can be productive. But I certainly don't want to make it feel
like a hostile takeover - we've got plenty of time before 1.6, so I
don't mind letting you work through a few more revisions of the series.
> My serial serves mainly for block image's info querying, different
> with Pavel, one serial fixing all is not easy to make.
> Instead, I'll send out small serial change the common part:
> 1 better bdrv_snapshot_find().
> 2 hmp/qemu-img dumping info code().
> Then we rebase on it, as two serial, do you think it is OK?
Splitting into pieces is also okay, as long as the pieces make sense. I
see several piecemeal changes being attempted between the two series
with several conflicts if we don't factor out common parts, such as
moving snapshot-related code into a new file, making snapshot lookup
cleaner, removing hard-coded length limits on HMP snapshot display.
Yes, getting the common parts clean as one series, then doing two more
relatively-independent series of 1. better query output, 2. QMP
counterpart to snapshot manipulations, is probably workable.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
2013-04-25 12:21 ` Eric Blake
@ 2013-04-26 2:39 ` Wenchao Xia
0 siblings, 0 replies; 44+ messages in thread
From: Wenchao Xia @ 2013-04-26 2:39 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, kwolf, Pavel Hrdina, qemu-devel, armbru
于 2013-4-25 20:21, Eric Blake 写道:
> On 04/25/2013 12:58 AM, Wenchao Xia wrote:
>>
>>>> + char buf[256];
>>>
>>> I know this fixed-size buffer is just a copy-and-paste from other code
>>> that displays snapshot information, but I really hate it. On the other
>>> hand, I can tolerate if we have it as an intermediate step between two
>>> series that both land in the same release.
>>>
>>> If your series goes in first, Wenchao's series that cleans up the
>>> fixed-size buffer will need to be rebased to tweak this additional spot.
>>> If Wenchao's patches go in first, then you will have a bit of rebase
>>> work to do. Since we are already deferring this series into 1.6, I
>>> think it would be nice to post a unified series of the best of both
>>> authors, rather than continuing to waffle on what should go in first.
>> That would be a very long serial, taking time to rebase for any code
>> change in it, that is why I haven't consider it before.
>
> s/serial/series/
>
> But it would at least have the end goal in mind, instead of trying to
> debate what the end goal is between two different series.
>
>>
>>> [And if I keep saying that often enough, I may end up getting my hands
>>> dirty and becoming the person that posts such a unified patch, although
>> Pls don't, I guess it would not be a good experience working in a
>> long serial which may need modification later.
>
> Sometimes, letting an additional author join in on attempting to post
> patches can be productive. But I certainly don't want to make it feel
> like a hostile takeover - we've got plenty of time before 1.6, so I
> don't mind letting you work through a few more revisions of the series.
>
>> My serial serves mainly for block image's info querying, different
>> with Pavel, one serial fixing all is not easy to make.
>> Instead, I'll send out small serial change the common part:
>> 1 better bdrv_snapshot_find().
>> 2 hmp/qemu-img dumping info code().
>> Then we rebase on it, as two serial, do you think it is OK?
>
> Splitting into pieces is also okay, as long as the pieces make sense. I
> see several piecemeal changes being attempted between the two series
> with several conflicts if we don't factor out common parts, such as
> moving snapshot-related code into a new file, making snapshot lookup
> cleaner, removing hard-coded length limits on HMP snapshot display.
> Yes, getting the common parts clean as one series, then doing two more
> relatively-independent series of 1. better query output, 2. QMP
> counterpart to snapshot manipulations, is probably workable.
>
OK, I'll send out 2 serial clean the common part. Pavel, please
wait for mine serial before v3.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm Pavel Hrdina
2013-04-24 22:54 ` Eric Blake
@ 2013-05-03 10:50 ` Kevin Wolf
1 sibling, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 10:50 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-delete takes two parameters: name and id. They are
> optional, but one of the name or id must be provided. If both are provided they
> will match only the snapshot with the same name and id. The command returns
> SnapshotInfo only if the snapshot exists, otherwise it returns an error message.
>
> HMP command delvm now uses the new vm-snapshot-delete, but behave slightly
> different. The delvm takes one optional flag -i and one parameter name. If you
> provide only the name parameter, it will match only snapshots with that name.
> If you also provide the flag, it will match only snapshots with name as id.
> Information about successfully deleted snapshot will be printed, otherwise
> an error message will be printed.
>
> These improves behavior of the command to be more strict on selecting snapshots
> because actual behavior is wrong. Now if you want to delete snapshot with name '2'
> but there is no snapshot with that name it could delete snapshot with id '2' and
> that isn't what you want.
>
> There is also small hack to ensure that in each block device with different
> driver type the correct snapshot is deleted. The 'qcow2' and 'sheepdog' internally
> search at first for id but 'rbd' has only name and therefore search only for name.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
One general comment: I'm not sure how much sense it really makes to
delete snapshots based on ID on multiple images. The user doesn't have
any influence on the ID, I think, so a VM-wide snapshot can only be
identified by name across multiple images.
> --- a/savevm.c
> +++ b/savevm.c
> @@ -40,6 +40,7 @@
> #include "trace.h"
> #include "qemu/bitops.h"
> #include "qemu/iov.h"
> +#include "block/block_int.h"
>
> #define SELF_ANNOUNCE_ROUNDS 5
>
> @@ -2556,31 +2557,73 @@ int load_vmstate(const char *name)
> return 0;
> }
>
> -void do_delvm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> + const bool has_id, const char *id,
> + Error **errp)
> {
> - BlockDriverState *bs, *bs1;
> + BlockDriverState *bs;
> + SnapshotInfo *info = NULL;
> + QEMUSnapshotInfo sn;
> Error *local_err = NULL;
> - const char *name = qdict_get_str(qdict, "name");
> +
> + if (!has_name && !has_id) {
> + error_setg(errp, "Name or id must be provided");
> + return NULL;
> + }
> +
> + if (!has_name) {
> + name = NULL;
> + }
> + if (!has_id) {
> + id = NULL;
> + }
>
> bs = bdrv_snapshots();
> if (!bs) {
> - monitor_printf(mon, "No block device supports snapshots\n");
> - return;
> + error_setg(errp, "No block device supports snapshots");
> + return NULL;
> }
>
> - bs1 = NULL;
> - while ((bs1 = bdrv_next(bs1))) {
> - if (bdrv_can_snapshot(bs1)) {
> - bdrv_snapshot_delete(bs1, name, &local_err);
> + if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
> + error_propagate(errp, local_err);
> + return NULL;
> + }
Why is this necessary? The check didn't exist before.
> +
> + 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;
> +
> + bs = NULL;
> + while ((bs = bdrv_next(bs))) {
> + if (bdrv_can_snapshot(bs) &&
> + bdrv_snapshot_find(bs, &sn, name, id, NULL, false)) {
Aha, this is the reason: The command is underspecified and you change
some behaviour that isn't mentioned in the documentation. Before, the
command would return an error if a device that supports snapshots
doesn't have the specific snapshot. After the patch, it would silently
ignore the snapshot - except in bdrv_snapshots(), which is more or less
randomly selected.
Why is this an improvement?
Independent of what we decide here, the result should be added to the
QMP documentation.
> + /* Small hack to ensure that proper snapshot is deleted for every
> + * block driver. This will be fixed soon. */
> + if (!strcmp(bs->drv->format_name, "rbd")) {
> + bdrv_snapshot_delete(bs, sn.name, &local_err);
> + } else {
> + bdrv_snapshot_delete(bs, sn.id_str, &local_err);
> + }
> if (error_is_set(&local_err)) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
> - "old snapshot on device '%s': %s",
> - bdrv_get_device_name(bs),
> - error_get_pretty(local_err));
> + error_setg(errp, "Failed to delete old snapshot on "
> + "device '%s': %s", bdrv_get_device_name(bs),
> + error_get_pretty(local_err));
> error_free(local_err);
> }
You can't use error_setg() multiple times on the same errp, the second
call would trigger an assertion failure. Should we immediately break
after an error?
> }
> }
> +
> + if (error_is_set(errp)) {
> + g_free(info);
> + return NULL;
> + }
> +
> + return info;
> }
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (3 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-04-25 17:06 ` Eric Blake
2013-05-03 11:03 ` Kevin Wolf
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 06/12] block: update error reporting for bdrv_snapshot_list() " Pavel Hrdina
` (8 subsequent siblings)
13 siblings, 2 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 34 ++++++++++++++++++----------------
block/qcow2-snapshot.c | 24 +++++++++++++++++-------
block/qcow2.h | 4 +++-
block/rbd.c | 10 +++++++---
block/sheepdog.c | 18 ++++++++----------
include/block/block.h | 5 +++--
include/block/block_int.h | 5 +++--
qemu-img.c | 7 ++-----
savevm.c | 10 +++++-----
9 files changed, 66 insertions(+), 51 deletions(-)
diff --git a/block.c b/block.c
index 3f7ee38..c36d3d5 100644
--- a/block.c
+++ b/block.c
@@ -3412,30 +3412,32 @@ int bdrv_snapshot_create(BlockDriverState *bs,
return -ENOTSUP;
}
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id)
+void bdrv_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
- int ret, open_ret;
-
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_goto)
- return drv->bdrv_snapshot_goto(bs, snapshot_id);
+ int ret;
- if (bs->file) {
+ if (!drv) {
+ error_setg(errp, "Device has no medium");
+ } else if (drv->bdrv_snapshot_goto) {
+ drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
+ } else if (bs->file) {
drv->bdrv_close(bs);
- ret = bdrv_snapshot_goto(bs->file, snapshot_id);
- open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
- if (open_ret < 0) {
+ bdrv_snapshot_goto(bs->file, snapshot_id, errp);
+ ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+ if (ret < 0) {
bdrv_delete(bs->file);
bs->drv = NULL;
- return open_ret;
+ if (!error_is_set(errp)) {
+ error_setg_errno(errp, -ret, "Failed to open '%s'",
+ bdrv_get_device_name(bs));
+ }
}
- return ret;
+ } else {
+ error_setg(errp, "Snapshots are not supported");
}
-
- return -ENOTSUP;
}
void bdrv_snapshot_delete(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index fd0e231..67a57fc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -417,7 +417,9 @@ fail:
}
/* copy the snapshot 'snapshot_name' into the current disk image */
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot *sn;
@@ -429,14 +431,15 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
/* Search the snapshot */
snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
if (snapshot_index < 0) {
- return -ENOENT;
+ error_setg(errp, "Failed to find snapshot '%s' by id or name",
+ snapshot_id);
+ return;
}
sn = &s->snapshots[snapshot_index];
if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
- error_report("qcow2: Loading snapshots with different disk "
- "size is not implemented");
- ret = -ENOTSUP;
+ error_setg(errp, "Loading snapshots with different disk size is not "
+ "implemented");
goto fail;
}
@@ -447,6 +450,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
*/
ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to grow L1 table");
goto fail;
}
@@ -465,18 +469,22 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
ret = bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to load data into L1 table");
goto fail;
}
ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset,
sn->l1_size, 1);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to increase the cluster refcounts "
+ "of the snapshot to load");
goto fail;
}
ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
cur_l1_bytes);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to save L1 table");
goto fail;
}
@@ -502,6 +510,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
}
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to decrease the cluster refcounts "
+ "of the old snapshot");
goto fail;
}
@@ -514,6 +524,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
*/
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to update the cluster flags");
goto fail;
}
@@ -523,11 +534,10 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
qcow2_check_refcounts(bs, &result, 0);
}
#endif
- return 0;
+ return;
fail:
g_free(sn_l1_table);
- return ret;
}
void qcow2_snapshot_delete(BlockDriverState *bs,
diff --git a/block/qcow2.h b/block/qcow2.h
index dbd332d..b70387c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -383,7 +383,9 @@ 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);
+void qcow2_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp);
void qcow2_snapshot_delete(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
diff --git a/block/rbd.c b/block/rbd.c
index 1e54c55..5047b59 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -913,14 +913,18 @@ static void qemu_rbd_snap_remove(BlockDriverState *bs,
}
}
-static int qemu_rbd_snap_rollback(BlockDriverState *bs,
- const char *snapshot_name)
+static void qemu_rbd_snap_rollback(BlockDriverState *bs,
+ const char *snapshot_name,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
r = rbd_snap_rollback(s->image, snapshot_name);
- return r;
+ if (r < 0) {
+ error_setg_errno(errp, -r, "Failed to rollback snapshot '%s'",
+ snapshot_name);
+ }
}
static int qemu_rbd_snap_list(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 7e0610f..868ab89 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1867,7 +1867,9 @@ cleanup:
return ret;
}
-static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+static void sd_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BDRVSheepdogState *s = bs->opaque;
BDRVSheepdogState *old_s;
@@ -1892,13 +1894,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
if (ret) {
- error_report("Failed to find_vdi_name");
+ error_setg_errno(errp, -ret, "Failed to find VDI snapshot '%s'", vdi);
goto out;
}
fd = connect_to_sdog(s);
if (fd < 0) {
- ret = fd;
+ error_setg_errno(errp, -fd, "Failed to connect to sdog");
goto out;
}
@@ -1909,14 +1911,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
closesocket(fd);
if (ret) {
+ error_setg_errno(errp, -ret, "Failed to open VDI snapshot '%s'", vdi);
goto out;
}
memcpy(&s->inode, buf, sizeof(s->inode));
if (!s->inode.vm_state_size) {
- error_report("Invalid snapshot");
- ret = -ENOENT;
+ error_setg(errp, "Invalid snapshot '%s'", snapshot_id);
goto out;
}
@@ -1925,16 +1927,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
g_free(buf);
g_free(old_s);
- return 0;
+ return;
out:
/* recover bdrv_sd_state */
memcpy(s, old_s, sizeof(BDRVSheepdogState));
g_free(buf);
g_free(old_s);
-
- error_report("failed to open. recover old bdrv_sd_state.");
-
- return ret;
}
static void sd_snapshot_delete(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index abb636d..3dccecc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,8 +335,9 @@ int bdrv_is_snapshot(BlockDriverState *bs);
BlockDriverState *bdrv_snapshots(void);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id);
+void bdrv_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp);
void bdrv_snapshot_delete(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c3f67c3..db67841 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -154,8 +154,9 @@ struct BlockDriver {
int (*bdrv_snapshot_create)(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
- int (*bdrv_snapshot_goto)(BlockDriverState *bs,
- const char *snapshot_id);
+ void (*bdrv_snapshot_goto)(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp);
void (*bdrv_snapshot_delete)(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 9cf3467..4aca51f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2022,11 +2022,8 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_APPLY:
- ret = bdrv_snapshot_goto(bs, snapshot_name);
- if (ret) {
- error_report("Could not apply snapshot '%s': %d (%s)",
- snapshot_name, ret, strerror(-ret));
- }
+ bdrv_snapshot_goto(bs, snapshot_name, &local_err);
+ ret = qemu_img_handle_error(local_err);
break;
case SNAPSHOT_DELETE:
diff --git a/savevm.c b/savevm.c
index 1b4aea8..f280aae 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2529,11 +2529,11 @@ int load_vmstate(const char *name)
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs)) {
- ret = bdrv_snapshot_goto(bs, name);
- if (ret < 0) {
- error_report("Error %d while activating snapshot '%s' on '%s'",
- ret, name, bdrv_get_device_name(bs));
- return ret;
+ bdrv_snapshot_goto(bs, name, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ return -EIO;
}
}
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions Pavel Hrdina
@ 2013-04-25 17:06 ` Eric Blake
2013-05-03 11:03 ` Kevin Wolf
1 sibling, 0 replies; 44+ messages in thread
From: Eric Blake @ 2013-04-25 17:06 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 758 bytes --]
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 34 ++++++++++++++++++----------------
> block/qcow2-snapshot.c | 24 +++++++++++++++++-------
> block/qcow2.h | 4 +++-
> block/rbd.c | 10 +++++++---
> block/sheepdog.c | 18 ++++++++----------
> include/block/block.h | 5 +++--
> include/block/block_int.h | 5 +++--
> qemu-img.c | 7 ++-----
> savevm.c | 10 +++++-----
> 9 files changed, 66 insertions(+), 51 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions Pavel Hrdina
2013-04-25 17:06 ` Eric Blake
@ 2013-05-03 11:03 ` Kevin Wolf
1 sibling, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 11:03 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1867,7 +1867,9 @@ cleanup:
> return ret;
> }
>
> -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_goto(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> BDRVSheepdogState *s = bs->opaque;
> BDRVSheepdogState *old_s;
> @@ -1892,13 +1894,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>
> ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
> if (ret) {
> - error_report("Failed to find_vdi_name");
> + error_setg_errno(errp, -ret, "Failed to find VDI snapshot '%s'", vdi);
Isn't snapid what identifies the snapshot? Or maybe the combination of
vdi and snapid.
> goto out;
> }
>
> fd = connect_to_sdog(s);
> if (fd < 0) {
> - ret = fd;
> + error_setg_errno(errp, -fd, "Failed to connect to sdog");
> goto out;
> }
>
> @@ -1909,14 +1911,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> closesocket(fd);
>
> if (ret) {
> + error_setg_errno(errp, -ret, "Failed to open VDI snapshot '%s'", vdi);
> goto out;
> }
>
> memcpy(&s->inode, buf, sizeof(s->inode));
>
> if (!s->inode.vm_state_size) {
> - error_report("Invalid snapshot");
> - ret = -ENOENT;
> + error_setg(errp, "Invalid snapshot '%s'", snapshot_id);
Here as well.
> goto out;
> }
>
> @@ -1925,16 +1927,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> g_free(buf);
> g_free(old_s);
>
> - return 0;
> + return;
> out:
> /* recover bdrv_sd_state */
> memcpy(s, old_s, sizeof(BDRVSheepdogState));
> g_free(buf);
> g_free(old_s);
> -
> - error_report("failed to open. recover old bdrv_sd_state.");
> -
> - return ret;
> }
>
> static void sd_snapshot_delete(BlockDriverState *bs,
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2529,11 +2529,11 @@ int load_vmstate(const char *name)
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs)) {
> - ret = bdrv_snapshot_goto(bs, name);
> - if (ret < 0) {
> - error_report("Error %d while activating snapshot '%s' on '%s'",
> - ret, name, bdrv_get_device_name(bs));
> - return ret;
> + bdrv_snapshot_goto(bs, name, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report_err(local_err);
Shouldn't we add the device name on which the failure occurred?
> + error_free(local_err);
> + return -EIO;
> }
> }
> }
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 06/12] block: update error reporting for bdrv_snapshot_list() and related functions
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (4 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-04-25 18:55 ` Eric Blake
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 07/12] savevm: update error reporting for qemu_loadvm_state() Pavel Hrdina
` (7 subsequent siblings)
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Now the bdrv_snapshot_list function returns only number of snapshots. In case
that there is any error, the proper error message is set and return value is 0.
The return value is no longer for testing for errors because there should be only
one error reporting.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 22 ++++++++++++++--------
block/qcow2-snapshot.c | 4 +++-
block/qcow2.h | 4 +++-
block/rbd.c | 10 ++++++++--
block/sheepdog.c | 20 +++++++++++---------
include/block/block.h | 3 ++-
include/block/block_int.h | 3 ++-
qemu-img.c | 11 ++++++++---
savevm.c | 15 +++++++++------
9 files changed, 60 insertions(+), 32 deletions(-)
diff --git a/block.c b/block.c
index c36d3d5..ff4d619 100644
--- a/block.c
+++ b/block.c
@@ -3458,16 +3458,22 @@ void bdrv_snapshot_delete(BlockDriverState *bs,
}
int bdrv_snapshot_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info)
+ QEMUSnapshotInfo **psn_info,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_list)
- return drv->bdrv_snapshot_list(bs, psn_info);
- if (bs->file)
- return bdrv_snapshot_list(bs->file, psn_info);
- return -ENOTSUP;
+
+ if (!drv) {
+ error_setg(errp, "Device has no medium");
+ return 0;
+ } else if (drv->bdrv_snapshot_list) {
+ return drv->bdrv_snapshot_list(bs, psn_info, errp);
+ } else if (bs->file) {
+ return bdrv_snapshot_list(bs->file, psn_info, errp);
+ } else {
+ error_setg(errp, "Snapshots are not supported");
+ return 0;
+ }
}
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 67a57fc..f15ca52 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -603,7 +603,9 @@ void qcow2_snapshot_delete(BlockDriverState *bs,
#endif
}
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
+int qcow2_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_tab,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QEMUSnapshotInfo *sn_tab, *sn_info;
diff --git a/block/qcow2.h b/block/qcow2.h
index b70387c..ae62953 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -389,7 +389,9 @@ void qcow2_snapshot_goto(BlockDriverState *bs,
void qcow2_snapshot_delete(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
+int qcow2_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_tab,
+ Error **errp);
int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
void qcow2_free_snapshots(BlockDriverState *bs);
diff --git a/block/rbd.c b/block/rbd.c
index 5047b59..11a95e3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -928,7 +928,8 @@ static void qemu_rbd_snap_rollback(BlockDriverState *bs,
}
static int qemu_rbd_snap_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_tab)
+ QEMUSnapshotInfo **psn_tab,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
QEMUSnapshotInfo *sn_info, *sn_tab = NULL;
@@ -944,7 +945,12 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
}
} while (snap_count == -ERANGE);
- if (snap_count <= 0) {
+ if (snap_count < 0) {
+ error_setg_errno(errp, -snap_count, "Failed to find snapshots");
+ snap_count = 0;
+ }
+
+ if (snap_count == 0) {
goto done;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 868ab89..bd1bf8f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1943,7 +1943,9 @@ static void sd_snapshot_delete(BlockDriverState *bs,
error_setg(errp, "Deleting snapshot is not supported");
}
-static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
+static int sd_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_tab,
+ Error **errp)
{
BDRVSheepdogState *s = bs->opaque;
SheepdogReq req;
@@ -1961,7 +1963,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
fd = connect_to_sdog(s);
if (fd < 0) {
- ret = fd;
+ error_setg_errno(errp, -fd, "Failed to connect to sdog");
goto out;
}
@@ -1977,6 +1979,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
closesocket(fd);
if (ret) {
+ error_setg_errno(errp, -ret, "Failed to read VDIs");
goto out;
}
@@ -1988,7 +1991,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
fd = connect_to_sdog(s);
if (fd < 0) {
- ret = fd;
+ error_setg_errno(errp, -fd, "Failed to connect to sdog");
goto out;
}
@@ -2022,15 +2025,14 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
}
closesocket(fd);
-out:
- *psn_tab = sn_tab;
-
- g_free(vdi_inuse);
-
if (ret < 0) {
- return ret;
+ error_setg_errno(errp, -ret, "Failed to read VDI object");
+ return 0;
}
+out:
+ *psn_tab = sn_tab;
+ g_free(vdi_inuse);
return found;
}
diff --git a/include/block/block.h b/include/block/block.h
index 3dccecc..b100937 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -342,7 +342,8 @@ void bdrv_snapshot_delete(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
int bdrv_snapshot_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info);
+ QEMUSnapshotInfo **psn_info,
+ Error **errp);
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
const char *snapshot_name);
char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index db67841..72f52ea 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -161,7 +161,8 @@ struct BlockDriver {
const char *snapshot_id,
Error **errp);
int (*bdrv_snapshot_list)(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info);
+ QEMUSnapshotInfo **psn_info,
+ Error **errp);
int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
const char *snapshot_name);
int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
diff --git a/qemu-img.c b/qemu-img.c
index 4aca51f..db9fe2a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1559,10 +1559,12 @@ static void dump_snapshots(BlockDriverState *bs)
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i;
char buf[256];
+ Error *local_err = NULL;
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- if (nb_sns <= 0)
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+ if (qemu_img_handle_error(local_err) || nb_sns == 0) {
return;
+ }
printf("Snapshot list:\n");
printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
for(i = 0; i < nb_sns; i++) {
@@ -1594,7 +1596,10 @@ static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
int i, sn_count;
QEMUSnapshotInfo *sn_tab = NULL;
SnapshotInfoList *info_list, *cur_item = NULL;
- sn_count = bdrv_snapshot_list(bs, &sn_tab);
+ Error *local_err = NULL;
+ sn_count = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+
+ qemu_img_handle_error(local_err);
for (i = 0; i < sn_count; i++) {
info->has_snapshots = true;
diff --git a/savevm.c b/savevm.c
index f280aae..25fc7cc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2268,14 +2268,15 @@ static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
bool old_match)
{
QEMUSnapshotInfo *sn_tab, *sn;
+ Error *local_err = NULL;
int nb_sns, i;
bool found = false;
assert(name || id);
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- if (nb_sns < 0) {
- error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
return found;
}
@@ -2634,6 +2635,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
int total;
int *available_snapshots;
char buf[256];
+ Error *local_err = NULL;
bs = bdrv_snapshots();
if (!bs) {
@@ -2641,9 +2643,10 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
return;
}
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- if (nb_sns < 0) {
- monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
return;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] block: update error reporting for bdrv_snapshot_list() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 06/12] block: update error reporting for bdrv_snapshot_list() " Pavel Hrdina
@ 2013-04-25 18:55 ` Eric Blake
0 siblings, 0 replies; 44+ messages in thread
From: Eric Blake @ 2013-04-25 18:55 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> Now the bdrv_snapshot_list function returns only number of snapshots. In case
> that there is any error, the proper error message is set and return value is 0.
> The return value is no longer for testing for errors because there should be only
> one error reporting.
Either a caller is just getting the list and doesn't care about errors
(pass in NULL, 0 return is fine), or the caller wants to distinguish
between errors and an empty list (pass in error object; check it before
relying on the return value). Seems usable.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 22 ++++++++++++++--------
> block/qcow2-snapshot.c | 4 +++-
> block/qcow2.h | 4 +++-
> block/rbd.c | 10 ++++++++--
> block/sheepdog.c | 20 +++++++++++---------
> include/block/block.h | 3 ++-
> include/block/block_int.h | 3 ++-
> qemu-img.c | 11 ++++++++---
> savevm.c | 15 +++++++++------
> 9 files changed, 60 insertions(+), 32 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 07/12] savevm: update error reporting for qemu_loadvm_state()
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (5 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 06/12] block: update error reporting for bdrv_snapshot_list() " Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-05-03 11:17 ` Kevin Wolf
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm Pavel Hrdina
` (6 subsequent siblings)
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
include/sysemu/sysemu.h | 2 +-
migration.c | 11 ++++++---
savevm.c | 65 ++++++++++++++++++++++++-------------------------
3 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index f46f9d2..70c6927 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -78,7 +78,7 @@ int qemu_savevm_state_iterate(QEMUFile *f);
void qemu_savevm_state_complete(QEMUFile *f);
void qemu_savevm_state_cancel(void);
uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
-int qemu_loadvm_state(QEMUFile *f);
+void qemu_loadvm_state(QEMUFile *f, Error **errp);
/* SLIRP */
void do_info_slirp(Monitor *mon);
diff --git a/migration.c b/migration.c
index 3eb0fad..0279911 100644
--- a/migration.c
+++ b/migration.c
@@ -93,12 +93,15 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
static void process_incoming_migration_co(void *opaque)
{
QEMUFile *f = opaque;
- int ret;
+ Error *local_err = NULL;
- ret = qemu_loadvm_state(f);
+ qemu_loadvm_state(f, &local_err);
qemu_fclose(f);
- if (ret < 0) {
- fprintf(stderr, "load of migration failed\n");
+ if (error_is_set(&local_err)) {
+ qerror_report(ERROR_CLASS_GENERIC_ERROR,
+ "Incoming migration failed: %s",
+ error_get_pretty(local_err));
+ error_free(local_err);
exit(EXIT_FAILURE);
}
qemu_announce_self();
diff --git a/savevm.c b/savevm.c
index 25fc7cc..54eb61c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2144,7 +2144,7 @@ typedef struct LoadStateEntry {
int version_id;
} LoadStateEntry;
-int qemu_loadvm_state(QEMUFile *f)
+void qemu_loadvm_state(QEMUFile *f, Error **errp)
{
QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
QLIST_HEAD_INITIALIZER(loadvm_handlers);
@@ -2153,21 +2153,26 @@ int qemu_loadvm_state(QEMUFile *f)
unsigned int v;
int ret;
- if (qemu_savevm_state_blocked(NULL)) {
- return -EINVAL;
+ if (qemu_savevm_state_blocked(errp)) {
+ return;
}
v = qemu_get_be32(f);
- if (v != QEMU_VM_FILE_MAGIC)
- return -EINVAL;
+ if (v != QEMU_VM_FILE_MAGIC) {
+ error_setg(errp, "Unknown vmstate file magic");
+ return;
+ }
v = qemu_get_be32(f);
if (v == QEMU_VM_FILE_VERSION_COMPAT) {
- fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
- return -ENOTSUP;
+ error_setg(errp, "vmstate v2 format is obsolete and doesn't work "
+ "anymore");
+ return;
+ }
+ if (v != QEMU_VM_FILE_VERSION) {
+ error_setg(errp, "Unknown vmstate file version");
+ return;
}
- if (v != QEMU_VM_FILE_VERSION)
- return -ENOTSUP;
while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
uint32_t instance_id, version_id, section_id;
@@ -2189,16 +2194,15 @@ int qemu_loadvm_state(QEMUFile *f)
/* Find savevm section */
se = find_se(idstr, instance_id);
if (se == NULL) {
- fprintf(stderr, "Unknown savevm section or instance '%s' %d\n", idstr, instance_id);
- ret = -EINVAL;
+ error_setg(errp, "Unknown vmstate section or instance '%s' %d",
+ idstr, instance_id);
goto out;
}
/* Validate version */
if (version_id > se->version_id) {
- fprintf(stderr, "savevm: unsupported version %d for '%s' v%d\n",
- version_id, idstr, se->version_id);
- ret = -EINVAL;
+ error_setg(errp, "Unsupported version %d for '%s' v%d",
+ version_id, idstr, se->version_id);
goto out;
}
@@ -2212,8 +2216,8 @@ int qemu_loadvm_state(QEMUFile *f)
ret = vmstate_load(f, le->se, le->version_id);
if (ret < 0) {
- fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
- instance_id, idstr);
+ error_setg(errp, "Error while loading state for instance "
+ "0x%x of device '%s'", instance_id, idstr);
goto out;
}
break;
@@ -2227,40 +2231,35 @@ int qemu_loadvm_state(QEMUFile *f)
}
}
if (le == NULL) {
- fprintf(stderr, "Unknown savevm section %d\n", section_id);
- ret = -EINVAL;
+ error_setg(errp, "Unknown vmstate section %d", section_id);
goto out;
}
ret = vmstate_load(f, le->se, le->version_id);
if (ret < 0) {
- fprintf(stderr, "qemu: warning: error while loading state section id %d\n",
+ error_setg(errp, "Error while loading state section id %d",
section_id);
goto out;
}
break;
default:
- fprintf(stderr, "Unknown savevm section type %d\n", section_type);
- ret = -EINVAL;
+ error_setg(errp, "Unknown vmstate section type %d", section_type);
goto out;
}
}
cpu_synchronize_all_post_init();
- ret = 0;
+ ret = qemu_file_get_error(f);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to load vmstate");
+ }
out:
QLIST_FOREACH_SAFE(le, &loadvm_handlers, entry, new_le) {
QLIST_REMOVE(le, entry);
g_free(le);
}
-
- if (ret == 0) {
- ret = qemu_file_get_error(f);
- }
-
- return ret;
}
static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
@@ -2482,7 +2481,6 @@ int load_vmstate(const char *name)
QEMUSnapshotInfo sn;
QEMUFile *f;
Error *local_err = NULL;
- int ret;
bs_vm_state = bdrv_snapshots();
if (!bs_vm_state) {
@@ -2547,12 +2545,13 @@ int load_vmstate(const char *name)
}
qemu_system_reset(VMRESET_SILENT);
- ret = qemu_loadvm_state(f);
+ qemu_loadvm_state(f, &local_err);
qemu_fclose(f);
- if (ret < 0) {
- error_report("Error %d while loading VM state", ret);
- return ret;
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ return -1;
}
return 0;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/12] savevm: update error reporting for qemu_loadvm_state()
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 07/12] savevm: update error reporting for qemu_loadvm_state() Pavel Hrdina
@ 2013-05-03 11:17 ` Kevin Wolf
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 11:17 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> include/sysemu/sysemu.h | 2 +-
> migration.c | 11 ++++++---
> savevm.c | 65 ++++++++++++++++++++++++-------------------------
> 3 files changed, 40 insertions(+), 38 deletions(-)
> @@ -2212,8 +2216,8 @@ int qemu_loadvm_state(QEMUFile *f)
>
> ret = vmstate_load(f, le->se, le->version_id);
> if (ret < 0) {
> - fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
> - instance_id, idstr);
> + error_setg(errp, "Error while loading state for instance "
> + "0x%x of device '%s'", instance_id, idstr);
error_setg_errno?
> goto out;
> }
> break;
> @@ -2227,40 +2231,35 @@ int qemu_loadvm_state(QEMUFile *f)
> }
> }
> if (le == NULL) {
> - fprintf(stderr, "Unknown savevm section %d\n", section_id);
> - ret = -EINVAL;
> + error_setg(errp, "Unknown vmstate section %d", section_id);
> goto out;
> }
>
> ret = vmstate_load(f, le->se, le->version_id);
> if (ret < 0) {
> - fprintf(stderr, "qemu: warning: error while loading state section id %d\n",
> + error_setg(errp, "Error while loading state section id %d",
> section_id);
Here, too.
> goto out;
> }
> break;
> default:
> - fprintf(stderr, "Unknown savevm section type %d\n", section_type);
> - ret = -EINVAL;
> + error_setg(errp, "Unknown vmstate section type %d", section_type);
> goto out;
> }
> }
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (6 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 07/12] savevm: update error reporting for qemu_loadvm_state() Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-05-03 11:31 ` Kevin Wolf
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 09/12] block: update error reporting for bdrv_snapshot_create() and related functions Pavel Hrdina
` (5 subsequent siblings)
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
QMP command vm-snapshot-load and HMP command loadvm behave similar to
vm-snapshot-delete and delvm. The only different is that they will load
the snapshot instead of deleting it.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 16 +++++----
hmp.c | 33 ++++++++++++++++++
hmp.h | 1 +
include/sysemu/sysemu.h | 1 -
monitor.c | 12 -------
qapi-schema.json | 18 ++++++++++
qmp-commands.hx | 38 ++++++++++++++++++++
savevm.c | 93 +++++++++++++++++++++++++++++++++++--------------
vl.c | 7 +++-
9 files changed, 172 insertions(+), 47 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 32f79d9..7defb31 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -324,17 +324,19 @@ ETEXI
{
.name = "loadvm",
- .args_type = "name:s",
- .params = "tag|id",
- .help = "restore a VM snapshot from its tag or id",
- .mhandler.cmd = do_loadvm,
+ .args_type = "id:-i,name:s",
+ .params = "[-i] name",
+ .help = "restore a VM snapshot by name as tag or with -i flag by name as id",
+ .mhandler.cmd = hmp_vm_snapshot_load,
},
STEXI
-@item loadvm @var{tag}|@var{id}
+@item loadvm [-i] @var{name}
@findex loadvm
-Set the whole virtual machine to the snapshot identified by the tag
-@var{tag} or the unique snapshot ID @var{id}.
+Set the whole virtual machine to the snapshot identified by @var{name} as tag.
+If flag -i is provided, snapshot identified by @var{name} as id will be loaded.
+It must be a snapshot of a whole VM.
+
ETEXI
{
diff --git a/hmp.c b/hmp.c
index fea1cee..69a750c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1458,3 +1458,36 @@ void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict)
qapi_free_SnapshotInfo(info);
hmp_handle_error(mon, &local_err);
}
+
+void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
+{
+ const char *name = qdict_get_try_str(qdict, "name");
+ const bool id = qdict_get_try_bool(qdict, "id", false);
+ Error *local_err = NULL;
+ SnapshotInfo *info = NULL;
+
+ if (id) {
+ info = qmp_vm_snapshot_load(false, NULL, true, name, &local_err);
+ } else {
+ info = qmp_vm_snapshot_load(true, name, false, NULL, &local_err);
+ }
+
+ if (info) {
+ char buf[256];
+ QEMUSnapshotInfo sn = {
+ .vm_state_size = info->vm_state_size,
+ .date_sec = info->date_sec,
+ .date_nsec = info->date_nsec,
+ .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
+ info->vm_clock_nsec,
+ };
+ pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
+ pstrcpy(sn.name, sizeof(sn.name), info->name);
+ monitor_printf(mon, "Loaded snapshot's info:\n");
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ }
+
+ qapi_free_SnapshotInfo(info);
+ hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index b0667b3..a65cbf2 100644
--- a/hmp.h
+++ b/hmp.h
@@ -86,5 +86,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
void hmp_chardev_add(Monitor *mon, const QDict *qdict);
void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict);
#endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 70c6927..913f49f 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -66,7 +66,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
void qemu_add_machine_init_done_notifier(Notifier *notify);
void do_savevm(Monitor *mon, const QDict *qdict);
-int load_vmstate(const char *name);
void do_info_snapshots(Monitor *mon, const QDict *qdict);
void qemu_announce_self(void);
diff --git a/monitor.c b/monitor.c
index 332abe7..02c20ef 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2093,18 +2093,6 @@ void qmp_closefd(const char *fdname, Error **errp)
error_set(errp, QERR_FD_NOT_FOUND, fdname);
}
-static void do_loadvm(Monitor *mon, const QDict *qdict)
-{
- int saved_vm_running = runstate_is_running();
- const char *name = qdict_get_str(qdict, "name");
-
- vm_stop(RUN_STATE_RESTORE_VM);
-
- if (load_vmstate(name) == 0 && saved_vm_running) {
- vm_start();
- }
-}
-
int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
{
mon_fd_t *monfd;
diff --git a/qapi-schema.json b/qapi-schema.json
index 0bd61b8..c15adc4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3522,3 +3522,21 @@
##
{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
'returns': 'SnapshotInfo' }
+
+##
+# @vm-snapshot-load:
+#
+# Set the whole virtual machine to the snapshot identified by the name
+# or the unique snapshot id or both. It must be a snapshot of a whole VM and
+# at least one of the name or id parameter must be specified.
+#
+# @name: #optional tag of existing snapshot
+#
+# @id: #optional id of existing snapshot
+#
+# Returns: SnapshotInfo on success
+#
+# Since: 1.5
+##
+{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'},
+ 'returns': 'SnapshotInfo' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9b15cb4..445bd74 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1461,6 +1461,44 @@ Example:
EQMP
{
+ .name = "vm-snapshot-load",
+ .args_type = "name:s?,id:s?",
+ .params = "[name] [id]",
+ .help = "restore a VM snapshot from its tag or id",
+ .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_load
+ },
+
+SQMP
+vm-snapshot-load
+------
+
+Set the whole virtual machine to the snapshot identified by the tag
+or the unique snapshot id or both. It must be a snapshot of a whole VM and
+at least one of the name or id parameter must be specified.
+
+Arguments:
+
+- "name": tag of existing snapshot (json-string, optional)
+
+- "id": id of existing snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "vm-snapshot-load", "arguments": { "name": "my_snapshot" } }
+<- {
+ "return": {
+ "id": "1",
+ "name": "my_snapshot",
+ "date-sec": 1364480534,
+ "date-nsec": 978215000,
+ "vm-clock-sec": 5,
+ "vm-clock-nsec": 153620449,
+ "vm-state-size": 5709953
+ }
+ }
+
+EQMP
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/savevm.c b/savevm.c
index 54eb61c..01cedf0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2475,28 +2475,46 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
vm_start();
}
-int load_vmstate(const char *name)
+SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
+ bool has_id, const char *id, Error **errp)
{
BlockDriverState *bs, *bs_vm_state;
QEMUSnapshotInfo sn;
QEMUFile *f;
Error *local_err = NULL;
+ SnapshotInfo *info = NULL;
+ bool saved_vm_running;
+
+ if (!has_name && !has_id) {
+ error_setg(errp, "Name or id must be specified");
+ return NULL;
+ }
+
+ if (!has_name) {
+ name = NULL;
+ }
+ if (!has_id) {
+ id = NULL;
+ }
bs_vm_state = bdrv_snapshots();
if (!bs_vm_state) {
- error_report("No block device supports snapshots");
- return -ENOTSUP;
+ error_setg(errp, "No block device supports snapshots");
+ return NULL;
}
/* Don't even try to load empty VM states */
- if (!bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, &local_err, true)) {
- error_report("Snapshot doesn't exist: %s", error_get_pretty(local_err));
+ if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, &local_err, false)) {
+ error_setg(errp, "Snapshot doesn't exist: %s",
+ error_get_pretty(local_err));
error_free(local_err);
- return -ENOENT;
- } else if (sn.vm_state_size == 0) {
- error_report("This is a disk-only snapshot. Revert to it offline "
- "using qemu-img.");
- return -EINVAL;
+ return NULL;
+ }
+
+ if (sn.vm_state_size == 0) {
+ error_setg(errp, "This is a disk-only snapshot, revert to it offline "
+ "using qemu-img");
+ return NULL;
}
/* Verify if there is any device that doesn't support snapshots and is
@@ -2509,30 +2527,41 @@ int load_vmstate(const char *name)
}
if (!bdrv_can_snapshot(bs)) {
- error_report("Device '%s' is writable but does not support snapshots.",
- bdrv_get_device_name(bs));
- return -ENOTSUP;
+ error_setg(errp, "Device '%s' is writable but does not support "
+ "snapshots", bdrv_get_device_name(bs));
+ return NULL;
}
- if (!bdrv_snapshot_find(bs, &sn, name, NULL, &local_err, true)) {
- error_report("Snapshot doesn't exist for device '%s': %s",
- bdrv_get_device_name(bs), error_get_pretty(local_err));
+ if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
+ error_setg(errp, "Snapshot doesn't exist for device '%s': %s",
+ bdrv_get_device_name(bs), error_get_pretty(local_err));
error_free(local_err);
- return -ENOENT;
+ return NULL;
}
}
+ saved_vm_running = runstate_is_running();
+ vm_stop(RUN_STATE_RESTORE_VM);
+
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all();
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs)) {
- bdrv_snapshot_goto(bs, name, &local_err);
+ /* Small hack to ensure that proper snapshot is deleted for every
+ * block driver. This will be fixed soon. */
+ if (!strcmp(bs->drv->format_name, "rbd")) {
+ bdrv_snapshot_goto(bs, sn.name, &local_err);
+ } else {
+ bdrv_snapshot_goto(bs, sn.id_str, &local_err);
+ }
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
+ error_setg(errp, "Failed to load snapshot for device '%s': %s",
+ bdrv_get_device_name(bs),
+ error_get_pretty(local_err));
error_free(local_err);
- return -EIO;
+ return NULL;
}
}
}
@@ -2540,8 +2569,8 @@ int load_vmstate(const char *name)
/* restore the VM state */
f = qemu_fopen_bdrv(bs_vm_state, 0);
if (!f) {
- error_report("Could not open VM state file");
- return -EINVAL;
+ error_setg(errp, "Could not open VM state file");
+ return NULL;
}
qemu_system_reset(VMRESET_SILENT);
@@ -2549,12 +2578,24 @@ int load_vmstate(const char *name)
qemu_fclose(f);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
- return -1;
+ error_propagate(errp, local_err);
+ return NULL;
}
- return 0;
+ if (saved_vm_running) {
+ vm_start();
+ }
+
+ 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;
}
SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
diff --git a/vl.c b/vl.c
index 2e0d1a7..f30dd67 100644
--- a/vl.c
+++ b/vl.c
@@ -4376,8 +4376,13 @@ int main(int argc, char **argv, char **envp)
qemu_system_reset(VMRESET_SILENT);
if (loadvm) {
- if (load_vmstate(loadvm) < 0) {
+ Error *local_err = NULL;
+ qmp_vm_snapshot_load(true, loadvm, false, NULL, &local_err);
+
+ if (error_is_set(&local_err)) {
autostart = 0;
+ qerror_report_err(local_err);
+ error_free(local_err);
}
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm Pavel Hrdina
@ 2013-05-03 11:31 ` Kevin Wolf
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 11:31 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-load and HMP command loadvm behave similar to
> vm-snapshot-delete and delvm. The only different is that they will load
> the snapshot instead of deleting it.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs)) {
> - bdrv_snapshot_goto(bs, name, &local_err);
> + /* Small hack to ensure that proper snapshot is deleted for every
> + * block driver. This will be fixed soon. */
> + if (!strcmp(bs->drv->format_name, "rbd")) {
> + bdrv_snapshot_goto(bs, sn.name, &local_err);
> + } else {
> + bdrv_snapshot_goto(bs, sn.id_str, &local_err);
> + }
I think I wanted to comment this in an earlier patch in this series, but
didn't actually do it, so here is what I intended to write there:
"Umm... Just no."
Special casing an image format should _never_ happen. Even more so
outside block.c (and it's disliked even there). It's a sign that we're
doing something seriously wrong.
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> + error_setg(errp, "Failed to load snapshot for device '%s': %s",
> + bdrv_get_device_name(bs),
> + error_get_pretty(local_err));
> error_free(local_err);
> - return -EIO;
> + return NULL;
> }
> }
> }
> --- a/vl.c
> +++ b/vl.c
> @@ -4376,8 +4376,13 @@ int main(int argc, char **argv, char **envp)
>
> qemu_system_reset(VMRESET_SILENT);
> if (loadvm) {
> - if (load_vmstate(loadvm) < 0) {
> + Error *local_err = NULL;
> + qmp_vm_snapshot_load(true, loadvm, false, NULL, &local_err);
> +
> + if (error_is_set(&local_err)) {
> autostart = 0;
> + qerror_report_err(local_err);
> + error_free(local_err);
> }
> }
Should we add something like "Loading the snapshot failed"? It's
probably not clear from all possible error messages.
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 09/12] block: update error reporting for bdrv_snapshot_create() and related functions
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (7 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 10/12] savevm: update error reporting of qemu_savevm_state() " Pavel Hrdina
` (4 subsequent siblings)
13 siblings, 0 replies; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 22 +++++++++++++---------
block/qcow2-snapshot.c | 16 ++++++++++------
block/qcow2.h | 4 +++-
block/rbd.c | 23 +++++++----------------
block/sheepdog.c | 22 +++++++++++-----------
include/block/block.h | 5 +++--
include/block/block_int.h | 5 +++--
qemu-img.c | 7 ++-----
savevm.c | 12 ++++++++----
9 files changed, 60 insertions(+), 56 deletions(-)
diff --git a/block.c b/block.c
index ff4d619..a4cbc51 100644
--- a/block.c
+++ b/block.c
@@ -3399,17 +3399,21 @@ BlockDriverState *bdrv_snapshots(void)
return NULL;
}
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
+void bdrv_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_create)
- return drv->bdrv_snapshot_create(bs, sn_info);
- if (bs->file)
- return bdrv_snapshot_create(bs->file, sn_info);
- return -ENOTSUP;
+
+ if (!drv) {
+ error_setg(errp, "Device has no medium");
+ } else if (drv->bdrv_snapshot_create) {
+ drv->bdrv_snapshot_create(bs, sn_info, errp);
+ } else if (bs->file) {
+ bdrv_snapshot_create(bs->file, sn_info, errp);
+ } else {
+ error_setg(errp, "Snapshots are not supported");
+ }
}
void bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index f15ca52..17368c0 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -315,7 +315,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
}
/* if no id is provided, a new one is constructed */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+void qcow2_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot *new_snapshot_list = NULL;
@@ -334,7 +336,8 @@ 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) {
- return -EEXIST;
+ error_setg(errp, "Parameter 'id' has to be unique ID");
+ return;
}
/* Populate sn with passed data */
@@ -350,7 +353,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
/* Allocate the L1 table of the snapshot and copy the current one there. */
l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
if (l1_table_offset < 0) {
- ret = l1_table_offset;
+ error_setg_errno(errp, -l1_table_offset, "Failed to allocate L1 table");
goto fail;
}
@@ -365,6 +368,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
s->l1_size * sizeof(uint64_t));
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to save L1 table");
goto fail;
}
@@ -378,6 +382,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
*/
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to update refcounts");
goto fail;
}
@@ -395,6 +400,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
if (ret < 0) {
g_free(s->snapshots);
s->snapshots = old_snapshot_list;
+ error_setg_errno(errp, -ret, "Failed to write new snapshot");
goto fail;
}
@@ -406,14 +412,12 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
qcow2_check_refcounts(bs, &result, 0);
}
#endif
- return 0;
+ return;
fail:
g_free(sn->id_str);
g_free(sn->name);
g_free(l1_table);
-
- return ret;
}
/* copy the snapshot 'snapshot_name' into the current disk image */
diff --git a/block/qcow2.h b/block/qcow2.h
index ae62953..c194cff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -382,7 +382,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
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);
+void qcow2_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
void qcow2_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
diff --git a/block/rbd.c b/block/rbd.c
index 11a95e3..5566f3d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -867,36 +867,27 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
return 0;
}
-static int qemu_rbd_snap_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
+static void qemu_rbd_snap_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
if (sn_info->name[0] == '\0') {
- return -EINVAL; /* we need a name for rbd snapshots */
+ error_setg(errp, "Parameter 'name' cannot be empty");
+ return;
}
/*
* rbd snapshots are using the name as the user controlled unique identifier
- * we can't use the rbd snapid for that purpose, as it can't be set
+ * we are not using the id for that purpose, as it can't be set
*/
- if (sn_info->id_str[0] != '\0' &&
- strcmp(sn_info->id_str, sn_info->name) != 0) {
- return -EINVAL;
- }
-
- if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
- return -ERANGE;
- }
r = rbd_snap_create(s->image, sn_info->name);
if (r < 0) {
- error_report("failed to create snap: %s", strerror(-r));
- return r;
+ error_setg_errno(errp, -r, "Failed to create snapshot");
}
-
- return 0;
}
static void qemu_rbd_snap_remove(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index bd1bf8f..ba1e335 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1796,7 +1796,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
return acb->ret;
}
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static void sd_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BDRVSheepdogState *s = bs->opaque;
int ret, fd;
@@ -1809,10 +1811,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
s->name, sn_info->vm_state_size, s->is_snapshot);
if (s->is_snapshot) {
- error_report("You can't create a snapshot of a snapshot VDI, "
- "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
- return -EINVAL;
+ error_setg(errp, "You can't create a snapshot '%s' of a snapshot "
+ "VDI %s (%" PRIu32 ")", sn_info->name, s->name,
+ s->inode.vdi_id);
+ return;
}
dprintf("%s %s\n", sn_info->name, sn_info->id_str);
@@ -1829,22 +1831,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
/* refresh inode. */
fd = connect_to_sdog(s);
if (fd < 0) {
- ret = fd;
+ error_setg_errno(errp, -fd, "Failed to connect to sdog");
goto cleanup;
}
ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
s->inode.nr_copies, datalen, 0, false, s->cache_flags);
if (ret < 0) {
- error_report("failed to write snapshot's inode.");
+ error_setg_errno(errp, -ret, "Failed to write snapshot's inode");
goto cleanup;
}
ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,
1);
if (ret < 0) {
- error_report("failed to create inode for snapshot. %s",
- strerror(errno));
+ error_setg_errno(errp, -ret, "Failed to create inode for snapshot");
goto cleanup;
}
@@ -1854,7 +1855,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
s->inode.nr_copies, datalen, 0, s->cache_flags);
if (ret < 0) {
- error_report("failed to read new inode info. %s", strerror(errno));
+ error_setg_errno(errp, -ret, "Failed to read new inode info");
goto cleanup;
}
@@ -1864,7 +1865,6 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
cleanup:
closesocket(fd);
- return ret;
}
static void sd_snapshot_goto(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index b100937..10a11c6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -333,8 +333,9 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_is_snapshot(BlockDriverState *bs);
BlockDriverState *bdrv_snapshots(void);
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
+void bdrv_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
void bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 72f52ea..8d948df 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -152,8 +152,9 @@ struct BlockDriver {
int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors);
- int (*bdrv_snapshot_create)(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
+ void (*bdrv_snapshot_create)(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
void (*bdrv_snapshot_goto)(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index db9fe2a..846926b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2019,11 +2019,8 @@ static int img_snapshot(int argc, char **argv)
sn.date_sec = tv.tv_sec;
sn.date_nsec = tv.tv_usec * 1000;
- ret = bdrv_snapshot_create(bs, &sn);
- if (ret) {
- error_report("Could not create snapshot '%s': %d (%s)",
- snapshot_name, ret, strerror(-ret));
- }
+ bdrv_snapshot_create(bs, &sn, &local_err);
+ ret = qemu_img_handle_error(local_err);
break;
case SNAPSHOT_APPLY:
diff --git a/savevm.c b/savevm.c
index 01cedf0..6458621 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2365,6 +2365,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
qemu_timeval tv;
struct tm tm;
const char *name = qdict_get_try_str(qdict, "name");
+ Error *local_err = NULL;
/* Verify if there is a device that doesn't support snapshots and is writable */
bs = NULL;
@@ -2437,10 +2438,13 @@ void do_savevm(Monitor *mon, const QDict *qdict)
if (bdrv_can_snapshot(bs1)) {
/* Write VM state size only to the image that contains the state */
sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
- ret = bdrv_snapshot_create(bs1, sn);
- if (ret < 0) {
- monitor_printf(mon, "Error while creating snapshot on '%s'\n",
- bdrv_get_device_name(bs1));
+ bdrv_snapshot_create(bs1, sn, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report(ERROR_CLASS_GENERIC_ERROR,
+ "Failed to create snapshot for device '%s': %s",
+ bdrv_get_device_name(bs1),
+ error_get_pretty(local_err));
+ error_free(local_err);
}
}
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH v2 10/12] savevm: update error reporting of qemu_savevm_state() and related functions
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (8 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 09/12] block: update error reporting for bdrv_snapshot_create() and related functions Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-05-03 12:40 ` Kevin Wolf
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 11/12] qapi: Convert savevm Pavel Hrdina
` (3 subsequent siblings)
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
include/sysemu/sysemu.h | 7 ++++---
migration.c | 6 +++---
savevm.c | 38 +++++++++++++++++++-------------------
3 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 913f49f..0e3f30a 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -72,9 +72,10 @@ void qemu_announce_self(void);
bool qemu_savevm_state_blocked(Error **errp);
void qemu_savevm_state_begin(QEMUFile *f,
- const MigrationParams *params);
-int qemu_savevm_state_iterate(QEMUFile *f);
-void qemu_savevm_state_complete(QEMUFile *f);
+ const MigrationParams *params,
+ Error **errp);
+int qemu_savevm_state_iterate(QEMUFile *f, Error **errp);
+void qemu_savevm_state_complete(QEMUFile *f, Error **errp);
void qemu_savevm_state_cancel(void);
uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
void qemu_loadvm_state(QEMUFile *f, Error **errp);
diff --git a/migration.c b/migration.c
index 0279911..15ecd3f 100644
--- a/migration.c
+++ b/migration.c
@@ -508,7 +508,7 @@ static void *migration_thread(void *opaque)
bool old_vm_running = false;
DPRINTF("beginning savevm\n");
- qemu_savevm_state_begin(s->file, &s->params);
+ qemu_savevm_state_begin(s->file, &s->params, NULL);
while (s->state == MIG_STATE_ACTIVE) {
int64_t current_time;
@@ -519,7 +519,7 @@ static void *migration_thread(void *opaque)
pending_size = qemu_savevm_state_pending(s->file, max_size);
DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
if (pending_size && pending_size >= max_size) {
- qemu_savevm_state_iterate(s->file);
+ qemu_savevm_state_iterate(s->file, NULL);
} else {
DPRINTF("done iterating\n");
qemu_mutex_lock_iothread();
@@ -528,7 +528,7 @@ static void *migration_thread(void *opaque)
old_vm_running = runstate_is_running();
vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
qemu_file_set_rate_limit(s->file, INT_MAX);
- qemu_savevm_state_complete(s->file);
+ qemu_savevm_state_complete(s->file, NULL);
qemu_mutex_unlock_iothread();
if (!qemu_file_get_error(s->file)) {
migrate_finish_set_state(s, MIG_STATE_COMPLETED);
diff --git a/savevm.c b/savevm.c
index 6458621..749d49d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1795,7 +1795,8 @@ bool qemu_savevm_state_blocked(Error **errp)
}
void qemu_savevm_state_begin(QEMUFile *f,
- const MigrationParams *params)
+ const MigrationParams *params,
+ Error **errp)
{
SaveStateEntry *se;
int ret;
@@ -1836,6 +1837,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
ret = se->ops->save_live_setup(f, se->opaque);
if (ret < 0) {
qemu_file_set_error(f, ret);
+ error_setg_errno(errp, -ret, "Failed to begin vmstate save");
break;
}
}
@@ -1847,7 +1849,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
* 0 : We haven't finished, caller have to go again
* 1 : We have finished, we can go to complete phase
*/
-int qemu_savevm_state_iterate(QEMUFile *f)
+int qemu_savevm_state_iterate(QEMUFile *f, Error **errp)
{
SaveStateEntry *se;
int ret = 1;
@@ -1874,6 +1876,7 @@ int qemu_savevm_state_iterate(QEMUFile *f)
if (ret < 0) {
qemu_file_set_error(f, ret);
+ error_setg_errno(errp, -ret, "Failed to iterate vmstate save");
}
if (ret <= 0) {
/* Do not proceed to the next vmstate before this one reported
@@ -1886,7 +1889,7 @@ int qemu_savevm_state_iterate(QEMUFile *f)
return ret;
}
-void qemu_savevm_state_complete(QEMUFile *f)
+void qemu_savevm_state_complete(QEMUFile *f, Error **errp)
{
SaveStateEntry *se;
int ret;
@@ -1911,6 +1914,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
trace_savevm_section_end(se->section_id);
if (ret < 0) {
qemu_file_set_error(f, ret);
+ error_setg_errno(errp, -ret, "Failed to complete vmstate save");
return;
}
}
@@ -1972,37 +1976,33 @@ void qemu_savevm_state_cancel(void)
}
}
-static int qemu_savevm_state(QEMUFile *f)
+static void qemu_savevm_state(QEMUFile *f, Error **errp)
{
- int ret;
MigrationParams params = {
.blk = 0,
.shared = 0
};
- if (qemu_savevm_state_blocked(NULL)) {
- return -EINVAL;
+ if (qemu_savevm_state_blocked(errp)) {
+ return;
}
qemu_mutex_unlock_iothread();
- qemu_savevm_state_begin(f, ¶ms);
+ qemu_savevm_state_begin(f, ¶ms, errp);
qemu_mutex_lock_iothread();
while (qemu_file_get_error(f) == 0) {
- if (qemu_savevm_state_iterate(f) > 0) {
+ if (qemu_savevm_state_iterate(f, errp) > 0) {
break;
}
}
- ret = qemu_file_get_error(f);
- if (ret == 0) {
- qemu_savevm_state_complete(f);
- ret = qemu_file_get_error(f);
+ if (!qemu_file_get_error(f)) {
+ qemu_savevm_state_complete(f, errp);
}
- if (ret != 0) {
+ if (qemu_file_get_error(f)) {
qemu_savevm_state_cancel();
}
- return ret;
}
static int qemu_save_device_state(QEMUFile *f)
@@ -2358,7 +2358,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
- int ret;
QEMUFile *f;
int saved_vm_running;
uint64_t vm_state_size;
@@ -2423,11 +2422,12 @@ void do_savevm(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "Could not open VM state file\n");
goto the_end;
}
- ret = qemu_savevm_state(f);
+ qemu_savevm_state(f, &local_err);
vm_state_size = qemu_ftell(f);
qemu_fclose(f);
- if (ret < 0) {
- monitor_printf(mon, "Error %d while writing VM\n", ret);
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
goto the_end;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 10/12] savevm: update error reporting of qemu_savevm_state() and related functions
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 10/12] savevm: update error reporting of qemu_savevm_state() " Pavel Hrdina
@ 2013-05-03 12:40 ` Kevin Wolf
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 12:40 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> include/sysemu/sysemu.h | 7 ++++---
> migration.c | 6 +++---
> savevm.c | 38 +++++++++++++++++++-------------------
> 3 files changed, 26 insertions(+), 25 deletions(-)
> --- a/migration.c
> +++ b/migration.c
> @@ -508,7 +508,7 @@ static void *migration_thread(void *opaque)
> bool old_vm_running = false;
>
> DPRINTF("beginning savevm\n");
> - qemu_savevm_state_begin(s->file, &s->params);
> + qemu_savevm_state_begin(s->file, &s->params, NULL);
>
> while (s->state == MIG_STATE_ACTIVE) {
> int64_t current_time;
> @@ -519,7 +519,7 @@ static void *migration_thread(void *opaque)
> pending_size = qemu_savevm_state_pending(s->file, max_size);
> DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> if (pending_size && pending_size >= max_size) {
> - qemu_savevm_state_iterate(s->file);
> + qemu_savevm_state_iterate(s->file, NULL);
> } else {
> DPRINTF("done iterating\n");
> qemu_mutex_lock_iothread();
> @@ -528,7 +528,7 @@ static void *migration_thread(void *opaque)
> old_vm_running = runstate_is_running();
> vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> qemu_file_set_rate_limit(s->file, INT_MAX);
> - qemu_savevm_state_complete(s->file);
> + qemu_savevm_state_complete(s->file, NULL);
> qemu_mutex_unlock_iothread();
> if (!qemu_file_get_error(s->file)) {
> migrate_finish_set_state(s, MIG_STATE_COMPLETED);
Why is it okay to ignore any errors in this function?
> diff --git a/savevm.c b/savevm.c
> index 6458621..749d49d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1972,37 +1976,33 @@ void qemu_savevm_state_cancel(void)
> }
> }
>
> -static int qemu_savevm_state(QEMUFile *f)
> +static void qemu_savevm_state(QEMUFile *f, Error **errp)
> {
> - int ret;
> MigrationParams params = {
> .blk = 0,
> .shared = 0
> };
>
> - if (qemu_savevm_state_blocked(NULL)) {
> - return -EINVAL;
> + if (qemu_savevm_state_blocked(errp)) {
> + return;
> }
>
> qemu_mutex_unlock_iothread();
> - qemu_savevm_state_begin(f, ¶ms);
> + qemu_savevm_state_begin(f, ¶ms, errp);
> qemu_mutex_lock_iothread();
You need to check errp here.
> while (qemu_file_get_error(f) == 0) {
> - if (qemu_savevm_state_iterate(f) > 0) {
> + if (qemu_savevm_state_iterate(f, errp) > 0) {
> break;
> }
And here as well.
The problem is that you must not overwrite an error, therefore you need
to explicitly free and ignore unused errors, and decide which one takes
precedence.
> }
>
> - ret = qemu_file_get_error(f);
> - if (ret == 0) {
> - qemu_savevm_state_complete(f);
> - ret = qemu_file_get_error(f);
> + if (!qemu_file_get_error(f)) {
> + qemu_savevm_state_complete(f, errp);
> }
> - if (ret != 0) {
> + if (qemu_file_get_error(f)) {
> qemu_savevm_state_cancel();
> }
> - return ret;
> }
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 11/12] qapi: Convert savevm
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (9 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 10/12] savevm: update error reporting of qemu_savevm_state() " Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-05-03 12:52 ` Kevin Wolf
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 12/12] savevm: remove backward compatibility from bdrv_snapshot_find() Pavel Hrdina
` (2 subsequent siblings)
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
QMP command vm-snapshot-save takes one parameter name and the name is
mandatory. The command returns SnapshotInfo on success, otherwise it returns
an error message. If there is a snapshot with the same name it also returns
an error message and if you want to overwrite that snapshot, you will have to
at first call vm-snapshot-delete.
HMP command savevm now has one optional parameter name and one flag -f.
If the name parameter isn't provided the HMP command will create new one for
internally used vm-snapshot-save. You can also specify the -f flag for overwrite
existing snapshot which will internally use vm-snapshot-delete before
vm-snapshot-save, otherwise it will print an error message if there is already
a snapshot with the same name. If something else goes wrong, an error message
will be printed.
These improves behavior of the command to let you select only the name of the
snapshot you want to create. This will ensure that if you want snapshot with
the name '2', it will not rewrite or fail if there is any snapshot with id '2'.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 18 +++++-----
hmp.c | 49 +++++++++++++++++++++++++++
hmp.h | 1 +
include/sysemu/sysemu.h | 1 -
qapi-schema.json | 19 +++++++++++
qmp-commands.hx | 39 +++++++++++++++++++++
savevm.c | 90 +++++++++++++++++--------------------------------
7 files changed, 147 insertions(+), 70 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7defb31..0bd115d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -307,19 +307,19 @@ ETEXI
{
.name = "savevm",
- .args_type = "name:s?",
- .params = "[tag|id]",
- .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
- .mhandler.cmd = do_savevm,
+ .args_type = "force:-f,name:s?",
+ .params = "[-f] [name]",
+ .help = "save a VM snapshot, to replace existing snapshot use force flag",
+ .mhandler.cmd = hmp_vm_snapshot_save,
},
STEXI
-@item savevm [@var{tag}|@var{id}]
+@item savevm [@var{-f}] [@var{name}]
@findex savevm
-Create a snapshot of the whole virtual machine. If @var{tag} is
-provided, it is used as human readable identifier. If there is already
-a snapshot with the same tag or ID, it is replaced. More info at
-@ref{vm_snapshots}.
+Create a snapshot of the whole virtual machine. If @var{name} is provided, it
+is used as human readable identifier. If there is already a snapshot with the
+same @var{name}, @var{-f} flag needs to be specified.
+More info at @ref{vm_snapshots}.
ETEXI
{
diff --git a/hmp.c b/hmp.c
index 69a750c..084404c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1491,3 +1491,52 @@ void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
qapi_free_SnapshotInfo(info);
hmp_handle_error(mon, &local_err);
}
+
+void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
+{
+ const char *name = qdict_get_try_str(qdict, "name");
+ bool force = qdict_get_try_bool(qdict, "force", 0);
+ Error *local_err = NULL;
+ SnapshotInfo *info = NULL;
+ qemu_timeval tv;
+ struct tm tm;
+ char tmp_name[256];
+
+ if (!name) {
+ localtime_r((const time_t *)&tv.tv_sec, &tm);
+ strftime(tmp_name, sizeof(tmp_name), "vm-%Y%m%d%H%M%S", &tm);
+ name = tmp_name;
+ }
+
+ if (force) {
+ info = qmp_vm_snapshot_delete(true, name, false, NULL, &local_err);
+ /* We don't need print info about deleted snapshot. It still needs to
+ * be freed. */
+ qapi_free_SnapshotInfo(info);
+ if (error_is_set(&local_err)) {
+ hmp_handle_error(mon, &local_err);
+ return;
+ }
+ }
+
+ info = qmp_vm_snapshot_save(name, &local_err);
+
+ if (info) {
+ char buf[256];
+ QEMUSnapshotInfo sn = {
+ .vm_state_size = info->vm_state_size,
+ .date_sec = info->date_sec,
+ .date_nsec = info->date_nsec,
+ .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
+ info->vm_clock_nsec,
+ };
+ pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
+ pstrcpy(sn.name, sizeof(sn.name), info->name);
+ monitor_printf(mon, "Created snapshot's info:\n");
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ }
+
+ qapi_free_SnapshotInfo(info);
+ hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index a65cbf2..77e1715 100644
--- a/hmp.h
+++ b/hmp.h
@@ -87,5 +87,6 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict);
void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict);
void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
#endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 0e3f30a..25cd310 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
void qemu_add_machine_init_done_notifier(Notifier *notify);
-void do_savevm(Monitor *mon, const QDict *qdict);
void do_info_snapshots(Monitor *mon, const QDict *qdict);
void qemu_announce_self(void);
diff --git a/qapi-schema.json b/qapi-schema.json
index c15adc4..27e2036 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3540,3 +3540,22 @@
##
{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'},
'returns': 'SnapshotInfo' }
+
+##
+# @vm-snapshot-save:
+#
+# Create a snapshot of the whole virtual machine. Provided name is used as human
+# readable identifier. If there is already a snapshot with the same @name it
+# returns an error.
+#
+# The VM is automatically stopped and resumed and saving a snapshot can take
+# a long time.
+#
+# @name: tag of new snapshot
+#
+# Returns: SnapshotInfo on success
+#
+# Since: 1.5
+##
+{ 'command': 'vm-snapshot-save', 'data': {'name': 'str'},
+ 'returns': 'SnapshotInfo' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 445bd74..c8e2779 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1499,6 +1499,45 @@ Example:
EQMP
{
+ .name = "vm-snapshot-save",
+ .args_type = "name:s",
+ .params = "name",
+ .help = "save a VM snapshot",
+ .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
+ },
+
+SQMP
+vm-snapshot-save
+------
+
+Create a snapshot of the whole virtual machine. Provided name is used as human
+readable identifier. If there is already a snapshot with the same name it
+returns an error.
+
+The VM is automatically stopped and resumed and saving a snapshot can take
+a long time.
+
+Arguments:
+
+- "name": tag of new snapshot (json-string)
+
+Example:
+
+-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
+<- {
+ "return": {
+ "id": "1",
+ "name": "my_snapshot",
+ "date-sec": 1364480534,
+ "date-nsec": 978215000,
+ "vm-clock-sec": 5,
+ "vm-clock-nsec": 153620449,
+ "vm-state-size": 5709953
+ }
+ }
+
+EQMP
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/savevm.c b/savevm.c
index 749d49d..2e849b8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2325,48 +2325,19 @@ static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
return found;
}
-/*
- * Deletes snapshots of a given name in all opened images.
- */
-static int del_existing_snapshots(Monitor *mon, const char *name)
-{
- BlockDriverState *bs;
- QEMUSnapshotInfo sn1, *snapshot = &sn1;
- Error *local_err = NULL;
-
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (bdrv_can_snapshot(bs) &&
- bdrv_snapshot_find(bs, snapshot, name, NULL, NULL, true))
- {
- bdrv_snapshot_delete(bs, name, &local_err);
- if (error_is_set(&local_err)) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
- "old snapshot on device '%s': %s",
- bdrv_get_device_name(bs),
- error_get_pretty(local_err));
- error_free(local_err);
- return -1;
- }
- }
- }
-
- return 0;
-}
-
-void do_savevm(Monitor *mon, const QDict *qdict)
+SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
+ SnapshotInfo *info = NULL;
QEMUFile *f;
int saved_vm_running;
uint64_t vm_state_size;
qemu_timeval tv;
- struct tm tm;
- const char *name = qdict_get_try_str(qdict, "name");
Error *local_err = NULL;
- /* Verify if there is a device that doesn't support snapshots and is writable */
+ /* Verify if there is a device that doesn't support snapshots and is
+ * writable */
bs = NULL;
while ((bs = bdrv_next(bs))) {
@@ -2375,16 +2346,16 @@ void do_savevm(Monitor *mon, const QDict *qdict)
}
if (!bdrv_can_snapshot(bs)) {
- monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
- bdrv_get_device_name(bs));
- return;
+ error_setg(errp, "Device '%s' is writable but does not support "
+ "snapshots", bdrv_get_device_name(bs));
+ return NULL;
}
}
bs = bdrv_snapshots();
if (!bs) {
- monitor_printf(mon, "No block device can accept snapshots\n");
- return;
+ error_setg(errp, "No block device can accept snapshots");
+ return NULL;
}
saved_vm_running = runstate_is_running();
@@ -2398,36 +2369,24 @@ void do_savevm(Monitor *mon, const QDict *qdict)
sn->date_nsec = tv.tv_usec * 1000;
sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
- if (name) {
- if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
- pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
- pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
- } else {
- pstrcpy(sn->name, sizeof(sn->name), name);
- }
- } else {
- /* cast below needed for OpenBSD where tv_sec is still 'long' */
- localtime_r((const time_t *)&tv.tv_sec, &tm);
- strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
- }
-
- /* Delete old snapshots of the same name */
- if (name && del_existing_snapshots(mon, name) < 0) {
+ if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, false)) {
+ error_setg(errp, "Snapshot '%s' exists", name);
goto the_end;
+ } else {
+ pstrcpy(sn->name, sizeof(sn->name), name);
}
/* save the VM state */
f = qemu_fopen_bdrv(bs, 1);
if (!f) {
- monitor_printf(mon, "Could not open VM state file\n");
+ error_setg(errp, "Failed to open '%s' file", bdrv_get_device_name(bs));
goto the_end;
}
qemu_savevm_state(f, &local_err);
vm_state_size = qemu_ftell(f);
qemu_fclose(f);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
+ error_propagate(errp, local_err);
goto the_end;
}
@@ -2440,18 +2399,29 @@ void do_savevm(Monitor *mon, const QDict *qdict)
sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
bdrv_snapshot_create(bs1, sn, &local_err);
if (error_is_set(&local_err)) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR,
- "Failed to create snapshot for device '%s': %s",
- bdrv_get_device_name(bs1),
- error_get_pretty(local_err));
+ error_setg(errp, "Failed to create snapshot for "
+ "device '%s': %s", bdrv_get_device_name(bs1),
+ error_get_pretty(local_err));
error_free(local_err);
+ goto the_end;
}
}
}
+ 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 = vm_state_size;
+ info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000;
+ info->vm_clock_sec = sn->vm_clock_nsec / 1000000000;
+
the_end:
if (saved_vm_running)
vm_start();
+
+ return info;
}
void qmp_xen_save_devices_state(const char *filename, Error **errp)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 11/12] qapi: Convert savevm
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 11/12] qapi: Convert savevm Pavel Hrdina
@ 2013-05-03 12:52 ` Kevin Wolf
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 12:52 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-save takes one parameter name and the name is
> mandatory. The command returns SnapshotInfo on success, otherwise it returns
> an error message. If there is a snapshot with the same name it also returns
> an error message and if you want to overwrite that snapshot, you will have to
> at first call vm-snapshot-delete.
>
> HMP command savevm now has one optional parameter name and one flag -f.
> If the name parameter isn't provided the HMP command will create new one for
> internally used vm-snapshot-save. You can also specify the -f flag for overwrite
> existing snapshot which will internally use vm-snapshot-delete before
> vm-snapshot-save, otherwise it will print an error message if there is already
> a snapshot with the same name. If something else goes wrong, an error message
> will be printed.
>
> These improves behavior of the command to let you select only the name of the
> snapshot you want to create. This will ensure that if you want snapshot with
> the name '2', it will not rewrite or fail if there is any snapshot with id '2'.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1491,3 +1491,52 @@ void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
> qapi_free_SnapshotInfo(info);
> hmp_handle_error(mon, &local_err);
> }
> +
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> +{
> + const char *name = qdict_get_try_str(qdict, "name");
> + bool force = qdict_get_try_bool(qdict, "force", 0);
> + Error *local_err = NULL;
> + SnapshotInfo *info = NULL;
> + qemu_timeval tv;
> + struct tm tm;
> + char tmp_name[256];
> +
> + if (!name) {
> + localtime_r((const time_t *)&tv.tv_sec, &tm);
> + strftime(tmp_name, sizeof(tmp_name), "vm-%Y%m%d%H%M%S", &tm);
> + name = tmp_name;
> + }
> +
> + if (force) {
> + info = qmp_vm_snapshot_delete(true, name, false, NULL, &local_err);
> + /* We don't need print info about deleted snapshot. It still needs to
> + * be freed. */
> + qapi_free_SnapshotInfo(info);
> + if (error_is_set(&local_err)) {
> + hmp_handle_error(mon, &local_err);
> + return;
> + }
> + }
Deleting a snapshot that doesn't exist returns an error. This means that
you can use -f _only_ to overwrite a snapshot. I think this is
unexpected, with -f all cases that work without -f should keep working.
> +
> + info = qmp_vm_snapshot_save(name, &local_err);
> +
> + if (info) {
> + char buf[256];
> + QEMUSnapshotInfo sn = {
> + .vm_state_size = info->vm_state_size,
> + .date_sec = info->date_sec,
> + .date_nsec = info->date_nsec,
> + .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
> + info->vm_clock_nsec,
> + };
> + pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
> + pstrcpy(sn.name, sizeof(sn.name), info->name);
> + monitor_printf(mon, "Created snapshot's info:\n");
> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
> + }
> +
> + qapi_free_SnapshotInfo(info);
> + hmp_handle_error(mon, &local_err);
> +}
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2398,36 +2369,24 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> sn->date_nsec = tv.tv_usec * 1000;
> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>
> - if (name) {
> - if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
> - pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> - pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> - } else {
> - pstrcpy(sn->name, sizeof(sn->name), name);
> - }
> - } else {
> - /* cast below needed for OpenBSD where tv_sec is still 'long' */
> - localtime_r((const time_t *)&tv.tv_sec, &tm);
> - strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> - }
> -
> - /* Delete old snapshots of the same name */
> - if (name && del_existing_snapshots(mon, name) < 0) {
> + if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, false)) {
> + error_setg(errp, "Snapshot '%s' exists", name);
This is only checked for the VM state device. For all other devices the
case isn't handled any more.
> goto the_end;
> + } else {
> + pstrcpy(sn->name, sizeof(sn->name), name);
> }
>
> /* save the VM state */
> f = qemu_fopen_bdrv(bs, 1);
> if (!f) {
> - monitor_printf(mon, "Could not open VM state file\n");
> + error_setg(errp, "Failed to open '%s' file", bdrv_get_device_name(bs));
> goto the_end;
> }
> qemu_savevm_state(f, &local_err);
> vm_state_size = qemu_ftell(f);
> qemu_fclose(f);
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> + error_propagate(errp, local_err);
> goto the_end;
> }
>
> @@ -2440,18 +2399,29 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
> bdrv_snapshot_create(bs1, sn, &local_err);
> if (error_is_set(&local_err)) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR,
> - "Failed to create snapshot for device '%s': %s",
> - bdrv_get_device_name(bs1),
> - error_get_pretty(local_err));
> + error_setg(errp, "Failed to create snapshot for "
> + "device '%s': %s", bdrv_get_device_name(bs1),
> + error_get_pretty(local_err));
> error_free(local_err);
> + goto the_end;
> }
> }
> }
>
> + 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 = vm_state_size;
> + info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000;
> + info->vm_clock_sec = sn->vm_clock_nsec / 1000000000;
You could consider using a compound literal for better readability:
info = g_malloc(...);
*info = (SnapshotInfo) {
.id = g_strdup(sn->id_str),
.name = g_strdup(sn->name),
.date_nsec = sn->date_nsec,
...
};
> the_end:
> if (saved_vm_running)
> vm_start();
> +
> + return info;
> }
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH v2 12/12] savevm: remove backward compatibility from bdrv_snapshot_find()
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (10 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 11/12] qapi: Convert savevm Pavel Hrdina
@ 2013-04-24 15:32 ` Pavel Hrdina
2013-05-03 12:55 ` Kevin Wolf
2013-04-24 16:15 ` [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Eric Blake
2013-04-25 13:34 ` Stefan Hajnoczi
13 siblings, 1 reply; 44+ messages in thread
From: Pavel Hrdina @ 2013-04-24 15:32 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, phrdina, armbru, lcapitulino, xiawenc
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
savevm.c | 35 ++++++++++++-----------------------
1 file changed, 12 insertions(+), 23 deletions(-)
diff --git a/savevm.c b/savevm.c
index 2e849b8..45d46c6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2263,8 +2263,7 @@ out:
}
static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name, const char *id, Error **errp,
- bool old_match)
+ const char *name, const char *id, Error **errp)
{
QEMUSnapshotInfo *sn_tab, *sn;
Error *local_err = NULL;
@@ -2293,20 +2292,10 @@ static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
break;
}
} else if (name) {
- /* for compatibility for old bdrv_snapshot_find call
- * will be removed */
- if (old_match) {
- if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
- *sn_info = *sn;
- found = true;
- break;
- }
- } else {
- if (!strcmp(sn->name, name)) {
- *sn_info = *sn;
- found = true;
- break;
- }
+ if (!strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ found = true;
+ break;
}
} else if (id) {
if (!strcmp(sn->id_str, id)) {
@@ -2369,7 +2358,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
sn->date_nsec = tv.tv_usec * 1000;
sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
- if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, false)) {
+ if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL)) {
error_setg(errp, "Snapshot '%s' exists", name);
goto the_end;
} else {
@@ -2478,7 +2467,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
}
/* Don't even try to load empty VM states */
- if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, &local_err, false)) {
+ if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, &local_err)) {
error_setg(errp, "Snapshot doesn't exist: %s",
error_get_pretty(local_err));
error_free(local_err);
@@ -2506,7 +2495,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
return NULL;
}
- if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
+ if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err)) {
error_setg(errp, "Snapshot doesn't exist for device '%s': %s",
bdrv_get_device_name(bs), error_get_pretty(local_err));
error_free(local_err);
@@ -2599,7 +2588,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
return NULL;
}
- if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
+ if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err)) {
error_propagate(errp, local_err);
return NULL;
}
@@ -2616,7 +2605,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs) &&
- bdrv_snapshot_find(bs, &sn, name, id, NULL, false)) {
+ bdrv_snapshot_find(bs, &sn, name, id, NULL)) {
/* Small hack to ensure that proper snapshot is deleted for every
* block driver. This will be fixed soon. */
if (!strcmp(bs->drv->format_name, "rbd")) {
@@ -2678,8 +2667,8 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- if (!bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL, NULL,
- true)) {
+ if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
+ NULL)) {
available = 0;
break;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/12] savevm: remove backward compatibility from bdrv_snapshot_find()
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 12/12] savevm: remove backward compatibility from bdrv_snapshot_find() Pavel Hrdina
@ 2013-05-03 12:55 ` Kevin Wolf
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Wolf @ 2013-05-03 12:55 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: xiawenc, lcapitulino, qemu-devel, armbru
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> savevm.c | 35 ++++++++++++-----------------------
> 1 file changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 2e849b8..45d46c6 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2263,8 +2263,7 @@ out:
> }
>
> static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> - const char *name, const char *id, Error **errp,
> - bool old_match)
> + const char *name, const char *id, Error **errp)
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> Error *local_err = NULL;
> @@ -2293,20 +2292,10 @@ static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> break;
> }
> } else if (name) {
> - /* for compatibility for old bdrv_snapshot_find call
> - * will be removed */
> - if (old_match) {
> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> - *sn_info = *sn;
> - found = true;
> - break;
> - }
> - } else {
> - if (!strcmp(sn->name, name)) {
> - *sn_info = *sn;
> - found = true;
> - break;
> - }
> + if (!strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + found = true;
> + break;
> }
> } else if (id) {
> if (!strcmp(sn->id_str, id)) {
> @@ -2369,7 +2358,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
> sn->date_nsec = tv.tv_usec * 1000;
> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>
> - if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, false)) {
> + if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL)) {
> error_setg(errp, "Snapshot '%s' exists", name);
> goto the_end;
> } else {
> @@ -2478,7 +2467,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
> }
>
> /* Don't even try to load empty VM states */
> - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, &local_err, false)) {
> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, &local_err)) {
> error_setg(errp, "Snapshot doesn't exist: %s",
> error_get_pretty(local_err));
> error_free(local_err);
> @@ -2506,7 +2495,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
> return NULL;
> }
>
> - if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
> + if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err)) {
> error_setg(errp, "Snapshot doesn't exist for device '%s': %s",
> bdrv_get_device_name(bs), error_get_pretty(local_err));
> error_free(local_err);
> @@ -2599,7 +2588,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> return NULL;
> }
>
> - if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
> + if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err)) {
> error_propagate(errp, local_err);
> return NULL;
> }
> @@ -2616,7 +2605,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs) &&
> - bdrv_snapshot_find(bs, &sn, name, id, NULL, false)) {
> + bdrv_snapshot_find(bs, &sn, name, id, NULL)) {
> /* Small hack to ensure that proper snapshot is deleted for every
> * block driver. This will be fixed soon. */
> if (!strcmp(bs->drv->format_name, "rbd")) {
> @@ -2678,8 +2667,8 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>
> while ((bs1 = bdrv_next(bs1))) {
> if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> - if (!bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL, NULL,
> - true)) {
> + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
> + NULL)) {
Now you require that both name and ID of the snapshots are equal. I
think in practice it's only the name that matches (which means that the
code is bad already before this patch).
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (11 preceding siblings ...)
2013-04-24 15:32 ` [Qemu-devel] [PATCH v2 12/12] savevm: remove backward compatibility from bdrv_snapshot_find() Pavel Hrdina
@ 2013-04-24 16:15 ` Eric Blake
2013-04-24 17:12 ` Luiz Capitulino
2013-04-25 13:34 ` Stefan Hajnoczi
13 siblings, 1 reply; 44+ messages in thread
From: Eric Blake @ 2013-04-24 16:15 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 1987 bytes --]
On 04/24/2013 09:31 AM, Pavel Hrdina wrote:
> I'm sending patches for all commands in one patch series because the
> savevm command depends on delvm command.
I debated whether to call this out, but here goes:
I pointed out s/covert/convert/ on the subject line of v1; that can't
bode too well if the v2 still has the same problem, even if the problem
does not affect anything that gets committed into git.
Here's hoping the rest of the series is better...
>
> This patch series introduces new design of these commands:
At this point, we've missed soft freeze. Is this something we are still
trying to get into 1.5, or is it okay to slip it to 1.6 where we can
feel more comfortable about having a solid review in place? Ultimately,
it's the maintainer's call, but I'm personally leaning towards deferring
- libvirt has survived with HMP savevm long enough that another qemu
release without QMP savevm won't be the end of the world. I'm also
hoping that the final product is clean enough that a distro could feel
comfortable with the idea of rebasing to 1.5 while still backporting the
QMP commands that go into 1.6.
>
> changes from v1:
> - patch for updating bdrv_snapshot_goto and bdrv_snapshot_list is split
> into two patches
> - fixes typos and grammar
> - vm-snapshot-delete and vm-snapshot-load now returns an error also if
> snapshot for delete or load not exists
> - all error messages starts with uppercase and are without trailing dot
> - updated error messages recording to comments
...at least the summary makes it sound like you made a good effort. It
might also help to provide a bit of context on how we are planning on
tackling any merge conflicts regarding the algorithm for snapshot
lookup, and whether there are any rebase dependencies between your
series and Wenchao's.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi
2013-04-24 16:15 ` [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Eric Blake
@ 2013-04-24 17:12 ` Luiz Capitulino
0 siblings, 0 replies; 44+ messages in thread
From: Luiz Capitulino @ 2013-04-24 17:12 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, xiawenc, Pavel Hrdina, qemu-devel, armbru
On Wed, 24 Apr 2013 10:15:54 -0600
Eric Blake <eblake@redhat.com> wrote:
> > This patch series introduces new design of these commands:
>
> At this point, we've missed soft freeze. Is this something we are still
> trying to get into 1.5, or is it okay to slip it to 1.6 where we can
> feel more comfortable about having a solid review in place? Ultimately,
> it's the maintainer's call, but I'm personally leaning towards deferring
> - libvirt has survived with HMP savevm long enough that another qemu
> release without QMP savevm won't be the end of the world. I'm also
> hoping that the final product is clean enough that a distro could feel
> comfortable with the idea of rebasing to 1.5 while still backporting the
> QMP commands that go into 1.6.
I prefer to wait for 1.6 too.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi
2013-04-24 15:31 [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Pavel Hrdina
` (12 preceding siblings ...)
2013-04-24 16:15 ` [Qemu-devel] [PATCH v2 00/12] covert savevm, loadvm and delvm into qapi Eric Blake
@ 2013-04-25 13:34 ` Stefan Hajnoczi
13 siblings, 0 replies; 44+ messages in thread
From: Stefan Hajnoczi @ 2013-04-25 13:34 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: kwolf, xiawenc, lcapitulino, qemu-devel, armbru
On Wed, Apr 24, 2013 at 05:31:58PM +0200, Pavel Hrdina wrote:
> I'm sending patches for all commands in one patch series because the
> savevm command depends on delvm command.
>
> This patch series introduces new design of these commands:
>
> * QMP vm-snapshot-save:
> - { 'command': 'vm-snapshot-save',
> 'data': { 'name': 'str' },
> 'returns': 'SnapshotInfo' }
> - vm-snapshot-save returns an error if there is an existing snapshot with
> the same name
> - you cannot provide an id for a new snapshot
> - all information about created snapshot will be returned
>
> * QMP vm-snapshot-load
> - { 'command': 'vm-snapshot-load',
> 'data': { '*name': 'str', '*id': 'str' },
> 'returns': 'SnapshotInfo' }
> - one of the name or id must be provided
> - if both are provided they will match only the snapshot with the same name
> and id
> - returns SnapshotInfo only if the snapshot exists.
>
> * QMP vm-snapshot-delete:
> - { 'command': 'vm-snapshot-delete',
> 'data': { '*name': 'str', '*id': 'str' },
> 'returns': 'SnapshotInfo' }
> - same rules as vm-snapshot-load
If I cannot specify the id when creating the snapshot, then why can I
provide it when loading/deleting?
In other words, is it useful to support the id arguments? If yes,
please include the reason in the API documentation.
^ permalink raw reply [flat|nested] 44+ messages in thread