* [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 0:52 ` Eric Blake
2012-12-14 16:00 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
` (17 subsequent siblings)
18 siblings, 2 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
error.c | 8 ++++++++
error.h | 6 ++++++
2 files changed, 14 insertions(+)
diff --git a/error.c b/error.c
index 128d88c..dd3ab66 100644
--- a/error.c
+++ b/error.c
@@ -113,3 +113,11 @@ void error_propagate(Error **dst_err, Error *local_err)
error_free(local_err);
}
}
+
+void handle_error(Error **errp)
+{
+ if (error_is_set(errp)) {
+ error_report("%s", error_get_pretty(*errp));
+ error_free(*errp);
+ }
+}
diff --git a/error.h b/error.h
index 4d52e73..6a6acb5 100644
--- a/error.h
+++ b/error.h
@@ -77,4 +77,10 @@ void error_propagate(Error **dst_err, Error *local_err);
*/
void error_free(Error *err);
+/**
+ * Print an error object as pretty string to current monitor or on stderr, then
+ * free the errot object.
+ */
+void handle_error(Error **errp);
+
#endif
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error Pavel Hrdina
@ 2012-12-14 0:52 ` Eric Blake
2012-12-14 16:00 ` Luiz Capitulino
1 sibling, 0 replies; 33+ messages in thread
From: Eric Blake @ 2012-12-14 0:52 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 452 bytes --]
On 12/13/2012 08:40 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> error.c | 8 ++++++++
> error.h | 6 ++++++
> 2 files changed, 14 insertions(+)
>
>
> +/**
> + * Print an error object as pretty string to current monitor or on stderr, then
> + * free the errot object.
s/errot/error/
--
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: 619 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error Pavel Hrdina
2012-12-14 0:52 ` Eric Blake
@ 2012-12-14 16:00 ` Luiz Capitulino
1 sibling, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:00 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:35 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> error.c | 8 ++++++++
> error.h | 6 ++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/error.c b/error.c
> index 128d88c..dd3ab66 100644
> --- a/error.c
> +++ b/error.c
> @@ -113,3 +113,11 @@ void error_propagate(Error **dst_err, Error *local_err)
> error_free(local_err);
> }
> }
> +
> +void handle_error(Error **errp)
> +{
> + if (error_is_set(errp)) {
> + error_report("%s", error_get_pretty(*errp));
> + error_free(*errp);
> + }
This is not good for a few reasons. The most important ones are that
we most probably shouldn't be using error_report() in new qapi code. The
other reason is that this function doesn't actually handle the error.
I've added a similar function in hmp.c but just to save some typing,
but we shouldn't do this elsewhere.
> +}
> diff --git a/error.h b/error.h
> index 4d52e73..6a6acb5 100644
> --- a/error.h
> +++ b/error.h
> @@ -77,4 +77,10 @@ void error_propagate(Error **dst_err, Error *local_err);
> */
> void error_free(Error *err);
>
> +/**
> + * Print an error object as pretty string to current monitor or on stderr, then
> + * free the errot object.
> + */
> +void handle_error(Error **errp);
> +
> #endif
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 16:29 ` Luiz Capitulino
2012-12-14 16:31 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 03/17] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
` (16 subsequent siblings)
18 siblings, 2 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 26 ++++++++++++++++++--------
block.h | 3 ++-
block/qcow2-snapshot.c | 11 ++++++++++-
block/qcow2.h | 4 +++-
block/rbd.c | 17 +++++++++++------
block/sheepdog.c | 18 ++++++++++--------
block_int.h | 3 ++-
qemu-img.c | 8 +++-----
savevm.c | 2 +-
9 files changed, 60 insertions(+), 32 deletions(-)
diff --git a/block.c b/block.c
index c05875f..fea429b 100644
--- a/block.c
+++ b/block.c
@@ -3075,16 +3075,26 @@ BlockDriverState *bdrv_snapshots(void)
}
int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
+ 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;
+ int ret;
+
+ if (!drv) {
+ error_setg(errp, "Device '%s' has no medium.",
+ bdrv_get_device_name(bs));
+ ret = -ENOMEDIUM;
+ } else if (drv->bdrv_snapshot_create) {
+ ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
+ } else if (bs->file) {
+ ret = bdrv_snapshot_create(bs->file, sn_info, errp);
+ } else {
+ error_setg(errp, "Not supported.");
+ ret = -ENOTSUP;
+ }
+
+ return ret;
}
int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block.h b/block.h
index 722c620..6abb687 100644
--- a/block.h
+++ b/block.h
@@ -321,7 +321,8 @@ 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);
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id);
int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4e7c93b..3fd8aff 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -25,6 +25,7 @@
#include "qemu-common.h"
#include "block_int.h"
#include "block/qcow2.h"
+#include "qerror.h"
typedef struct QEMU_PACKED QCowSnapshotHeader {
/* header is 8 byte aligned */
@@ -312,7 +313,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)
+int qcow2_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot *new_snapshot_list = NULL;
@@ -331,6 +334,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
/* Check that the ID is unique */
if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+ error_setg(errp, "Parameter 'name' has to be unique ID.");
return -EEXIST;
}
@@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
if (l1_table_offset < 0) {
ret = l1_table_offset;
+ error_setg(errp, "Failed to allocate L1 table.");
goto fail;
}
@@ -362,6 +367,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(errp, "Failed to save L1 table.");
goto fail;
}
@@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
*/
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
if (ret < 0) {
+ error_setg(errp, "Failed to update snapshot refcount.");
goto fail;
}
ret = bdrv_flush(bs);
if (ret < 0) {
+ error_setg(errp, "Failed to flush data.");
goto fail;
}
@@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
if (ret < 0) {
g_free(s->snapshots);
s->snapshots = old_snapshot_list;
+ error_setg(errp, "Failed to write new snapshots.");
goto fail;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index b4eb654..854bd12 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -308,7 +308,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);
+int qcow2_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
diff --git a/block/rbd.c b/block/rbd.c
index f3becc7..cb5acf8 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -16,6 +16,7 @@
#include "qemu-common.h"
#include "qemu-error.h"
#include "block_int.h"
+#include "qerror.h"
#include <rbd/librbd.h>
@@ -811,12 +812,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
}
static int qemu_rbd_snap_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
- int r;
+ int ret;
if (sn_info->name[0] == '\0') {
+ error_setg(errp, "Parameter 'name' cannot be empty.");
return -EINVAL; /* we need a name for rbd snapshots */
}
@@ -826,17 +829,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
*/
if (sn_info->id_str[0] != '\0' &&
strcmp(sn_info->id_str, sn_info->name) != 0) {
+ error_setg(errp, "ID and name have to be equal.");
return -EINVAL;
}
if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+ error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
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;
+ ret = rbd_snap_create(s->image, sn_info->name);
+ if (ret < 0) {
+ error_setg(errp, "Failed to create snapshot.");
+ return ret;
}
return 0;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a48f58c..094634a 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -17,6 +17,7 @@
#include "qemu_socket.h"
#include "block_int.h"
#include "bitops.h"
+#include "qerror.h"
#define SD_PROTO_VER 0x01
@@ -1735,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
return 0;
}
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static int sd_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BDRVSheepdogState *s = bs->opaque;
int ret, fd;
@@ -1748,9 +1751,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
s->name, sn_info->vm_state_size, s->is_snapshot);
if (s->is_snapshot) {
- error_report("You can't create a snapshot of a snapshot VDI, "
- "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
+ error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
+ sn_info->name);
return -EINVAL;
}
@@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
fd = connect_to_sdog(s->addr, s->port);
if (fd < 0) {
ret = fd;
+ error_setg(errp, "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_enabled);
if (ret < 0) {
- error_report("failed to write snapshot's inode.");
+ error_setg(errp, "Failed to write snapshot's inode.");
goto cleanup;
}
ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
s->addr, s->port);
if (ret < 0) {
- error_report("failed to create inode for snapshot. %s",
- strerror(errno));
+ error_setg(errp, "Failed to create inode for snapshot.");
goto cleanup;
}
@@ -1793,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
s->inode.nr_copies, datalen, 0, s->cache_enabled);
if (ret < 0) {
- error_report("failed to read new inode info. %s", strerror(errno));
+ error_setg(errp, "Failed to read new inode info.");
goto cleanup;
}
diff --git a/block_int.h b/block_int.h
index 9deedb8..976ea1f 100644
--- a/block_int.h
+++ b/block_int.h
@@ -147,7 +147,8 @@ struct BlockDriver {
const uint8_t *buf, int nb_sectors);
int (*bdrv_snapshot_create)(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int (*bdrv_snapshot_goto)(BlockDriverState *bs,
const char *snapshot_id);
int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
diff --git a/qemu-img.c b/qemu-img.c
index e29e01b..93e4022 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)
int c, ret = 0, bdrv_oflags;
int action = 0;
qemu_timeval tv;
+ Error *err = NULL;
bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
/* Parse commandline parameters */
@@ -1559,11 +1560,7 @@ 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));
- }
+ ret = bdrv_snapshot_create(bs, &sn, &err);
break;
case SNAPSHOT_APPLY:
@@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)
/* Cleanup */
bdrv_delete(bs);
+ handle_error(&err);
if (ret) {
return 1;
}
diff --git a/savevm.c b/savevm.c
index 5d04d59..ef2f305 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2211,7 +2211,7 @@ 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);
+ ret = bdrv_snapshot_create(bs1, sn, NULL);
if (ret < 0) {
monitor_printf(mon, "Error while creating snapshot on '%s'\n",
bdrv_get_device_name(bs1));
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
@ 2012-12-14 16:29 ` Luiz Capitulino
2012-12-14 16:31 ` Luiz Capitulino
1 sibling, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:29 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:36 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 26 ++++++++++++++++++--------
> block.h | 3 ++-
> block/qcow2-snapshot.c | 11 ++++++++++-
> block/qcow2.h | 4 +++-
> block/rbd.c | 17 +++++++++++------
> block/sheepdog.c | 18 ++++++++++--------
> block_int.h | 3 ++-
> qemu-img.c | 8 +++-----
> savevm.c | 2 +-
> 9 files changed, 60 insertions(+), 32 deletions(-)
>
> diff --git a/block.c b/block.c
> index c05875f..fea429b 100644
> --- a/block.c
> +++ b/block.c
> @@ -3075,16 +3075,26 @@ BlockDriverState *bdrv_snapshots(void)
> }
>
> int bdrv_snapshot_create(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info)
> + 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;
> + int ret;
> +
> + if (!drv) {
> + error_setg(errp, "Device '%s' has no medium.",
> + bdrv_get_device_name(bs));
> + ret = -ENOMEDIUM;
> + } else if (drv->bdrv_snapshot_create) {
> + ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
> + } else if (bs->file) {
> + ret = bdrv_snapshot_create(bs->file, sn_info, errp);
> + } else {
> + error_setg(errp, "Not supported.");
> + ret = -ENOTSUP;
> + }
> +
> + return ret;
This function returns an error code _and_ an error object. This is fine
if you drop the error code in a later patch, but you don't do it in this
series.
IMO, we shouldn't return both unless there's a strong reason to do so
(ie. you need the error code and a very specific error message, or you
plan to drop the error code in the future). If this is the case here,
please tell us.
When converting functions that return an error code, I check if any
caller of the function check for a specific error code. If no caller
does this, then I just drop the error code (but in a different patch).
If a caller actually checks the error code, you generally have two
options:
1. See if checking the error code is actually required, if it's
not then just drop it
2. Don't add the Error object to the function returning the error code.
Add it to the callers instead
We (block layer guys and me) had a discussion about this some weeks ago,
I don't exactly remember if what I described above matches our conclusions,
so I'm CC'ing them.
> }
>
> int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 722c620..6abb687 100644
> --- a/block.h
> +++ b/block.h
> @@ -321,7 +321,8 @@ 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);
> + QEMUSnapshotInfo *sn_info,
> + Error **errp);
> int bdrv_snapshot_goto(BlockDriverState *bs,
> const char *snapshot_id);
> int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 4e7c93b..3fd8aff 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -25,6 +25,7 @@
> #include "qemu-common.h"
> #include "block_int.h"
> #include "block/qcow2.h"
> +#include "qerror.h"
I think you want to include error.h. qerror.h is legacy.
> typedef struct QEMU_PACKED QCowSnapshotHeader {
> /* header is 8 byte aligned */
> @@ -312,7 +313,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)
> +int qcow2_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot *new_snapshot_list = NULL;
> @@ -331,6 +334,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>
> /* Check that the ID is unique */
> if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
> + error_setg(errp, "Parameter 'name' has to be unique ID.");
Here you're converting qcow2_snapshot_create() to propagate the error, and
I see that you do it for more functions below.
Basically, this kind of conversion should be split into one per-patch or
even one per-series depending on the complexity.
A good series converting do_foo() to take an Error object would do:
1. Add an Error object to do_foo() and fill it properly. Callers of do_foo()
should pass NULL for the new argument
2. For each caller of do_foo() create a new patch, which changes the caller
to pass an Error object to do_foo() and use it internally
3. If do_foo() returns an error code, drop it
> return -EEXIST;
> }
>
> @@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
> if (l1_table_offset < 0) {
> ret = l1_table_offset;
> + error_setg(errp, "Failed to allocate L1 table.");
> goto fail;
> }
>
> @@ -362,6 +367,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(errp, "Failed to save L1 table.");
> goto fail;
> }
>
> @@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> */
> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
> if (ret < 0) {
> + error_setg(errp, "Failed to update snapshot refcount.");
> goto fail;
> }
>
> ret = bdrv_flush(bs);
> if (ret < 0) {
> + error_setg(errp, "Failed to flush data.");
> goto fail;
> }
>
> @@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> if (ret < 0) {
> g_free(s->snapshots);
> s->snapshots = old_snapshot_list;
> + error_setg(errp, "Failed to write new snapshots.");
> goto fail;
> }
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b4eb654..854bd12 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -308,7 +308,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);
> +int qcow2_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info,
> + Error **errp);
> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
> int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
> diff --git a/block/rbd.c b/block/rbd.c
> index f3becc7..cb5acf8 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -16,6 +16,7 @@
> #include "qemu-common.h"
> #include "qemu-error.h"
> #include "block_int.h"
> +#include "qerror.h"
>
> #include <rbd/librbd.h>
>
> @@ -811,12 +812,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
> }
>
> static int qemu_rbd_snap_create(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info)
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BDRVRBDState *s = bs->opaque;
> - int r;
> + int ret;
>
> if (sn_info->name[0] == '\0') {
> + error_setg(errp, "Parameter 'name' cannot be empty.");
> return -EINVAL; /* we need a name for rbd snapshots */
> }
>
> @@ -826,17 +829,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
> */
> if (sn_info->id_str[0] != '\0' &&
> strcmp(sn_info->id_str, sn_info->name) != 0) {
> + error_setg(errp, "ID and name have to be equal.");
> return -EINVAL;
> }
>
> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> + error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
> 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;
> + ret = rbd_snap_create(s->image, sn_info->name);
> + if (ret < 0) {
> + error_setg(errp, "Failed to create snapshot.");
> + return ret;
> }
>
> return 0;
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index a48f58c..094634a 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -17,6 +17,7 @@
> #include "qemu_socket.h"
> #include "block_int.h"
> #include "bitops.h"
> +#include "qerror.h"
>
> #define SD_PROTO_VER 0x01
>
> @@ -1735,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
> return 0;
> }
>
> -static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> +static int sd_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BDRVSheepdogState *s = bs->opaque;
> int ret, fd;
> @@ -1748,9 +1751,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> s->name, sn_info->vm_state_size, s->is_snapshot);
>
> if (s->is_snapshot) {
> - error_report("You can't create a snapshot of a snapshot VDI, "
> - "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
> -
> + error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
> + sn_info->name);
> return -EINVAL;
> }
>
> @@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> fd = connect_to_sdog(s->addr, s->port);
> if (fd < 0) {
> ret = fd;
> + error_setg(errp, "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_enabled);
> if (ret < 0) {
> - error_report("failed to write snapshot's inode.");
> + error_setg(errp, "Failed to write snapshot's inode.");
> goto cleanup;
> }
>
> ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
> s->addr, s->port);
> if (ret < 0) {
> - error_report("failed to create inode for snapshot. %s",
> - strerror(errno));
> + error_setg(errp, "Failed to create inode for snapshot.");
> goto cleanup;
> }
>
> @@ -1793,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> s->inode.nr_copies, datalen, 0, s->cache_enabled);
>
> if (ret < 0) {
> - error_report("failed to read new inode info. %s", strerror(errno));
> + error_setg(errp, "Failed to read new inode info.");
> goto cleanup;
> }
>
> diff --git a/block_int.h b/block_int.h
> index 9deedb8..976ea1f 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -147,7 +147,8 @@ struct BlockDriver {
> const uint8_t *buf, int nb_sectors);
>
> int (*bdrv_snapshot_create)(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info);
> + QEMUSnapshotInfo *sn_info,
> + Error **errp);
> int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> const char *snapshot_id);
> int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..93e4022 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)
> int c, ret = 0, bdrv_oflags;
> int action = 0;
> qemu_timeval tv;
> + Error *err = NULL;
>
> bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
> /* Parse commandline parameters */
> @@ -1559,11 +1560,7 @@ 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));
> - }
> + ret = bdrv_snapshot_create(bs, &sn, &err);
> break;
>
> case SNAPSHOT_APPLY:
> @@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)
>
> /* Cleanup */
> bdrv_delete(bs);
> + handle_error(&err);
> if (ret) {
> return 1;
> }
> diff --git a/savevm.c b/savevm.c
> index 5d04d59..ef2f305 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2211,7 +2211,7 @@ 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);
> + ret = bdrv_snapshot_create(bs1, sn, NULL);
> if (ret < 0) {
> monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> bdrv_get_device_name(bs1));
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
2012-12-14 16:29 ` Luiz Capitulino
@ 2012-12-14 16:31 ` Luiz Capitulino
1 sibling, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:31 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, armbru
[Forgot adding block layer guys, re-sending]
On Thu, 13 Dec 2012 16:40:36 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 26 ++++++++++++++++++--------
> block.h | 3 ++-
> block/qcow2-snapshot.c | 11 ++++++++++-
> block/qcow2.h | 4 +++-
> block/rbd.c | 17 +++++++++++------
> block/sheepdog.c | 18 ++++++++++--------
> block_int.h | 3 ++-
> qemu-img.c | 8 +++-----
> savevm.c | 2 +-
> 9 files changed, 60 insertions(+), 32 deletions(-)
>
> diff --git a/block.c b/block.c
> index c05875f..fea429b 100644
> --- a/block.c
> +++ b/block.c
> @@ -3075,16 +3075,26 @@ BlockDriverState *bdrv_snapshots(void)
> }
>
> int bdrv_snapshot_create(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info)
> + 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;
> + int ret;
> +
> + if (!drv) {
> + error_setg(errp, "Device '%s' has no medium.",
> + bdrv_get_device_name(bs));
> + ret = -ENOMEDIUM;
> + } else if (drv->bdrv_snapshot_create) {
> + ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
> + } else if (bs->file) {
> + ret = bdrv_snapshot_create(bs->file, sn_info, errp);
> + } else {
> + error_setg(errp, "Not supported.");
> + ret = -ENOTSUP;
> + }
> +
> + return ret;
This function returns an error code _and_ an error object. This is fine
if you drop the error code in a later patch, but you don't do it in this
series.
IMO, we shouldn't return both unless there's a strong reason to do so
(ie. you need the error code and a very specific error message, or you
plan to drop the error code in the future). If this is the case here,
please tell us.
When converting functions that return an error code, I check if any
caller of the function check for a specific error code. If no caller
does this, then I just drop the error code (but in a different patch).
If a caller actually checks the error code, you generally have two
options:
1. See if checking the error code is actually required, if it's
not then just drop it
2. Don't add the Error object to the function returning the error code.
Add it to the callers instead
We (block layer guys and me) had a discussion about this some weeks ago,
I don't exactly remember if what I described above matches our conclusions,
so I'm CC'ing them.
> }
>
> int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 722c620..6abb687 100644
> --- a/block.h
> +++ b/block.h
> @@ -321,7 +321,8 @@ 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);
> + QEMUSnapshotInfo *sn_info,
> + Error **errp);
> int bdrv_snapshot_goto(BlockDriverState *bs,
> const char *snapshot_id);
> int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 4e7c93b..3fd8aff 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -25,6 +25,7 @@
> #include "qemu-common.h"
> #include "block_int.h"
> #include "block/qcow2.h"
> +#include "qerror.h"
I think you want to include error.h. qerror.h is legacy.
> typedef struct QEMU_PACKED QCowSnapshotHeader {
> /* header is 8 byte aligned */
> @@ -312,7 +313,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)
> +int qcow2_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot *new_snapshot_list = NULL;
> @@ -331,6 +334,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>
> /* Check that the ID is unique */
> if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
> + error_setg(errp, "Parameter 'name' has to be unique ID.");
Here you're converting qcow2_snapshot_create() to propagate the error, and
I see that you do it for more functions below.
Basically, this kind of conversion should be split into one per-patch or
even one per-series depending on the complexity.
A good series converting do_foo() to take an Error object would do:
1. Add an Error object to do_foo() and fill it properly. Callers of do_foo()
should pass NULL for the new argument
2. For each caller of do_foo() create a new patch, which changes the caller
to pass an Error object to do_foo() and use it internally
3. If do_foo() returns an error code, drop it
> return -EEXIST;
> }
>
> @@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
> if (l1_table_offset < 0) {
> ret = l1_table_offset;
> + error_setg(errp, "Failed to allocate L1 table.");
> goto fail;
> }
>
> @@ -362,6 +367,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(errp, "Failed to save L1 table.");
> goto fail;
> }
>
> @@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> */
> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
> if (ret < 0) {
> + error_setg(errp, "Failed to update snapshot refcount.");
> goto fail;
> }
>
> ret = bdrv_flush(bs);
> if (ret < 0) {
> + error_setg(errp, "Failed to flush data.");
> goto fail;
> }
>
> @@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> if (ret < 0) {
> g_free(s->snapshots);
> s->snapshots = old_snapshot_list;
> + error_setg(errp, "Failed to write new snapshots.");
> goto fail;
> }
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b4eb654..854bd12 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -308,7 +308,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);
> +int qcow2_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info,
> + Error **errp);
> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
> int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
> diff --git a/block/rbd.c b/block/rbd.c
> index f3becc7..cb5acf8 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -16,6 +16,7 @@
> #include "qemu-common.h"
> #include "qemu-error.h"
> #include "block_int.h"
> +#include "qerror.h"
>
> #include <rbd/librbd.h>
>
> @@ -811,12 +812,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
> }
>
> static int qemu_rbd_snap_create(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info)
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BDRVRBDState *s = bs->opaque;
> - int r;
> + int ret;
>
> if (sn_info->name[0] == '\0') {
> + error_setg(errp, "Parameter 'name' cannot be empty.");
> return -EINVAL; /* we need a name for rbd snapshots */
> }
>
> @@ -826,17 +829,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
> */
> if (sn_info->id_str[0] != '\0' &&
> strcmp(sn_info->id_str, sn_info->name) != 0) {
> + error_setg(errp, "ID and name have to be equal.");
> return -EINVAL;
> }
>
> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> + error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
> 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;
> + ret = rbd_snap_create(s->image, sn_info->name);
> + if (ret < 0) {
> + error_setg(errp, "Failed to create snapshot.");
> + return ret;
> }
>
> return 0;
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index a48f58c..094634a 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -17,6 +17,7 @@
> #include "qemu_socket.h"
> #include "block_int.h"
> #include "bitops.h"
> +#include "qerror.h"
>
> #define SD_PROTO_VER 0x01
>
> @@ -1735,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
> return 0;
> }
>
> -static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> +static int sd_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BDRVSheepdogState *s = bs->opaque;
> int ret, fd;
> @@ -1748,9 +1751,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> s->name, sn_info->vm_state_size, s->is_snapshot);
>
> if (s->is_snapshot) {
> - error_report("You can't create a snapshot of a snapshot VDI, "
> - "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
> -
> + error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
> + sn_info->name);
> return -EINVAL;
> }
>
> @@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> fd = connect_to_sdog(s->addr, s->port);
> if (fd < 0) {
> ret = fd;
> + error_setg(errp, "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_enabled);
> if (ret < 0) {
> - error_report("failed to write snapshot's inode.");
> + error_setg(errp, "Failed to write snapshot's inode.");
> goto cleanup;
> }
>
> ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
> s->addr, s->port);
> if (ret < 0) {
> - error_report("failed to create inode for snapshot. %s",
> - strerror(errno));
> + error_setg(errp, "Failed to create inode for snapshot.");
> goto cleanup;
> }
>
> @@ -1793,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> s->inode.nr_copies, datalen, 0, s->cache_enabled);
>
> if (ret < 0) {
> - error_report("failed to read new inode info. %s", strerror(errno));
> + error_setg(errp, "Failed to read new inode info.");
> goto cleanup;
> }
>
> diff --git a/block_int.h b/block_int.h
> index 9deedb8..976ea1f 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -147,7 +147,8 @@ struct BlockDriver {
> const uint8_t *buf, int nb_sectors);
>
> int (*bdrv_snapshot_create)(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info);
> + QEMUSnapshotInfo *sn_info,
> + Error **errp);
> int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> const char *snapshot_id);
> int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..93e4022 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)
> int c, ret = 0, bdrv_oflags;
> int action = 0;
> qemu_timeval tv;
> + Error *err = NULL;
>
> bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
> /* Parse commandline parameters */
> @@ -1559,11 +1560,7 @@ 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));
> - }
> + ret = bdrv_snapshot_create(bs, &sn, &err);
> break;
>
> case SNAPSHOT_APPLY:
> @@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)
>
> /* Cleanup */
> bdrv_delete(bs);
> + handle_error(&err);
> if (ret) {
> return 1;
> }
> diff --git a/savevm.c b/savevm.c
> index 5d04d59..ef2f305 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2211,7 +2211,7 @@ 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);
> + ret = bdrv_snapshot_create(bs1, sn, NULL);
> if (ret < 0) {
> monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> bdrv_get_device_name(bs1));
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 03/17] block: add error parameter to bdrv_snapshot_goto() and related functions
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 16:30 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 04/17] block: add error parameter to bdrv_snapshot_delete() " Pavel Hrdina
` (15 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 27 ++++++++++++++++-----------
block.h | 3 ++-
block/qcow2-snapshot.c | 15 ++++++++++++---
block/qcow2.h | 4 +++-
block/rbd.c | 6 +++++-
block/sheepdog.c | 14 +++++++-------
block_int.h | 3 ++-
qemu-img.c | 6 +-----
savevm.c | 2 +-
9 files changed, 49 insertions(+), 31 deletions(-)
diff --git a/block.c b/block.c
index fea429b..d012b0e 100644
--- a/block.c
+++ b/block.c
@@ -3098,29 +3098,34 @@ int bdrv_snapshot_create(BlockDriverState *bs,
}
int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id)
+ 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);
-
- if (bs->file) {
+ if (!drv) {
+ error_setg(errp, "Device '%s' has no medium.",
+ bdrv_get_device_name(bs));
+ ret = -ENOMEDIUM;
+ } else if (drv->bdrv_snapshot_goto) {
+ ret = drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
+ } else if (bs->file) {
drv->bdrv_close(bs);
- ret = bdrv_snapshot_goto(bs->file, snapshot_id);
+ ret = bdrv_snapshot_goto(bs->file, snapshot_id, errp);
open_ret = drv->bdrv_open(bs, bs->open_flags);
if (open_ret < 0) {
bdrv_delete(bs->file);
bs->drv = NULL;
- return open_ret;
+ error_setg(errp, "Failed to open '%s'.", bdrv_get_device_name(bs));
+ ret = open_ret;
}
- return ret;
+ } else {
+ error_setg(errp, "Not supported.");
+ ret = -ENOTSUP;
}
- return -ENOTSUP;
+ return ret;
}
int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
diff --git a/block.h b/block.h
index 6abb687..31642c2 100644
--- a/block.h
+++ b/block.h
@@ -324,7 +324,8 @@ int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info,
Error **errp);
int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id);
+ const char *snapshot_id,
+ Error **errp);
int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 3fd8aff..e07f9ac 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -428,7 +428,9 @@ fail:
}
/* copy the snapshot 'snapshot_name' into the current disk image */
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot *sn;
@@ -440,13 +442,14 @@ 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) {
+ error_setg(errp, "Failed to find snapshot '%s' by id or name.",
+ snapshot_id);
return -ENOENT;
}
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");
+ error_setg(errp, "Not supported.");
ret = -ENOTSUP;
goto fail;
}
@@ -458,6 +461,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(errp, "Failed to grow L1 table.");
goto fail;
}
@@ -476,18 +480,21 @@ 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(errp, "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(errp, "Failed to update snapshot refcount.");
goto fail;
}
ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
cur_l1_bytes);
if (ret < 0) {
+ error_setg(errp, "Failed to save L1 table.");
goto fail;
}
@@ -513,6 +520,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
}
if (ret < 0) {
+ error_setg(errp, "Failed to update snapshot refcount.");
goto fail;
}
@@ -525,6 +533,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(errp, "Failed to update snapshot refcount.");
goto fail;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 854bd12..6babb56 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -311,7 +311,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
int qcow2_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info,
Error **errp);
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
+int qcow2_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp);
int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
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 cb5acf8..3e789c6 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -858,12 +858,16 @@ static int qemu_rbd_snap_remove(BlockDriverState *bs,
}
static int qemu_rbd_snap_rollback(BlockDriverState *bs,
- const char *snapshot_name)
+ const char *snapshot_name,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
r = rbd_snap_rollback(s->image, snapshot_name);
+ if (r < 0) {
+ error_setg(errp, "Failed to rollback snapshot.");
+ }
return r;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 094634a..34ef75c 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1808,7 +1808,9 @@ cleanup:
return ret;
}
-static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BDRVSheepdogState *s = bs->opaque;
BDRVSheepdogState *old_s;
@@ -1833,14 +1835,14 @@ 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(errp, "Failed to find VDI snapshot '%s'.", vdi);
goto out;
}
fd = connect_to_sdog(s->addr, s->port);
if (fd < 0) {
- error_report("failed to connect");
ret = fd;
+ error_setg(errp, "Failed to connect to sdog.");
goto out;
}
@@ -1851,13 +1853,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
closesocket(fd);
if (ret) {
+ error_setg(errp, "Failed to open '%s'.", vdi);
goto out;
}
memcpy(&s->inode, buf, sizeof(s->inode));
if (!s->inode.vm_state_size) {
- error_report("Invalid snapshot");
+ error_setg(errp, "Invalid snapshot '%s'.", snapshot_id);
ret = -ENOENT;
goto out;
}
@@ -1873,9 +1876,6 @@ out:
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;
}
diff --git a/block_int.h b/block_int.h
index 976ea1f..016782c 100644
--- a/block_int.h
+++ b/block_int.h
@@ -150,7 +150,8 @@ struct BlockDriver {
QEMUSnapshotInfo *sn_info,
Error **errp);
int (*bdrv_snapshot_goto)(BlockDriverState *bs,
- const char *snapshot_id);
+ const char *snapshot_id,
+ Error **errp);
int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
int (*bdrv_snapshot_list)(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
diff --git a/qemu-img.c b/qemu-img.c
index 93e4022..91671a8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1564,11 +1564,7 @@ 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));
- }
+ ret = bdrv_snapshot_goto(bs, snapshot_name, &err);
break;
case SNAPSHOT_DELETE:
diff --git a/savevm.c b/savevm.c
index ef2f305..29e41fc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2301,7 +2301,7 @@ int load_vmstate(const char *name)
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs)) {
- ret = bdrv_snapshot_goto(bs, name);
+ ret = bdrv_snapshot_goto(bs, name, NULL);
if (ret < 0) {
error_report("Error %d while activating snapshot '%s' on '%s'",
ret, name, bdrv_get_device_name(bs));
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/17] block: add error parameter to bdrv_snapshot_goto() and related functions
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 03/17] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
@ 2012-12-14 16:30 ` Luiz Capitulino
0 siblings, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:30 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:37 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
The same comments I made in the previous patch apply here: you're
keeping the error code and converting callers to propagate error
in the same patch (which makes this difficult to review).
> ---
> block.c | 27 ++++++++++++++++-----------
> block.h | 3 ++-
> block/qcow2-snapshot.c | 15 ++++++++++++---
> block/qcow2.h | 4 +++-
> block/rbd.c | 6 +++++-
> block/sheepdog.c | 14 +++++++-------
> block_int.h | 3 ++-
> qemu-img.c | 6 +-----
> savevm.c | 2 +-
> 9 files changed, 49 insertions(+), 31 deletions(-)
>
> diff --git a/block.c b/block.c
> index fea429b..d012b0e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3098,29 +3098,34 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> }
>
> int bdrv_snapshot_goto(BlockDriverState *bs,
> - const char *snapshot_id)
> + 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);
> -
> - if (bs->file) {
> + if (!drv) {
> + error_setg(errp, "Device '%s' has no medium.",
> + bdrv_get_device_name(bs));
> + ret = -ENOMEDIUM;
> + } else if (drv->bdrv_snapshot_goto) {
> + ret = drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
> + } else if (bs->file) {
> drv->bdrv_close(bs);
> - ret = bdrv_snapshot_goto(bs->file, snapshot_id);
> + ret = bdrv_snapshot_goto(bs->file, snapshot_id, errp);
> open_ret = drv->bdrv_open(bs, bs->open_flags);
> if (open_ret < 0) {
> bdrv_delete(bs->file);
> bs->drv = NULL;
> - return open_ret;
> + error_setg(errp, "Failed to open '%s'.", bdrv_get_device_name(bs));
> + ret = open_ret;
> }
> - return ret;
> + } else {
> + error_setg(errp, "Not supported.");
> + ret = -ENOTSUP;
> }
>
> - return -ENOTSUP;
> + return ret;
> }
>
> int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> diff --git a/block.h b/block.h
> index 6abb687..31642c2 100644
> --- a/block.h
> +++ b/block.h
> @@ -324,7 +324,8 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info,
> Error **errp);
> int bdrv_snapshot_goto(BlockDriverState *bs,
> - const char *snapshot_id);
> + const char *snapshot_id,
> + Error **errp);
> int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> int bdrv_snapshot_list(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_info);
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 3fd8aff..e07f9ac 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -428,7 +428,9 @@ fail:
> }
>
> /* copy the snapshot 'snapshot_name' into the current disk image */
> -int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_goto(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot *sn;
> @@ -440,13 +442,14 @@ 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) {
> + error_setg(errp, "Failed to find snapshot '%s' by id or name.",
> + snapshot_id);
> return -ENOENT;
> }
> 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");
> + error_setg(errp, "Not supported.");
> ret = -ENOTSUP;
> goto fail;
> }
> @@ -458,6 +461,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(errp, "Failed to grow L1 table.");
> goto fail;
> }
>
> @@ -476,18 +480,21 @@ 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(errp, "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(errp, "Failed to update snapshot refcount.");
> goto fail;
> }
>
> ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
> cur_l1_bytes);
> if (ret < 0) {
> + error_setg(errp, "Failed to save L1 table.");
> goto fail;
> }
>
> @@ -513,6 +520,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> }
>
> if (ret < 0) {
> + error_setg(errp, "Failed to update snapshot refcount.");
> goto fail;
> }
>
> @@ -525,6 +533,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(errp, "Failed to update snapshot refcount.");
> goto fail;
> }
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 854bd12..6babb56 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -311,7 +311,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
> int qcow2_snapshot_create(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info,
> Error **errp);
> -int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
> +int qcow2_snapshot_goto(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp);
> int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> 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 cb5acf8..3e789c6 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -858,12 +858,16 @@ static int qemu_rbd_snap_remove(BlockDriverState *bs,
> }
>
> static int qemu_rbd_snap_rollback(BlockDriverState *bs,
> - const char *snapshot_name)
> + const char *snapshot_name,
> + Error **errp)
> {
> BDRVRBDState *s = bs->opaque;
> int r;
>
> r = rbd_snap_rollback(s->image, snapshot_name);
> + if (r < 0) {
> + error_setg(errp, "Failed to rollback snapshot.");
> + }
> return r;
> }
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 094634a..34ef75c 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1808,7 +1808,9 @@ cleanup:
> return ret;
> }
>
> -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +static int sd_snapshot_goto(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> BDRVSheepdogState *s = bs->opaque;
> BDRVSheepdogState *old_s;
> @@ -1833,14 +1835,14 @@ 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(errp, "Failed to find VDI snapshot '%s'.", vdi);
> goto out;
> }
>
> fd = connect_to_sdog(s->addr, s->port);
> if (fd < 0) {
> - error_report("failed to connect");
> ret = fd;
> + error_setg(errp, "Failed to connect to sdog.");
> goto out;
> }
>
> @@ -1851,13 +1853,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> closesocket(fd);
>
> if (ret) {
> + error_setg(errp, "Failed to open '%s'.", vdi);
> goto out;
> }
>
> memcpy(&s->inode, buf, sizeof(s->inode));
>
> if (!s->inode.vm_state_size) {
> - error_report("Invalid snapshot");
> + error_setg(errp, "Invalid snapshot '%s'.", snapshot_id);
> ret = -ENOENT;
> goto out;
> }
> @@ -1873,9 +1876,6 @@ out:
> 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;
> }
>
> diff --git a/block_int.h b/block_int.h
> index 976ea1f..016782c 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -150,7 +150,8 @@ struct BlockDriver {
> QEMUSnapshotInfo *sn_info,
> Error **errp);
> int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> - const char *snapshot_id);
> + const char *snapshot_id,
> + Error **errp);
> int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
> int (*bdrv_snapshot_list)(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_info);
> diff --git a/qemu-img.c b/qemu-img.c
> index 93e4022..91671a8 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1564,11 +1564,7 @@ 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));
> - }
> + ret = bdrv_snapshot_goto(bs, snapshot_name, &err);
> break;
>
> case SNAPSHOT_DELETE:
> diff --git a/savevm.c b/savevm.c
> index ef2f305..29e41fc 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2301,7 +2301,7 @@ int load_vmstate(const char *name)
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs)) {
> - ret = bdrv_snapshot_goto(bs, name);
> + ret = bdrv_snapshot_goto(bs, name, NULL);
> if (ret < 0) {
> error_report("Error %d while activating snapshot '%s' on '%s'",
> ret, name, bdrv_get_device_name(bs));
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 04/17] block: add error parameter to bdrv_snapshot_delete() and related functions
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (2 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 03/17] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 05/17] block: add error parameter to bdrv_snapshot_list() " Pavel Hrdina
` (14 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 27 +++++++++++++++++++--------
block.h | 4 +++-
block/qcow2-snapshot.c | 10 +++++++++-
block/qcow2.h | 4 +++-
block/rbd.c | 6 +++++-
block/sheepdog.c | 4 +++-
block_int.h | 4 +++-
qemu-img.c | 6 +-----
savevm.c | 4 ++--
9 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/block.c b/block.c
index d012b0e..1ec4d24 100644
--- a/block.c
+++ b/block.c
@@ -3128,16 +3128,27 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret;
}
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int 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;
+ int ret;
+
+ if (!drv) {
+ error_setg(errp, "Device '%s' has no medium.",
+ bdrv_get_device_name(bs));
+ ret = -ENOMEDIUM;
+ } else if (drv->bdrv_snapshot_delete) {
+ ret = drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
+ } else if (bs->file) {
+ ret = bdrv_snapshot_delete(bs->file, snapshot_id, errp);
+ } else {
+ error_setg(errp, "Not supported.");
+ ret = -ENOTSUP;
+ }
+
+ return ret;
}
int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/block.h b/block.h
index 31642c2..b6b25a7 100644
--- a/block.h
+++ b/block.h
@@ -326,7 +326,9 @@ int bdrv_snapshot_create(BlockDriverState *bs,
int bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int 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/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e07f9ac..0bd0d1f 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -550,7 +550,9 @@ fail:
return ret;
}
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot sn;
@@ -559,6 +561,8 @@ 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) {
+ error_setg(errp, "Failed to find snapshot '%s' by id or name.",
+ snapshot_id);
return -ENOENT;
}
sn = s->snapshots[snapshot_index];
@@ -570,6 +574,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
s->nb_snapshots--;
ret = qcow2_write_snapshots(bs);
if (ret < 0) {
+ error_setg(errp, "Failed to remove snapshot '%s' from snapshot list",
+ snapshot_id);
return ret;
}
@@ -587,6 +593,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
sn.l1_size, -1);
if (ret < 0) {
+ error_setg(errp, "Failed to update snapshot refcount.");
return ret;
}
qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
@@ -594,6 +601,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
/* must update the copied flag on the current cluster offsets */
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
if (ret < 0) {
+ error_setg(errp, "Failed to update snapshot refcount.");
return ret;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 6babb56..8dfdbb7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -314,7 +314,9 @@ int qcow2_snapshot_create(BlockDriverState *bs,
int qcow2_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int 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 3e789c6..f8fe938 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -848,12 +848,16 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
}
static int qemu_rbd_snap_remove(BlockDriverState *bs,
- const char *snapshot_name)
+ const char *snapshot_name,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
r = rbd_snap_remove(s->image, snapshot_name);
+ if (r < 0) {
+ error_setg(errp, "Failed to remove snapshot.");
+ }
return r;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 34ef75c..c7f8d9e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1879,7 +1879,9 @@ out:
return ret;
}
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
{
/* FIXME: Delete specified snapshot id. */
return 0;
diff --git a/block_int.h b/block_int.h
index 016782c..779e3cb 100644
--- a/block_int.h
+++ b/block_int.h
@@ -152,7 +152,9 @@ struct BlockDriver {
int (*bdrv_snapshot_goto)(BlockDriverState *bs,
const char *snapshot_id,
Error **errp);
- int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
+ int (*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 91671a8..fd9b525 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1568,11 +1568,7 @@ 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));
- }
+ ret = bdrv_snapshot_delete(bs, snapshot_name, &err);
break;
}
diff --git a/savevm.c b/savevm.c
index 29e41fc..2aaa08b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2097,7 +2097,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0)
{
- ret = bdrv_snapshot_delete(bs, name);
+ ret = bdrv_snapshot_delete(bs, name, NULL);
if (ret < 0) {
monitor_printf(mon,
"Error while deleting snapshot on '%s'\n",
@@ -2344,7 +2344,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
bs1 = NULL;
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1)) {
- ret = bdrv_snapshot_delete(bs1, name);
+ ret = bdrv_snapshot_delete(bs1, name, NULL);
if (ret < 0) {
if (ret == -ENOTSUP)
monitor_printf(mon,
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 05/17] block: add error parameter to bdrv_snapshot_list() and related functions
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (3 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 04/17] block: add error parameter to bdrv_snapshot_delete() " Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 06/17] block: add error parameter to bdrv_snapshot_find() Pavel Hrdina
` (13 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 26 ++++++++++++++++++--------
block.h | 3 ++-
block/qcow2-snapshot.c | 5 ++++-
block/qcow2.h | 4 +++-
block/rbd.c | 4 +++-
block/sheepdog.c | 9 +++++++--
block_int.h | 3 ++-
qemu-img.c | 8 ++++++--
savevm.c | 4 ++--
9 files changed, 47 insertions(+), 19 deletions(-)
diff --git a/block.c b/block.c
index 1ec4d24..2983dda 100644
--- a/block.c
+++ b/block.c
@@ -3152,16 +3152,26 @@ int 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;
+ int ret;
+
+ if (!drv) {
+ error_setg(errp, "Device '%s' has no medium.",
+ bdrv_get_device_name(bs));
+ ret = -ENOMEDIUM;
+ } else if (drv->bdrv_snapshot_list) {
+ ret = drv->bdrv_snapshot_list(bs, psn_info, errp);
+ } else if (bs->file) {
+ ret = bdrv_snapshot_list(bs->file, psn_info, errp);
+ } else {
+ error_setg(errp, "Not supported.");
+ ret = -ENOTSUP;
+ }
+
+ return ret;
}
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/block.h b/block.h
index b6b25a7..ac28fff 100644
--- a/block.h
+++ b/block.h
@@ -330,7 +330,8 @@ int 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/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0bd0d1f..c834605 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -614,7 +614,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
return 0;
}
-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;
@@ -623,6 +625,7 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
if (!s->nb_snapshots) {
*psn_tab = NULL;
+ error_setg(errp, "No snapshot list find.");
return s->nb_snapshots;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 8dfdbb7..2fe5687 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -317,7 +317,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs,
int 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 f8fe938..d05e9d1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -876,7 +876,8 @@ static int 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;
@@ -893,6 +894,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
} while (snap_count == -ERANGE);
if (snap_count <= 0) {
+ error_setg(errp, "No snapshot list find.");
goto done;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index c7f8d9e..c2cd3ad 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1887,7 +1887,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,
return 0;
}
-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;
@@ -1905,6 +1907,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
fd = connect_to_sdog(s->addr, s->port);
if (fd < 0) {
+ error_setg(errp, "Failed to connect to sdog.");
ret = fd;
goto out;
}
@@ -1921,6 +1924,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
closesocket(fd);
if (ret) {
+ error_setg(errp, "No snapshot list find.");
goto out;
}
@@ -1932,7 +1936,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
fd = connect_to_sdog(s->addr, s->port);
if (fd < 0) {
- error_report("failed to connect");
+ error_setg(errp, "Failed to connect to sdog.");
ret = fd;
goto out;
}
@@ -1973,6 +1977,7 @@ out:
g_free(vdi_inuse);
if (ret < 0) {
+ error_setg(errp, "%s", strerror(errno));
return ret;
}
diff --git a/block_int.h b/block_int.h
index 779e3cb..cc60f51 100644
--- a/block_int.h
+++ b/block_int.h
@@ -156,7 +156,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 fd9b525..d1690d4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1099,8 +1099,10 @@ static void dump_snapshots(BlockDriverState *bs)
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i;
char buf[256];
+ Error *err = NULL;
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, &err);
+ handle_error(&err);
if (nb_sns <= 0)
return;
printf("Snapshot list:\n");
@@ -1134,7 +1136,9 @@ 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 *err = NULL;
+ sn_count = bdrv_snapshot_list(bs, &sn_tab, &err);
+ handle_error(&err);
for (i = 0; i < sn_count; i++) {
info->has_snapshots = true;
diff --git a/savevm.c b/savevm.c
index 2aaa08b..e57c108 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2068,7 +2068,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
int nb_sns, i, ret;
ret = -ENOENT;
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
if (nb_sns < 0)
return ret;
for(i = 0; i < nb_sns; i++) {
@@ -2373,7 +2373,7 @@ void do_info_snapshots(Monitor *mon)
return;
}
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
if (nb_sns < 0) {
monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
return;
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 06/17] block: add error parameter to bdrv_snapshot_find()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (4 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 05/17] block: add error parameter to bdrv_snapshot_list() " Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 07/17] block: add error parameter to del_existing_snapshots() Pavel Hrdina
` (12 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
savevm.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/savevm.c b/savevm.c
index e57c108..7ab7c7c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2061,16 +2061,21 @@ out:
return ret;
}
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name)
+static int bdrv_snapshot_find(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info,
+ const char *name,
+ Error **errp)
{
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i, ret;
+
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, errp);
+ if (error_is_set(errp)) {
+ return nb_sns;
+ }
+
ret = -ENOENT;
- nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
- if (nb_sns < 0)
- return ret;
for(i = 0; i < nb_sns; i++) {
sn = &sn_tab[i];
if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
@@ -2080,6 +2085,9 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
}
}
g_free(sn_tab);
+ if (ret < 0) {
+ error_setg(errp, "Failed to find snapshot '%s'.", name);
+ }
return ret;
}
@@ -2095,7 +2103,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) >= 0)
{
ret = bdrv_snapshot_delete(bs, name, NULL);
if (ret < 0) {
@@ -2166,7 +2174,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);
+ ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
if (ret >= 0) {
pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
@@ -2263,7 +2271,7 @@ int load_vmstate(const char *name)
}
/* Don't even try to load empty VM states */
- ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+ ret = bdrv_snapshot_find(bs_vm_state, &sn, name, NULL);
if (ret < 0) {
return ret;
} else if (sn.vm_state_size == 0) {
@@ -2287,7 +2295,7 @@ int load_vmstate(const char *name)
return -ENOTSUP;
}
- ret = bdrv_snapshot_find(bs, &sn, name);
+ ret = bdrv_snapshot_find(bs, &sn, name, NULL);
if (ret < 0) {
error_report("Device '%s' does not have the requested snapshot '%s'",
bdrv_get_device_name(bs), name);
@@ -2393,7 +2401,7 @@ void do_info_snapshots(Monitor *mon)
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
+ ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
if (ret < 0) {
available = 0;
break;
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 07/17] block: add error parameter to del_existing_snapshots()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (5 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 06/17] block: add error parameter to bdrv_snapshot_find() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 16:42 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 08/17] savevm: add error parameter to qemu_savevm_state_begin() Pavel Hrdina
` (11 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
savevm.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/savevm.c b/savevm.c
index 7ab7c7c..3ee7da5 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2094,22 +2094,18 @@ static int bdrv_snapshot_find(BlockDriverState *bs,
/*
* Deletes snapshots of a given name in all opened images.
*/
-static int del_existing_snapshots(Monitor *mon, const char *name)
+static int del_existing_snapshots(const char *name,
+ Error **errp)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
- int ret;
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name, NULL) >= 0)
{
- ret = bdrv_snapshot_delete(bs, name, NULL);
- if (ret < 0) {
- monitor_printf(mon,
- "Error while deleting snapshot on '%s'\n",
- bdrv_get_device_name(bs));
+ if (bdrv_snapshot_delete(bs, name, errp) < 0) {
return -1;
}
}
@@ -2194,7 +2190,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
}
/* Delete old snapshots of the same name */
- if (name && del_existing_snapshots(mon, name) < 0) {
+ if (name && del_existing_snapshots(name, NULL) < 0) {
goto the_end;
}
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/17] block: add error parameter to del_existing_snapshots()
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 07/17] block: add error parameter to del_existing_snapshots() Pavel Hrdina
@ 2012-12-14 16:42 ` Luiz Capitulino
0 siblings, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:42 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:41 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> savevm.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 7ab7c7c..3ee7da5 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2094,22 +2094,18 @@ static int bdrv_snapshot_find(BlockDriverState *bs,
> /*
> * Deletes snapshots of a given name in all opened images.
> */
> -static int del_existing_snapshots(Monitor *mon, const char *name)
> +static int del_existing_snapshots(const char *name,
> + Error **errp)
> {
> BlockDriverState *bs;
> QEMUSnapshotInfo sn1, *snapshot = &sn1;
> - int ret;
>
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs) &&
> bdrv_snapshot_find(bs, snapshot, name, NULL) >= 0)
> {
> - ret = bdrv_snapshot_delete(bs, name, NULL);
> - if (ret < 0) {
> - monitor_printf(mon,
> - "Error while deleting snapshot on '%s'\n",
> - bdrv_get_device_name(bs));
> + if (bdrv_snapshot_delete(bs, name, errp) < 0) {
> return -1;
> }
> }
> @@ -2194,7 +2190,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> }
>
> /* Delete old snapshots of the same name */
> - if (name && del_existing_snapshots(mon, name) < 0) {
> + if (name && del_existing_snapshots(name, NULL) < 0) {
Where do you print the error message returned by
del_existing_snapshots()? Actually, you seem to be leaking the
error object...
> goto the_end;
> }
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 08/17] savevm: add error parameter to qemu_savevm_state_begin()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (6 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 07/17] block: add error parameter to del_existing_snapshots() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 16:45 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 09/17] savevm: add error parameter to qemu_savevm_state_iterate() Pavel Hrdina
` (10 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
migration.c | 2 +-
savevm.c | 7 +++++--
sysemu.h | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/migration.c b/migration.c
index 73ce170..bf51a07 100644
--- a/migration.c
+++ b/migration.c
@@ -451,7 +451,7 @@ void migrate_fd_connect(MigrationState *s)
s->file = qemu_fopen_ops_buffered(s);
DPRINTF("beginning savevm\n");
- ret = qemu_savevm_state_begin(s->file, &s->params);
+ ret = qemu_savevm_state_begin(s->file, &s->params, NULL);
if (ret < 0) {
DPRINTF("failed, %d\n", ret);
migrate_fd_error(s);
diff --git a/savevm.c b/savevm.c
index 3ee7da5..633a697 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1601,7 +1601,8 @@ bool qemu_savevm_state_blocked(Error **errp)
}
int qemu_savevm_state_begin(QEMUFile *f,
- const MigrationParams *params)
+ const MigrationParams *params,
+ Error **errp)
{
SaveStateEntry *se;
int ret;
@@ -1641,12 +1642,14 @@ int qemu_savevm_state_begin(QEMUFile *f,
ret = se->ops->save_live_setup(f, se->opaque);
if (ret < 0) {
+ error_setg(errp, "Failed to begin vmstate save.");
qemu_savevm_state_cancel(f);
return ret;
}
}
ret = qemu_file_get_error(f);
if (ret != 0) {
+ error_setg(errp, "%s", strerror(errno));
qemu_savevm_state_cancel(f);
}
@@ -1783,7 +1786,7 @@ static int qemu_savevm_state(QEMUFile *f)
goto out;
}
- ret = qemu_savevm_state_begin(f, ¶ms);
+ ret = qemu_savevm_state_begin(f, ¶ms, NULL);
if (ret < 0)
goto out;
diff --git a/sysemu.h b/sysemu.h
index 1b6add2..07c5322 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -74,7 +74,8 @@ void qemu_announce_self(void);
bool qemu_savevm_state_blocked(Error **errp);
int qemu_savevm_state_begin(QEMUFile *f,
- const MigrationParams *params);
+ const MigrationParams *params,
+ Error **errp);
int qemu_savevm_state_iterate(QEMUFile *f);
int qemu_savevm_state_complete(QEMUFile *f);
void qemu_savevm_state_cancel(QEMUFile *f);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/17] savevm: add error parameter to qemu_savevm_state_begin()
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 08/17] savevm: add error parameter to qemu_savevm_state_begin() Pavel Hrdina
@ 2012-12-14 16:45 ` Luiz Capitulino
0 siblings, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:45 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:42 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> migration.c | 2 +-
> savevm.c | 7 +++++--
> sysemu.h | 3 ++-
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 73ce170..bf51a07 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -451,7 +451,7 @@ void migrate_fd_connect(MigrationState *s)
> s->file = qemu_fopen_ops_buffered(s);
>
> DPRINTF("beginning savevm\n");
> - ret = qemu_savevm_state_begin(s->file, &s->params);
> + ret = qemu_savevm_state_begin(s->file, &s->params, NULL);
> if (ret < 0) {
> DPRINTF("failed, %d\n", ret);
> migrate_fd_error(s);
> diff --git a/savevm.c b/savevm.c
> index 3ee7da5..633a697 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1601,7 +1601,8 @@ bool qemu_savevm_state_blocked(Error **errp)
> }
>
> int qemu_savevm_state_begin(QEMUFile *f,
> - const MigrationParams *params)
> + const MigrationParams *params,
> + Error **errp)
> {
> SaveStateEntry *se;
> int ret;
> @@ -1641,12 +1642,14 @@ int qemu_savevm_state_begin(QEMUFile *f,
>
> ret = se->ops->save_live_setup(f, se->opaque);
> if (ret < 0) {
> + error_setg(errp, "Failed to begin vmstate save.");
If possible, it would be nice to say why.
> qemu_savevm_state_cancel(f);
> return ret;
> }
> }
> ret = qemu_file_get_error(f);
> if (ret != 0) {
> + error_setg(errp, "%s", strerror(errno));
We have error_setg_errno().
> qemu_savevm_state_cancel(f);
> }
>
> @@ -1783,7 +1786,7 @@ static int qemu_savevm_state(QEMUFile *f)
> goto out;
> }
>
> - ret = qemu_savevm_state_begin(f, ¶ms);
> + ret = qemu_savevm_state_begin(f, ¶ms, NULL);
> if (ret < 0)
> goto out;
>
> diff --git a/sysemu.h b/sysemu.h
> index 1b6add2..07c5322 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -74,7 +74,8 @@ void qemu_announce_self(void);
>
> bool qemu_savevm_state_blocked(Error **errp);
> int qemu_savevm_state_begin(QEMUFile *f,
> - const MigrationParams *params);
> + const MigrationParams *params,
> + Error **errp);
> int qemu_savevm_state_iterate(QEMUFile *f);
> int qemu_savevm_state_complete(QEMUFile *f);
> void qemu_savevm_state_cancel(QEMUFile *f);
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 09/17] savevm: add error parameter to qemu_savevm_state_iterate()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (7 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 08/17] savevm: add error parameter to qemu_savevm_state_begin() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 10/17] savevm: add error parameter to qemu_savevm_state_complete() Pavel Hrdina
` (9 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
migration.c | 2 +-
savevm.c | 11 +++++++++--
sysemu.h | 3 ++-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/migration.c b/migration.c
index bf51a07..bbc0cac 100644
--- a/migration.c
+++ b/migration.c
@@ -341,7 +341,7 @@ void migrate_fd_put_ready(MigrationState *s)
}
DPRINTF("iterate\n");
- ret = qemu_savevm_state_iterate(s->file);
+ ret = qemu_savevm_state_iterate(s->file, NULL);
if (ret < 0) {
migrate_fd_error(s);
} else if (ret == 1) {
diff --git a/savevm.c b/savevm.c
index 633a697..26f12e1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1663,7 +1663,8 @@ int 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;
@@ -1697,10 +1698,16 @@ int qemu_savevm_state_iterate(QEMUFile *f)
}
}
if (ret != 0) {
+ if (ret < 0) {
+ error_setg(errp, "Failed to iterate vmstate save.");
+ }
return ret;
}
ret = qemu_file_get_error(f);
if (ret != 0) {
+ if (ret < 0) {
+ error_setg(errp, "%s", strerror(errno));
+ }
qemu_savevm_state_cancel(f);
}
return ret;
@@ -1791,7 +1798,7 @@ static int qemu_savevm_state(QEMUFile *f)
goto out;
do {
- ret = qemu_savevm_state_iterate(f);
+ ret = qemu_savevm_state_iterate(f, NULL);
if (ret < 0)
goto out;
} while (ret == 0);
diff --git a/sysemu.h b/sysemu.h
index 07c5322..d0530b2 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -76,7 +76,8 @@ bool qemu_savevm_state_blocked(Error **errp);
int qemu_savevm_state_begin(QEMUFile *f,
const MigrationParams *params,
Error **errp);
-int qemu_savevm_state_iterate(QEMUFile *f);
+int qemu_savevm_state_iterate(QEMUFile *f,
+ Error **errp);
int qemu_savevm_state_complete(QEMUFile *f);
void qemu_savevm_state_cancel(QEMUFile *f);
int qemu_loadvm_state(QEMUFile *f);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 10/17] savevm: add error parameter to qemu_savevm_state_complete()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (8 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 09/17] savevm: add error parameter to qemu_savevm_state_iterate() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 11/17] savevm: add error parameter to qemu_savevm_state() Pavel Hrdina
` (8 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
migration.c | 2 +-
savevm.c | 13 ++++++++++---
sysemu.h | 3 ++-
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/migration.c b/migration.c
index bbc0cac..3ae1db0 100644
--- a/migration.c
+++ b/migration.c
@@ -353,7 +353,7 @@ void migrate_fd_put_ready(MigrationState *s)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
- if (qemu_savevm_state_complete(s->file) < 0) {
+ if (qemu_savevm_state_complete(s->file, NULL) < 0) {
migrate_fd_error(s);
} else {
migrate_fd_completed(s);
diff --git a/savevm.c b/savevm.c
index 26f12e1..c17cc7f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1713,7 +1713,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,
return ret;
}
-int qemu_savevm_state_complete(QEMUFile *f)
+int qemu_savevm_state_complete(QEMUFile *f,
+ Error **errp)
{
SaveStateEntry *se;
int ret;
@@ -1737,6 +1738,7 @@ int qemu_savevm_state_complete(QEMUFile *f)
ret = se->ops->save_live_complete(f, se->opaque);
trace_savevm_section_end(se->section_id);
if (ret < 0) {
+ error_setg(errp, "Failed to complete vmstate save.");
return ret;
}
}
@@ -1766,7 +1768,12 @@ int qemu_savevm_state_complete(QEMUFile *f)
qemu_put_byte(f, QEMU_VM_EOF);
- return qemu_file_get_error(f);
+ ret = qemu_file_get_error(f);
+ if (ret < 0) {
+ error_setg(errp, "%s", strerror(errno));
+ }
+
+ return ret;
}
void qemu_savevm_state_cancel(QEMUFile *f)
@@ -1803,7 +1810,7 @@ static int qemu_savevm_state(QEMUFile *f)
goto out;
} while (ret == 0);
- ret = qemu_savevm_state_complete(f);
+ ret = qemu_savevm_state_complete(f, NULL);
out:
if (ret == 0) {
diff --git a/sysemu.h b/sysemu.h
index d0530b2..11a4560 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -78,7 +78,8 @@ int qemu_savevm_state_begin(QEMUFile *f,
Error **errp);
int qemu_savevm_state_iterate(QEMUFile *f,
Error **errp);
-int qemu_savevm_state_complete(QEMUFile *f);
+int qemu_savevm_state_complete(QEMUFile *f,
+ Error **errp);
void qemu_savevm_state_cancel(QEMUFile *f);
int qemu_loadvm_state(QEMUFile *f);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 11/17] savevm: add error parameter to qemu_savevm_state()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (9 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 10/17] savevm: add error parameter to qemu_savevm_state_complete() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
` (7 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
savevm.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/savevm.c b/savevm.c
index c17cc7f..71c7df8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1787,7 +1787,8 @@ void qemu_savevm_state_cancel(QEMUFile *f)
}
}
-static int qemu_savevm_state(QEMUFile *f)
+static int qemu_savevm_state(QEMUFile *f,
+ Error **errp)
{
int ret;
MigrationParams params = {
@@ -1795,22 +1796,24 @@ static int qemu_savevm_state(QEMUFile *f)
.shared = 0
};
- if (qemu_savevm_state_blocked(NULL)) {
+ if (qemu_savevm_state_blocked(errp)) {
ret = -EINVAL;
goto out;
}
- ret = qemu_savevm_state_begin(f, ¶ms, NULL);
- if (ret < 0)
+ ret = qemu_savevm_state_begin(f, ¶ms, errp);
+ if (ret < 0) {
goto out;
+ }
do {
- ret = qemu_savevm_state_iterate(f, NULL);
- if (ret < 0)
+ ret = qemu_savevm_state_iterate(f, errp);
+ if (ret < 0) {
goto out;
+ }
} while (ret == 0);
- ret = qemu_savevm_state_complete(f, NULL);
+ ret = qemu_savevm_state_complete(f, errp);
out:
if (ret == 0) {
@@ -2217,7 +2220,7 @@ 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);
+ ret = qemu_savevm_state(f, NULL);
vm_state_size = qemu_ftell(f);
qemu_fclose(f);
if (ret < 0) {
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state()
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (10 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 11/17] savevm: add error parameter to qemu_savevm_state() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 16:52 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm Pavel Hrdina
` (6 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
migration.c | 8 ++++----
savevm.c | 39 +++++++++++++++++++++++----------------
sysemu.h | 3 ++-
3 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/migration.c b/migration.c
index 3ae1db0..4c2d2d3 100644
--- a/migration.c
+++ b/migration.c
@@ -86,13 +86,13 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
static void process_incoming_migration_co(void *opaque)
{
QEMUFile *f = opaque;
- int ret;
+ Error **errp = NULL;
- ret = qemu_loadvm_state(f);
+ qemu_loadvm_state(f, errp);
qemu_set_fd_handler(qemu_get_fd(f), NULL, NULL, NULL);
qemu_fclose(f);
- if (ret < 0) {
- fprintf(stderr, "load of migration failed\n");
+ if (error_is_set(errp)) {
+ handle_error(errp);
exit(0);
}
qemu_announce_self();
diff --git a/savevm.c b/savevm.c
index 71c7df8..2c63aad 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1962,7 +1962,8 @@ typedef struct LoadStateEntry {
int version_id;
} LoadStateEntry;
-int qemu_loadvm_state(QEMUFile *f)
+int qemu_loadvm_state(QEMUFile *f,
+ Error **errp)
{
QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
QLIST_HEAD_INITIALIZER(loadvm_handlers);
@@ -1971,21 +1972,25 @@ 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 -ENOTSUP;
}
v = qemu_get_be32(f);
- if (v != QEMU_VM_FILE_MAGIC)
+ if (v != QEMU_VM_FILE_MAGIC) {
+ error_setg(errp, "Unknown vmstate file magic");
return -EINVAL;
+ }
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");
+ error_setg(errp, "Not supported.");
return -ENOTSUP;
}
- if (v != QEMU_VM_FILE_VERSION)
+ if (v != QEMU_VM_FILE_VERSION) {
+ error_setg(errp, "Not supported.");
return -ENOTSUP;
+ }
while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
uint32_t instance_id, version_id, section_id;
@@ -2007,15 +2012,16 @@ 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);
+ error_setg(errp, "Unknown savevm section or instance '%s' %d",
+ idstr, instance_id);
ret = -EINVAL;
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);
+ error_setg(errp, "savevm: unsupported version %d for '%s' v%d",
+ version_id, idstr, se->version_id);
ret = -EINVAL;
goto out;
}
@@ -2030,8 +2036,7 @@ 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, "Failed to load vmstate.");
goto out;
}
break;
@@ -2045,20 +2050,19 @@ int qemu_loadvm_state(QEMUFile *f)
}
}
if (le == NULL) {
- fprintf(stderr, "Unknown savevm section %d\n", section_id);
+ error_setg(errp, "Unknown savevm section %d", section_id);
ret = -EINVAL;
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",
- section_id);
+ error_setg(errp, "Failed to load vmstate.");
goto out;
}
break;
default:
- fprintf(stderr, "Unknown savevm section type %d\n", section_type);
+ error_setg(errp, "Unknown savevm section type %d", section_type);
ret = -EINVAL;
goto out;
}
@@ -2076,6 +2080,9 @@ out:
if (ret == 0) {
ret = qemu_file_get_error(f);
+ if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
}
return ret;
@@ -2342,7 +2349,7 @@ int load_vmstate(const char *name)
}
qemu_system_reset(VMRESET_SILENT);
- ret = qemu_loadvm_state(f);
+ ret = qemu_loadvm_state(f, NULL);
qemu_fclose(f);
if (ret < 0) {
diff --git a/sysemu.h b/sysemu.h
index 11a4560..c08f7a3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -81,7 +81,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,
int qemu_savevm_state_complete(QEMUFile *f,
Error **errp);
void qemu_savevm_state_cancel(QEMUFile *f);
-int qemu_loadvm_state(QEMUFile *f);
+int qemu_loadvm_state(QEMUFile *f,
+ Error **errp);
/* SLIRP */
void do_info_slirp(Monitor *mon);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state()
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
@ 2012-12-14 16:52 ` Luiz Capitulino
0 siblings, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:52 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:46 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> migration.c | 8 ++++----
> savevm.c | 39 +++++++++++++++++++++++----------------
> sysemu.h | 3 ++-
> 3 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 3ae1db0..4c2d2d3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -86,13 +86,13 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
> static void process_incoming_migration_co(void *opaque)
> {
> QEMUFile *f = opaque;
> - int ret;
> + Error **errp = NULL;
>
> - ret = qemu_loadvm_state(f);
> + qemu_loadvm_state(f, errp);
> qemu_set_fd_handler(qemu_get_fd(f), NULL, NULL, NULL);
> qemu_fclose(f);
> - if (ret < 0) {
> - fprintf(stderr, "load of migration failed\n");
> + if (error_is_set(errp)) {
> + handle_error(errp);
You can just print to stderr here, as qemu is going to exit.
> exit(0);
> }
> qemu_announce_self();
> diff --git a/savevm.c b/savevm.c
> index 71c7df8..2c63aad 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1962,7 +1962,8 @@ typedef struct LoadStateEntry {
> int version_id;
> } LoadStateEntry;
>
> -int qemu_loadvm_state(QEMUFile *f)
> +int qemu_loadvm_state(QEMUFile *f,
> + Error **errp)
> {
> QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
> QLIST_HEAD_INITIALIZER(loadvm_handlers);
> @@ -1971,21 +1972,25 @@ 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 -ENOTSUP;
Why?
> }
>
> v = qemu_get_be32(f);
> - if (v != QEMU_VM_FILE_MAGIC)
> + if (v != QEMU_VM_FILE_MAGIC) {
> + error_setg(errp, "Unknown vmstate file magic");
> return -EINVAL;
> + }
>
> 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");
> + error_setg(errp, "Not supported.");
Will this new error message reach the terminal as fprintf() does? Also,
please keep the good error message.
> return -ENOTSUP;
> }
> - if (v != QEMU_VM_FILE_VERSION)
> + if (v != QEMU_VM_FILE_VERSION) {
> + error_setg(errp, "Not supported.");
Please, improve the error message.
> return -ENOTSUP;
> + }
>
> while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
> uint32_t instance_id, version_id, section_id;
> @@ -2007,15 +2012,16 @@ 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);
> + error_setg(errp, "Unknown savevm section or instance '%s' %d",
> + idstr, instance_id);
> ret = -EINVAL;
> 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);
> + error_setg(errp, "savevm: unsupported version %d for '%s' v%d",
> + version_id, idstr, se->version_id);
> ret = -EINVAL;
> goto out;
> }
> @@ -2030,8 +2036,7 @@ 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, "Failed to load vmstate.");
> goto out;
> }
> break;
> @@ -2045,20 +2050,19 @@ int qemu_loadvm_state(QEMUFile *f)
> }
> }
> if (le == NULL) {
> - fprintf(stderr, "Unknown savevm section %d\n", section_id);
> + error_setg(errp, "Unknown savevm section %d", section_id);
> ret = -EINVAL;
> 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",
> - section_id);
> + error_setg(errp, "Failed to load vmstate.");
> goto out;
> }
> break;
> default:
> - fprintf(stderr, "Unknown savevm section type %d\n", section_type);
> + error_setg(errp, "Unknown savevm section type %d", section_type);
> ret = -EINVAL;
> goto out;
> }
> @@ -2076,6 +2080,9 @@ out:
>
> if (ret == 0) {
> ret = qemu_file_get_error(f);
> + if (ret < 0) {
> + error_set(errp, QERR_GENERIC_ERROR, ret);
No reason to sue QERR_ macros here.
> + }
> }
>
> return ret;
> @@ -2342,7 +2349,7 @@ int load_vmstate(const char *name)
> }
>
> qemu_system_reset(VMRESET_SILENT);
> - ret = qemu_loadvm_state(f);
> + ret = qemu_loadvm_state(f, NULL);
>
> qemu_fclose(f);
> if (ret < 0) {
> diff --git a/sysemu.h b/sysemu.h
> index 11a4560..c08f7a3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -81,7 +81,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,
> int qemu_savevm_state_complete(QEMUFile *f,
> Error **errp);
> void qemu_savevm_state_cancel(QEMUFile *f);
> -int qemu_loadvm_state(QEMUFile *f);
> +int qemu_loadvm_state(QEMUFile *f,
> + Error **errp);
>
> /* SLIRP */
> void do_info_slirp(Monitor *mon);
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (11 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 16:57 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 14/17] qapi: Convert loadvm Pavel Hrdina
` (5 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 4 ++--
hmp.c | 9 +++++++++
hmp.h | 1 +
qapi-schema.json | 19 +++++++++++++++++++
qmp-commands.hx | 29 +++++++++++++++++++++++++++++
savevm.c | 26 ++++++++++----------------
sysemu.h | 1 -
7 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..3b781f7 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -310,7 +310,7 @@ ETEXI
.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,
+ .mhandler.cmd = hmp_vm_snapshot_save,
},
STEXI
@@ -318,7 +318,7 @@ STEXI
@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
+a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at
@ref{vm_snapshots}.
ETEXI
diff --git a/hmp.c b/hmp.c
index 180ba2b..2dbdcbf 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1335,3 +1335,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
qmp_nbd_server_stop(&errp);
hmp_handle_error(mon, &errp);
}
+
+void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
+{
+ const char *name = qdict_get_try_str(qdict, "name");
+ Error *err = NULL;
+
+ qmp_vm_snapshot_save(!!name, name, &err);
+ hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 0ab03be..0bd7e47 100644
--- a/hmp.h
+++ b/hmp.h
@@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..08543d2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3017,3 +3017,22 @@
# Since: 1.3.0
##
{ 'command': 'nbd-server-stop' }
+
+##
+# @vm-snapshot-save:
+#
+# Create a snapshot of the whole virtual machine. If tag is provided as @name,
+# it is used as human readable identifier. If there is already a snapshot
+# with the same tag or ID, it is replaced.
+#
+# The VM is automatically stopped and resumed and saving a snapshot can take
+# a long time.
+#
+# @name: #optional tag of new snapshot or tag|id of existing snapshot
+#
+# Returns: Nothing on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.4.0
+##
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..58e2d0a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1348,6 +1348,35 @@ Example:
EQMP
{
+ .name = "vm-snapshot-save",
+ .args_type = "name:s?",
+ .params = "name",
+ .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+ .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
+ },
+
+SQMP
+vm-snapshot-save
+------
+
+Create a snapshot of the whole virtual machine. If tag is provided as name,
+it is used as human readable identifier. If there is already a snapshot
+with the same tag or id, it is replaced.
+
+The VM is automatically stopped and resumed and saving a snapshot can take
+a long time.
+
+Arguments:
+
+- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
+<- { "return": {} }
+
+EQMP
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/savevm.c b/savevm.c
index 2c63aad..91120c4 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,
return 0;
}
-void do_savevm(Monitor *mon, const QDict *qdict)
+void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2156,7 +2156,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
struct timeval tv;
struct tm tm;
#endif
- const char *name = qdict_get_try_str(qdict, "name");
/* Verify if there is a device that doesn't support snapshots and is writable */
bs = NULL;
@@ -2167,15 +2166,15 @@ 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));
+ error_setg(errp, "Device '%s' is writable but does not support "
+ "snapshots.", bdrv_get_device_name(bs));
return;
}
}
bs = bdrv_snapshots();
if (!bs) {
- monitor_printf(mon, "No block device can accept snapshots\n");
+ error_setg(errp, "No block device can accept snapshots.");
return;
}
@@ -2196,7 +2195,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
#endif
sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
- if (name) {
+ if (has_name) {
ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
if (ret >= 0) {
pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
@@ -2217,21 +2216,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
}
/* Delete old snapshots of the same name */
- if (name && del_existing_snapshots(name, NULL) < 0) {
+ if (has_name && del_existing_snapshots(name, errp) < 0) {
goto the_end;
}
/* 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;
}
- ret = qemu_savevm_state(f, NULL);
+ ret = qemu_savevm_state(f, errp);
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(errp)) {
goto the_end;
}
@@ -2242,11 +2240,7 @@ 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, NULL);
- if (ret < 0) {
- monitor_printf(mon, "Error while creating snapshot on '%s'\n",
- bdrv_get_device_name(bs1));
- }
+ bdrv_snapshot_create(bs1, sn, errp);
}
}
diff --git a/sysemu.h b/sysemu.h
index c08f7a3..96b4879 100644
--- a/sysemu.h
+++ b/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);
int load_vmstate(const char *name);
void do_delvm(Monitor *mon, const QDict *qdict);
void do_info_snapshots(Monitor *mon);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm Pavel Hrdina
@ 2012-12-14 16:57 ` Luiz Capitulino
2012-12-14 17:09 ` Eric Blake
0 siblings, 1 reply; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 16:57 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:47 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> hmp-commands.hx | 4 ++--
> hmp.c | 9 +++++++++
> hmp.h | 1 +
> qapi-schema.json | 19 +++++++++++++++++++
> qmp-commands.hx | 29 +++++++++++++++++++++++++++++
> savevm.c | 26 ++++++++++----------------
> sysemu.h | 1 -
> 7 files changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..3b781f7 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -310,7 +310,7 @@ ETEXI
> .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,
> + .mhandler.cmd = hmp_vm_snapshot_save,
> },
>
> STEXI
> @@ -318,7 +318,7 @@ STEXI
> @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
> +a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at
> @ref{vm_snapshots}.
> ETEXI
>
> diff --git a/hmp.c b/hmp.c
> index 180ba2b..2dbdcbf 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1335,3 +1335,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> qmp_nbd_server_stop(&errp);
> hmp_handle_error(mon, &errp);
> }
> +
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> +{
> + const char *name = qdict_get_try_str(qdict, "name");
> + Error *err = NULL;
> +
> + qmp_vm_snapshot_save(!!name, name, &err);
> + hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 0ab03be..0bd7e47 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
> void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
> void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5dfa052..08543d2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3017,3 +3017,22 @@
> # Since: 1.3.0
> ##
> { 'command': 'nbd-server-stop' }
> +
> +##
> +# @vm-snapshot-save:
> +#
> +# Create a snapshot of the whole virtual machine. If tag is provided as @name,
> +# it is used as human readable identifier. If there is already a snapshot
> +# with the same tag or ID, it is replaced.
> +#
> +# The VM is automatically stopped and resumed and saving a snapshot can take
> +# a long time.
> +#
> +# @name: #optional tag of new snapshot or tag|id of existing snapshot
I wonder if we should allow 'name' to be optional in QMP.
> +#
> +# Returns: Nothing on success
> +# If an error occurs, GenericError with error message
You can drop this last line.
> +#
> +# Since: 1.4.0
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5c692d0..58e2d0a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1348,6 +1348,35 @@ Example:
>
> EQMP
> {
> + .name = "vm-snapshot-save",
> + .args_type = "name:s?",
> + .params = "name",
> + .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
> + },
> +
> +SQMP
> +vm-snapshot-save
> +------
> +
> +Create a snapshot of the whole virtual machine. If tag is provided as name,
> +it is used as human readable identifier. If there is already a snapshot
> +with the same tag or id, it is replaced.
> +
> +The VM is automatically stopped and resumed and saving a snapshot can take
> +a long time.
> +
> +Arguments:
> +
> +- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
> +<- { "return": {} }
> +
> +EQMP
> + {
> .name = "qmp_capabilities",
> .args_type = "",
> .params = "",
> diff --git a/savevm.c b/savevm.c
> index 2c63aad..91120c4 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,
> return 0;
> }
>
> -void do_savevm(Monitor *mon, const QDict *qdict)
> +void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
> {
> BlockDriverState *bs, *bs1;
> QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2156,7 +2156,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> struct timeval tv;
> struct tm tm;
> #endif
> - const char *name = qdict_get_try_str(qdict, "name");
>
> /* Verify if there is a device that doesn't support snapshots and is writable */
> bs = NULL;
> @@ -2167,15 +2166,15 @@ 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));
> + error_setg(errp, "Device '%s' is writable but does not support "
> + "snapshots.", bdrv_get_device_name(bs));
> return;
> }
> }
>
> bs = bdrv_snapshots();
> if (!bs) {
> - monitor_printf(mon, "No block device can accept snapshots\n");
> + error_setg(errp, "No block device can accept snapshots.");
> return;
> }
>
> @@ -2196,7 +2195,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> #endif
> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>
> - if (name) {
> + if (has_name) {
> ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
> if (ret >= 0) {
> pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> @@ -2217,21 +2216,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> }
>
> /* Delete old snapshots of the same name */
> - if (name && del_existing_snapshots(name, NULL) < 0) {
> + if (has_name && del_existing_snapshots(name, errp) < 0) {
You shouldn't use errp (the argument passed to qmp_vm_snapshot_save) to
detect internal error. You should have Error *local_err, pass it instead
and propagate errors with error_propagate().
> goto the_end;
> }
>
> /* 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;
> }
> - ret = qemu_savevm_state(f, NULL);
> + ret = qemu_savevm_state(f, errp);
> 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(errp)) {
> goto the_end;
> }
>
> @@ -2242,11 +2240,7 @@ 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, NULL);
> - if (ret < 0) {
> - monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> - bdrv_get_device_name(bs1));
> - }
> + bdrv_snapshot_create(bs1, sn, errp);
> }
> }
>
> diff --git a/sysemu.h b/sysemu.h
> index c08f7a3..96b4879 100644
> --- a/sysemu.h
> +++ b/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);
> int load_vmstate(const char *name);
> void do_delvm(Monitor *mon, const QDict *qdict);
> void do_info_snapshots(Monitor *mon);
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm
2012-12-14 16:57 ` Luiz Capitulino
@ 2012-12-14 17:09 ` Eric Blake
0 siblings, 0 replies; 33+ messages in thread
From: Eric Blake @ 2012-12-14 17:09 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Pavel Hrdina, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]
On 12/14/2012 09:57 AM, Luiz Capitulino wrote:
>> +##
>> +# @vm-snapshot-save:
>> +#
>> +# Create a snapshot of the whole virtual machine. If tag is provided as @name,
>> +# it is used as human readable identifier. If there is already a snapshot
>> +# with the same tag or ID, it is replaced.
>> +#
>> +# The VM is automatically stopped and resumed and saving a snapshot can take
>> +# a long time.
>> +#
>> +# @name: #optional tag of new snapshot or tag|id of existing snapshot
>
> I wonder if we should allow 'name' to be optional in QMP.
Libvirt will always pass a 'name'. Where it gets confusing is that
'name' can be all digits; so if I say a name of '3', but it gets id '2',
then future operations get way confusing (especially if later we also
create a snapshot whose id becomes 3).
>
>> +#
>> +# Returns: Nothing on success
One thing is for certain - if 'name' remains optional, then you MUST
return the 'id' that was auto-allocated. And even if name is not
optional, returning the 'id' that was either auto-allocated or
successfully looked up as an existing snapshot would be more useful than
returning nothing.
--
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: 619 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 14/17] qapi: Convert loadvm
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (12 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 18:30 ` Eric Blake
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 15/17] qapi: Convert delvm Pavel Hrdina
` (4 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 4 ++--
hmp.c | 9 ++++++++
hmp.h | 1 +
monitor.c | 12 -----------
qapi-schema.json | 15 +++++++++++++
qmp-commands.hx | 25 ++++++++++++++++++++++
savevm.c | 65 ++++++++++++++++++++++++++++----------------------------
sysemu.h | 1 -
vl.c | 6 +++++-
9 files changed, 90 insertions(+), 48 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3b781f7..5194f15 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -327,14 +327,14 @@ ETEXI
.args_type = "name:s",
.params = "tag|id",
.help = "restore a VM snapshot from its tag or id",
- .mhandler.cmd = do_loadvm,
+ .mhandler.cmd = hmp_vm_snapshot_load,
},
STEXI
@item loadvm @var{tag}|@var{id}
@findex loadvm
Set the whole virtual machine to the snapshot identified by the tag
-@var{tag} or the unique snapshot ID @var{id}.
+@var{tag} or the unique snapshot ID @var{id}. It must be snapshot of whole VM.
ETEXI
{
diff --git a/hmp.c b/hmp.c
index 2dbdcbf..29f60a8 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1344,3 +1344,12 @@ void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
qmp_vm_snapshot_save(!!name, name, &err);
hmp_handle_error(mon, &err);
}
+
+void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
+{
+ const char *name = qdict_get_str(qdict, "name");
+ Error *err = NULL;
+
+ qmp_vm_snapshot_load(name, &err);
+ hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 0bd7e47..3f61b32 100644
--- a/hmp.h
+++ b/hmp.h
@@ -81,5 +81,6 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict);
#endif
diff --git a/monitor.c b/monitor.c
index c0e32d6..b8abff2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2065,18 +2065,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 08543d2..f8ab4d9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3036,3 +3036,18 @@
# Since: 1.4.0
##
{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
+
+##
+# @vm-snapshot-load:
+#
+# Set the whole virtual machine to the snapshot identified by the tag
+# or the unique snapshot id. It must be snapshot of whole VM.
+#
+# @name: tag|id of existing snapshot
+#
+# Returns: Nothing on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.3
+##
+{ 'command': 'vm-snapshot-load', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 58e2d0a..62da843 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1377,6 +1377,31 @@ Example:
EQMP
{
+ .name = "vm-snapshot-load",
+ .args_type = "name:s",
+ .params = "name",
+ .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. It must be snapshot of whole VM.
+
+Arguments:
+
+- "name": tag|id of existing snapshot (json-string)
+
+Example:
+
+-> { "execute": "vm-snapshot-load", "arguments": { "name": "my_snapshot" } }
+<- { "return": {} }
+
+EQMP
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/savevm.c b/savevm.c
index 91120c4..636cca8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2081,7 +2081,7 @@ out:
if (ret == 0) {
ret = qemu_file_get_error(f);
if (ret < 0) {
- error_set(errp, QERR_GENERIC_ERROR, ret);
+ error_setg(errp, "%s", strerror(errno));
}
}
@@ -2274,27 +2274,28 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
vm_start();
}
-int load_vmstate(const char *name)
+void qmp_vm_snapshot_load(const char *name, Error **errp)
{
BlockDriverState *bs, *bs_vm_state;
QEMUSnapshotInfo sn;
QEMUFile *f;
- int ret;
+ int ret, saved_vm_running;
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;
}
/* Don't even try to load empty VM states */
- ret = bdrv_snapshot_find(bs_vm_state, &sn, name, NULL);
- if (ret < 0) {
- return ret;
- } else if (sn.vm_state_size == 0) {
- error_report("This is a disk-only snapshot. Revert to it offline "
- "using qemu-img.");
- return -EINVAL;
+ bdrv_snapshot_find(bs_vm_state, &sn, name, errp);
+ if (error_is_set(errp)) {
+ return;
+ }
+ if (sn.vm_state_size == 0) {
+ error_setg(errp, "This is a disk-only snapshot. Revert to it offline "
+ "using qemu-img.");
+ return;
}
/* Verify if there is any device that doesn't support snapshots and is
@@ -2307,30 +2308,29 @@ 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;
}
- ret = bdrv_snapshot_find(bs, &sn, name, NULL);
- if (ret < 0) {
- error_report("Device '%s' does not have the requested snapshot '%s'",
- bdrv_get_device_name(bs), name);
- return ret;
+ bdrv_snapshot_find(bs, &sn, name, errp);
+ if (error_is_set(errp)) {
+ return;
}
}
+ 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)) {
- ret = bdrv_snapshot_goto(bs, name, NULL);
- if (ret < 0) {
- error_report("Error %d while activating snapshot '%s' on '%s'",
- ret, name, bdrv_get_device_name(bs));
- return ret;
+ ret = bdrv_snapshot_goto(bs, name, errp);
+ if (error_is_set(errp)) {
+ return;
}
}
}
@@ -2338,20 +2338,21 @@ 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, "Failed to open '%s' file.", bdrv_get_device_name(bs));
+ return;
}
qemu_system_reset(VMRESET_SILENT);
- ret = qemu_loadvm_state(f, NULL);
+ qemu_loadvm_state(f, errp);
qemu_fclose(f);
- if (ret < 0) {
- error_report("Error %d while loading VM state", ret);
- return ret;
+ if (error_is_set(errp)) {
+ return;
}
- return 0;
+ if (saved_vm_running) {
+ vm_start();
+ }
}
void do_delvm(Monitor *mon, const QDict *qdict)
diff --git a/sysemu.h b/sysemu.h
index 96b4879..83fc17e 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
void qemu_add_machine_init_done_notifier(Notifier *notify);
-int load_vmstate(const char *name);
void do_delvm(Monitor *mon, const QDict *qdict);
void do_info_snapshots(Monitor *mon);
diff --git a/vl.c b/vl.c
index a3ab384..4774092 100644
--- a/vl.c
+++ b/vl.c
@@ -3976,8 +3976,12 @@ int main(int argc, char **argv, char **envp)
qemu_system_reset(VMRESET_SILENT);
if (loadvm) {
- if (load_vmstate(loadvm) < 0) {
+ Error *errp = NULL;
+ qmp_vm_snapshot_load(loadvm, &errp);
+
+ if (error_is_set(&errp)) {
autostart = 0;
+ handle_error(&errp);
}
}
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 15/17] qapi: Convert delvm
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (13 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 14/17] qapi: Convert loadvm Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 18:39 ` Eric Blake
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 16/17] qapi: Convert info snapshots Pavel Hrdina
` (3 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 2 +-
hmp.c | 10 ++++++++++
hmp.h | 1 +
qapi-schema.json | 14 ++++++++++++++
qmp-commands.hx | 24 ++++++++++++++++++++++++
savevm.c | 17 +++--------------
sysemu.h | 1 -
7 files changed, 53 insertions(+), 16 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 5194f15..0b3d783 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -342,7 +342,7 @@ ETEXI
.args_type = "name:s",
.params = "tag|id",
.help = "delete a VM snapshot from its tag or id",
- .mhandler.cmd = do_delvm,
+ .mhandler.cmd = hmp_vm_snapshot_delete,
},
STEXI
diff --git a/hmp.c b/hmp.c
index 29f60a8..78c9a7c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1353,3 +1353,13 @@ void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
qmp_vm_snapshot_load(name, &err);
hmp_handle_error(mon, &err);
}
+
+void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict)
+{
+ const char *name = qdict_get_str(qdict, "name");
+ Error *err = NULL;
+
+ qmp_vm_snapshot_delete(name, &err);
+
+ hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 3f61b32..8f929af 100644
--- a/hmp.h
+++ b/hmp.h
@@ -82,5 +82,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict);
#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index f8ab4d9..431df69 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3051,3 +3051,17 @@
# Since: 1.3
##
{ 'command': 'vm-snapshot-load', 'data': {'name': 'str'} }
+
+##
+# @vm-snapshot-delete:
+#
+# Delete the snapshot identified by tag or id.
+#
+# @name: tag|id of existing snapshot
+#
+# Returns: Nothing on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.3
+##
+{ 'command': 'vm-snapshot-delete', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 62da843..c3671f7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1402,6 +1402,30 @@ Example:
EQMP
{
+ .name = "vm-snapshot-delete",
+ .args_type = "name: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 the snapshot identified by tag or id.
+
+Arguments:
+
+- "name": tag|id of existing snapshot (json-string)
+
+Example:
+
+-> { "execute": "vm-snapshot-delete", "arguments": { "name": "my_snapshot" } }
+<- { "return": {} }
+
+EQMP
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/savevm.c b/savevm.c
index 636cca8..8da888e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2355,31 +2355,20 @@ void qmp_vm_snapshot_load(const char *name, Error **errp)
}
}
-void do_delvm(Monitor *mon, const QDict *qdict)
+void qmp_vm_snapshot_delete(const char *name, Error **errp)
{
BlockDriverState *bs, *bs1;
- int ret;
- const char *name = qdict_get_str(qdict, "name");
bs = bdrv_snapshots();
if (!bs) {
- monitor_printf(mon, "No block device supports snapshots\n");
+ error_setg(errp, "No block device supports snapshots.");
return;
}
bs1 = NULL;
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1)) {
- ret = bdrv_snapshot_delete(bs1, name, NULL);
- 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, errp);
}
}
}
diff --git a/sysemu.h b/sysemu.h
index 83fc17e..e440dcc 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
void qemu_add_machine_init_done_notifier(Notifier *notify);
-void do_delvm(Monitor *mon, const QDict *qdict);
void do_info_snapshots(Monitor *mon);
void qemu_announce_self(void);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v2 16/17] qapi: Convert info snapshots
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (14 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 15/17] qapi: Convert delvm Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 17:18 ` Luiz Capitulino
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 17/17] vm-snapshot-save: add force parameter Pavel Hrdina
` (2 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp.c | 33 +++++++++++++++++++++++++++++++++
hmp.h | 1 +
monitor.c | 2 +-
qapi-schema.json | 13 +++++++++++++
qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++
savevm.c | 52 +++++++++++++++++++++++++---------------------------
sysemu.h | 2 --
7 files changed, 107 insertions(+), 30 deletions(-)
diff --git a/hmp.c b/hmp.c
index 78c9a7c..1983210 100644
--- a/hmp.c
+++ b/hmp.c
@@ -628,6 +628,39 @@ void hmp_info_block_jobs(Monitor *mon)
}
}
+void hmp_info_snapshots(Monitor *mon)
+{
+ SnapshotInfoList *list;
+ Error *err = NULL;
+ char buf[256];
+ QEMUSnapshotInfo sn;
+
+ list = qmp_query_vm_snapshots(&err);
+
+ if (error_is_set(&err)) {
+ hmp_handle_error(mon, &err);
+ return;
+ }
+
+ if (!list) {
+ monitor_printf(mon, "There is no snapshot available.\n");
+ return;
+ }
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ while (list) {
+ memcpy(&(sn.id_str), list->value->id, sizeof(sn.id_str));
+ memcpy(&(sn.name), list->value->name, sizeof(sn.name));
+ sn.date_sec = list->value->date_sec;
+ sn.date_nsec = list->value->date_nsec;
+ sn.vm_clock_nsec = list->value->vm_clock_nsec;
+ sn.vm_state_size = list->value->vm_state_size;
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ list = list->next;
+ }
+
+ qapi_free_SnapshotInfoList(list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 8f929af..1c21a82 100644
--- a/hmp.h
+++ b/hmp.h
@@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon);
void hmp_info_balloon(Monitor *mon);
void hmp_info_pci(Monitor *mon);
void hmp_info_block_jobs(Monitor *mon);
+void hmp_info_snapshots(Monitor *mon);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index b8abff2..2be93c0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2596,7 +2596,7 @@ static mon_cmd_t info_cmds[] = {
.args_type = "",
.params = "",
.help = "show the currently saved VM snapshots",
- .mhandler.info = do_info_snapshots,
+ .mhandler.info = hmp_info_snapshots,
},
{
.name = "status",
diff --git a/qapi-schema.json b/qapi-schema.json
index 431df69..a8742a1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1224,6 +1224,19 @@
{ 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
##
+# @query-vm-snapshots:
+#
+# List available snapshots for VM.
+#
+# Returns: an array of @SnapshotInfo describing each snapshot or an empty array
+# on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.3
+##
+{ 'command': 'query-vm-snapshots', 'returns': ['SnapshotInfo'] }
+
+##
# @quit:
#
# This command will cause the QEMU process to exit gracefully. While every
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c3671f7..f839c40 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2732,3 +2732,37 @@ EQMP
.args_type = "",
.mhandler.cmd_new = qmp_marshal_input_query_target,
},
+
+SQMP
+query-vm-snapshots
+----------
+
+List available snapshots for VM.
+
+Return an array of json-object, each with the following information:
+
+- "id": unique snapshot id
+
+- "name": user choosen name
+
+- "vm-state-size": size of the VM state
+
+- "date-sec": UTC date of the snapshot
+
+- "date-nsec": date in nano seconds
+
+- "vm-clock-nsec": VM clock relative to boot in nano seconds
+
+Example:
+
+-> { "execute": "query-vm-snapshots" }
+<- {"return": [{"vm-clock-nsec": 36420253254, "name": "my_snapshot",
+ "date-sec": 1345120008, "date-nsec": 151984000, "id": "6",
+ "vm-state-size": 166515500}]}
+
+EQMP
+ {
+ .name = "query-vm-snapshots",
+ .args_type = "",
+ .mhandler.cmd_new = qmp_marshal_input_query_vm_snapshots,
+ },
diff --git a/savevm.c b/savevm.c
index 8da888e..36ccc46 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2373,34 +2373,28 @@ void qmp_vm_snapshot_delete(const char *name, Error **errp)
}
}
-void do_info_snapshots(Monitor *mon)
+SnapshotInfoList *qmp_query_vm_snapshots(Error **errp)
{
+ SnapshotInfoList *snapshot_list = NULL, *last = NULL;
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
int nb_sns, i, ret, available;
- int total;
- int *available_snapshots;
- char buf[256];
bs = bdrv_snapshots();
if (!bs) {
- monitor_printf(mon, "No available block device supports snapshots\n");
- return;
+ error_setg(errp, "No block device supports snapshots.");
+ return NULL;
}
- nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
- if (nb_sns < 0) {
- monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
- return;
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, errp);
+ if (error_is_set(errp)) {
+ return NULL;
}
if (nb_sns == 0) {
- monitor_printf(mon, "There is no snapshot available.\n");
- return;
+ return NULL;
}
- available_snapshots = g_malloc0(sizeof(int) * nb_sns);
- total = 0;
for (i = 0; i < nb_sns; i++) {
sn = &sn_tab[i];
available = 1;
@@ -2417,24 +2411,28 @@ void do_info_snapshots(Monitor *mon)
}
if (available) {
- available_snapshots[total] = i;
- total++;
- }
- }
-
- if (total > 0) {
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
- for (i = 0; i < total; i++) {
- sn = &sn_tab[available_snapshots[i]];
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ SnapshotInfoList *info = g_malloc0(sizeof(*info));
+ info->value = g_malloc0(sizeof(*info->value));
+ info->value->id = g_strdup(sn->id_str);
+ info->value->name = g_strdup(sn->name);
+ info->value->vm_state_size = sn->vm_state_size;
+ info->value->date_sec = sn->date_sec;
+ info->value->date_nsec = sn->date_nsec;
+ info->value->vm_clock_nsec = sn->vm_clock_nsec;
+
+ if (!snapshot_list) {
+ snapshot_list = info;
+ last = info;
+ } else {
+ last->next = info;
+ last = info;
+ }
}
- } else {
- monitor_printf(mon, "There is no suitable snapshot available\n");
}
g_free(sn_tab);
- g_free(available_snapshots);
+ return snapshot_list;
}
void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
diff --git a/sysemu.h b/sysemu.h
index e440dcc..e7182f4 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -65,8 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
void qemu_add_machine_init_done_notifier(Notifier *notify);
-void do_info_snapshots(Monitor *mon);
-
void qemu_announce_self(void);
bool qemu_savevm_state_blocked(Error **errp);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 16/17] qapi: Convert info snapshots
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 16/17] qapi: Convert info snapshots Pavel Hrdina
@ 2012-12-14 17:18 ` Luiz Capitulino
0 siblings, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 17:18 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:50 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> hmp.c | 33 +++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> monitor.c | 2 +-
> qapi-schema.json | 13 +++++++++++++
> qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++
> savevm.c | 52 +++++++++++++++++++++++++---------------------------
> sysemu.h | 2 --
> 7 files changed, 107 insertions(+), 30 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 78c9a7c..1983210 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -628,6 +628,39 @@ void hmp_info_block_jobs(Monitor *mon)
> }
> }
>
> +void hmp_info_snapshots(Monitor *mon)
> +{
> + SnapshotInfoList *list;
> + Error *err = NULL;
> + char buf[256];
> + QEMUSnapshotInfo sn;
> +
> + list = qmp_query_vm_snapshots(&err);
> +
> + if (error_is_set(&err)) {
> + hmp_handle_error(mon, &err);
> + return;
> + }
> +
> + if (!list) {
> + monitor_printf(mon, "There is no snapshot available.\n");
> + return;
> + }
> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> + while (list) {
> + memcpy(&(sn.id_str), list->value->id, sizeof(sn.id_str));
> + memcpy(&(sn.name), list->value->name, sizeof(sn.name));
> + sn.date_sec = list->value->date_sec;
> + sn.date_nsec = list->value->date_nsec;
> + sn.vm_clock_nsec = list->value->vm_clock_nsec;
> + sn.vm_state_size = list->value->vm_state_size;
> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
> + list = list->next;
> + }
> +
> + qapi_free_SnapshotInfoList(list);
list should be NULL here, so you're actually leaking the list returned
by qmp_query_vm_snapshots().
> +}
> +
> void hmp_quit(Monitor *mon, const QDict *qdict)
> {
> monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index 8f929af..1c21a82 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon);
> void hmp_info_balloon(Monitor *mon);
> void hmp_info_pci(Monitor *mon);
> void hmp_info_block_jobs(Monitor *mon);
> +void hmp_info_snapshots(Monitor *mon);
> void hmp_quit(Monitor *mon, const QDict *qdict);
> void hmp_stop(Monitor *mon, const QDict *qdict);
> void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/monitor.c b/monitor.c
> index b8abff2..2be93c0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2596,7 +2596,7 @@ static mon_cmd_t info_cmds[] = {
> .args_type = "",
> .params = "",
> .help = "show the currently saved VM snapshots",
> - .mhandler.info = do_info_snapshots,
> + .mhandler.info = hmp_info_snapshots,
> },
> {
> .name = "status",
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 431df69..a8742a1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1224,6 +1224,19 @@
> { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
>
> ##
> +# @query-vm-snapshots:
> +#
> +# List available snapshots for VM.
> +#
> +# Returns: an array of @SnapshotInfo describing each snapshot or an empty array
> +# on success
> +# If an error occurs, GenericError with error message
We don't need the line above.
> +#
> +# Since: 1.3
> +##
> +{ 'command': 'query-vm-snapshots', 'returns': ['SnapshotInfo'] }
> +
> +##
> # @quit:
> #
> # This command will cause the QEMU process to exit gracefully. While every
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c3671f7..f839c40 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2732,3 +2732,37 @@ EQMP
> .args_type = "",
> .mhandler.cmd_new = qmp_marshal_input_query_target,
> },
> +
> +SQMP
> +query-vm-snapshots
> +----------
> +
> +List available snapshots for VM.
> +
> +Return an array of json-object, each with the following information:
> +
> +- "id": unique snapshot id
> +
> +- "name": user choosen name
> +
> +- "vm-state-size": size of the VM state
> +
> +- "date-sec": UTC date of the snapshot
> +
> +- "date-nsec": date in nano seconds
> +
> +- "vm-clock-nsec": VM clock relative to boot in nano seconds
> +
> +Example:
> +
> +-> { "execute": "query-vm-snapshots" }
> +<- {"return": [{"vm-clock-nsec": 36420253254, "name": "my_snapshot",
> + "date-sec": 1345120008, "date-nsec": 151984000, "id": "6",
> + "vm-state-size": 166515500}]}
> +
> +EQMP
> + {
> + .name = "query-vm-snapshots",
> + .args_type = "",
> + .mhandler.cmd_new = qmp_marshal_input_query_vm_snapshots,
> + },
> diff --git a/savevm.c b/savevm.c
> index 8da888e..36ccc46 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2373,34 +2373,28 @@ void qmp_vm_snapshot_delete(const char *name, Error **errp)
> }
> }
>
> -void do_info_snapshots(Monitor *mon)
> +SnapshotInfoList *qmp_query_vm_snapshots(Error **errp)
> {
> + SnapshotInfoList *snapshot_list = NULL, *last = NULL;
> BlockDriverState *bs, *bs1;
> QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
> int nb_sns, i, ret, available;
> - int total;
> - int *available_snapshots;
> - char buf[256];
>
> bs = bdrv_snapshots();
> if (!bs) {
> - monitor_printf(mon, "No available block device supports snapshots\n");
> - return;
> + error_setg(errp, "No block device supports snapshots.");
> + return NULL;
> }
>
> - nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
> - if (nb_sns < 0) {
> - monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> - return;
> + nb_sns = bdrv_snapshot_list(bs, &sn_tab, errp);
You should not use errp to detect internal errors.
> + if (error_is_set(errp)) {
> + return NULL;
> }
>
> if (nb_sns == 0) {
> - monitor_printf(mon, "There is no snapshot available.\n");
> - return;
> + return NULL;
> }
>
> - available_snapshots = g_malloc0(sizeof(int) * nb_sns);
> - total = 0;
> for (i = 0; i < nb_sns; i++) {
> sn = &sn_tab[i];
> available = 1;
> @@ -2417,24 +2411,28 @@ void do_info_snapshots(Monitor *mon)
> }
>
> if (available) {
> - available_snapshots[total] = i;
> - total++;
> - }
> - }
> -
> - if (total > 0) {
> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> - for (i = 0; i < total; i++) {
> - sn = &sn_tab[available_snapshots[i]];
> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> + SnapshotInfoList *info = g_malloc0(sizeof(*info));
> + info->value = g_malloc0(sizeof(*info->value));
> + info->value->id = g_strdup(sn->id_str);
> + info->value->name = g_strdup(sn->name);
> + info->value->vm_state_size = sn->vm_state_size;
> + info->value->date_sec = sn->date_sec;
> + info->value->date_nsec = sn->date_nsec;
> + info->value->vm_clock_nsec = sn->vm_clock_nsec;
> +
> + if (!snapshot_list) {
> + snapshot_list = info;
> + last = info;
> + } else {
> + last->next = info;
> + last = info;
> + }
> }
> - } else {
> - monitor_printf(mon, "There is no suitable snapshot available\n");
> }
>
> g_free(sn_tab);
> - g_free(available_snapshots);
>
> + return snapshot_list;
> }
>
> void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
> diff --git a/sysemu.h b/sysemu.h
> index e440dcc..e7182f4 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -65,8 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
>
> void qemu_add_machine_init_done_notifier(Notifier *notify);
>
> -void do_info_snapshots(Monitor *mon);
> -
> void qemu_announce_self(void);
>
> bool qemu_savevm_state_blocked(Error **errp);
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v2 17/17] vm-snapshot-save: add force parameter
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (15 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 16/17] qapi: Convert info snapshots Pavel Hrdina
@ 2012-12-13 15:40 ` Pavel Hrdina
2012-12-14 17:24 ` [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Luiz Capitulino
2012-12-20 2:27 ` Wenchao Xia
18 siblings, 0 replies; 33+ messages in thread
From: Pavel Hrdina @ 2012-12-13 15:40 UTC (permalink / raw)
To: qemu-devel; +Cc: phrdina
HMP command "savevm" now takes extra optional force parameter to specifi whether
replace existing snapshot or not.
QMP command "vm-snapshot-save" has also extra optional force parameter and
name parameter isn't optional anymore.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 16 ++++++++--------
hmp.c | 25 ++++++++++++++++++++++++-
qapi-schema.json | 9 ++++++---
qmp-commands.hx | 15 +++++++++------
savevm.c | 34 +++++++++++++---------------------
5 files changed, 60 insertions(+), 39 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 0b3d783..04cee55 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",
+ .args_type = "force:-b,name:s?,",
+ .params = "[-f] [tag|id]",
+ .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{tag}|@var{id}]
@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 @var{tag} or @var{id}, it is replaced. More info at
-@ref{vm_snapshots}.
+Create a snapshot of the whole virtual machine. Paramtere "name" is optional.
+If @var{tag} is provided, it is used as human readable identifier. If there is
+already a snapshot with the same @var{tag} or @var{id}, @var{-f} flag needs to
+be specified. More info at @ref{vm_snapshots}.
ETEXI
{
diff --git a/hmp.c b/hmp.c
index 1983210..f5e7334 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1372,9 +1372,32 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
{
const char *name = qdict_get_try_str(qdict, "name");
+ char new_name[256];
+ bool force = qdict_get_try_bool(qdict, "force", 0);
Error *err = NULL;
+#ifdef _WIN32
+ struct _timeb tb;
+ struct tm *ptm;
+#else
+ struct timeval tv;
+ struct tm tm;
+#endif
+
+ if (!name) {
+#ifdef _WIN32
+ time_t t = tb.time;
+ ptm = localtime(&t);
+ strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm);
+#else
+ /* cast below needed for OpenBSD where tv_sec is still 'long' */
+ localtime_r((const time_t *)&tv.tv_sec, &tm);
+ strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", &tm);
+#endif
+ } else {
+ pstrcpy(new_name, sizeof(new_name), name);
+ }
- qmp_vm_snapshot_save(!!name, name, &err);
+ qmp_vm_snapshot_save(new_name, !!force, force, &err);
hmp_handle_error(mon, &err);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index a8742a1..452b52b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3036,19 +3036,22 @@
#
# Create a snapshot of the whole virtual machine. If tag is provided as @name,
# it is used as human readable identifier. If there is already a snapshot
-# with the same tag or ID, it is replaced.
+# with the same tag or id, the force argument needs to be true to replace it.
#
# The VM is automatically stopped and resumed and saving a snapshot can take
# a long time.
#
-# @name: #optional tag of new snapshot or tag|id of existing snapshot
+# @name: tag of new snapshot or tag|id of existing snapshot
+#
+# @force: #optional specify whether existing snapshot is replaced or not,
+# default is false
#
# Returns: Nothing on success
# If an error occurs, GenericError with error message
#
# Since: 1.4.0
##
-{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
+{ 'command': 'vm-snapshot-save', 'data': {'name': 'str', '*force': 'bool'} }
##
# @vm-snapshot-load:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f839c40..077276e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1349,9 +1349,9 @@ Example:
EQMP
{
.name = "vm-snapshot-save",
- .args_type = "name:s?",
- .params = "name",
- .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+ .args_type = "name:s,force:b?",
+ .params = "name [force]",
+ .help = "save a VM snapshot. To replace existing snapshot use force parameter.",
.mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
},
@@ -1360,15 +1360,18 @@ vm-snapshot-save
------
Create a snapshot of the whole virtual machine. If tag is provided as name,
-it is used as human readable identifier. If there is already a snapshot
-with the same tag or id, it is replaced.
+it is used as human readable identifier. If there is already a snapshot with
+the same tag, the force argument needs to be true to replace it.
The VM is automatically stopped and resumed and saving a snapshot can take
a long time.
Arguments:
-- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional)
+- "name": tag of new snapshot or tag|id of existing snapshot (json-string)
+
+- "force": specify whether existing snapshot is replaced or not,
+ default is false (json-bool, optional)
Example:
diff --git a/savevm.c b/savevm.c
index 36ccc46..effb4df 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,
return 0;
}
-void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
+void qmp_vm_snapshot_save(const char *name, bool has_force, bool force, Error **errp)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2151,10 +2151,8 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
uint64_t vm_state_size;
#ifdef _WIN32
struct _timeb tb;
- struct tm *ptm;
#else
struct timeval tv;
- struct tm tm;
#endif
/* Verify if there is a device that doesn't support snapshots and is writable */
@@ -2195,29 +2193,23 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
#endif
sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
- if (has_name) {
- ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
- if (ret >= 0) {
+ ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
+ if (ret >= 0) {
+ if (has_force && force) {
pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+
+ del_existing_snapshots(name, errp);
+ if (error_is_set(errp)) {
+ goto the_end;
+ }
} else {
- pstrcpy(sn->name, sizeof(sn->name), name);
+ error_setg(errp, "Snapshot '%s' exist. For override specify "
+ "'force=yes'.", name);
+ goto the_end;
}
} else {
-#ifdef _WIN32
- time_t t = tb.time;
- ptm = localtime(&t);
- strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", ptm);
-#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);
-#endif
- }
-
- /* Delete old snapshots of the same name */
- if (has_name && del_existing_snapshots(name, errp) < 0) {
- goto the_end;
+ pstrcpy(sn->name, sizeof(sn->name), name);
}
/* save the VM state */
--
1.8.0.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (16 preceding siblings ...)
2012-12-13 15:40 ` [Qemu-devel] [PATCH v2 17/17] vm-snapshot-save: add force parameter Pavel Hrdina
@ 2012-12-14 17:24 ` Luiz Capitulino
2012-12-20 2:27 ` Wenchao Xia
18 siblings, 0 replies; 33+ messages in thread
From: Luiz Capitulino @ 2012-12-14 17:24 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Thu, 13 Dec 2012 16:40:34 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:
> This patch series convert these commands into qapi and introduce QMP commands
> vm-snapshot-save, vm-snapshot-load, vm-snapshot-delete and query-vm-snapshots.
> It also rewrite error report for functions used by these commands.
I've reviewed most of this series and identified several problems, but I
think that the major problem here is that this series is doing too much at
once.
A properly done and split series would easily reach twice as many patches
this series currently has.
So, my main advice here is to start simple. You could take one of savevm,
delvm or loadvm and convert it to propagate errors internally.
Also note that this series doesn't build:
CC x86_64-softmmu/savevm.o
/home/lcapitulino/work/src/qmp-unstable/savevm.c: In function ‘qmp_vm_snapshot_load’:
/home/lcapitulino/work/src/qmp-unstable/savevm.c:2274:9: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
cc1: all warnings being treated as errors
make[1]: *** [savevm.o] Error 1
make: *** [subdir-x86_64-softmmu] Error 2
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots
2012-12-13 15:40 [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (17 preceding siblings ...)
2012-12-14 17:24 ` [Qemu-devel] [PATCH v2 00/17] qapi: Convert savevm, loadvm, delvm and info snapshots Luiz Capitulino
@ 2012-12-20 2:27 ` Wenchao Xia
18 siblings, 0 replies; 33+ messages in thread
From: Wenchao Xia @ 2012-12-20 2:27 UTC (permalink / raw)
To: Pavel Hrdina
Cc: Paolo Bonzini, Anthony Liguori, Juan Quintela, qemu-devel,
Dietmar Maurer
>
> We should also merge functionality of migrations and vm-snapshots and make
> some clean-ups of this code. I could do it as another patch. With this
> rewrite we could make vm-snapshots asynchronous.
>
>
Hi, Pavel
For vm-snapshots asynchronous, I have sent a RFC patch for it at:
http://lists.nongnu.org/archive/html/qemu-devel/2012-12/msg00800.html
and seems dietmar@proxmox.com also have his own solution at:
https://git.proxmox.com/?p=pve-qemu-kvm.git;a=blob;f=debian/patches/internal-snapshot-async.patch;
So I am reworking on it to include Dietmar's solution, My plan
will include three steps:
1) abstract a function for internal/external snapshots(block only).
2) abstract a function for internal/external live vmstate save.
3) assemble above functions at will at above layer.
patch for 1) is sent at
http://lists.nongnu.org/archive/html/qemu-devel/2012-12/msg02393.html
I agree that code should be merged as much as possible, just hope
my patch wouldn't conflict with yours patch about vmstate save.
For vmstate it have a issue about how to limit the file size to be
written, that is predict the size to write. We may need a discuss about
that.
^ permalink raw reply [flat|nested] 33+ messages in thread