* [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots
@ 2012-08-15 7:41 Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR Pavel Hrdina
` (18 more replies)
0 siblings, 19 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
This patch series convert these commands into qapi and intruduce QMP commands
vm-snapshot-save, vm-snapshot-load, vm-snapshot-delete and query-vm-snapshots.
It also rewrite error report for function used by these commands.
Last two patches introduce new functionality of savevm and vm-snapshot-save
that you cannot override existing snapshot witouth using 'force' parameter
and for QMP you have to always provide name parameter.
Pavel Hrdina (18):
qerror: introduce QERR_GENERIC_ERROR
block: add error parameter to bdrv_snapshot_create() and related
functions
block: add error parameter to bdrv_snapshot_goto() and related
functions
block: add error parameter to bdrv_snapshot_delete() and related
functions
block: add error parameter to bdrv_snapshot_list() and related
functions
block: add error parameter to bdrv_snapshot_find()
block: add error parameter to del_existing_snapshots()
savevm: add error parameter to qemu_savevm_state_begin()
savevm: add error parameter to qemu_savevm_state_iterate()
savevm: add error parameter to qemu_savevm_state_complete()
savevm: add error parameter to qemu_savevm_state()
savevm: add error parameter to qemu_loadvm_state()
qapi: Convert savevm
qapi: Convert loadvm
qapi: Convert delvm
qapi: Convert info snapshots
hmp: allow "bool" parameter to be optional
vm-snapshot-save: add force parameter
block.c | 102 +++++++++++------
block.h | 13 ++-
block/qcow2-snapshot.c | 33 +++++-
block/qcow2.h | 16 ++-
block/rbd.c | 36 ++++--
block/sheepdog.c | 48 +++++---
block_int.h | 13 ++-
hmp-commands.hx | 20 ++--
hmp.c | 86 ++++++++++++++
hmp.h | 4 +
migration.c | 8 +-
monitor.c | 17 +--
qapi-schema.json | 84 ++++++++++++++
qemu-img.c | 8 +-
qerror.h | 3 +
qmp-commands.hx | 110 ++++++++++++++++++
savevm.c | 301 +++++++++++++++++++++++++------------------------
sysemu.h | 17 ++-
vl.c | 7 +-
19 files changed, 659 insertions(+), 267 deletions(-)
--
1.7.11.2
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-30 12:11 ` Luiz Capitulino
2012-08-15 7:41 ` [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
` (17 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
qerror.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/qerror.h b/qerror.h
index d0a76a4..7e0bae7 100644
--- a/qerror.h
+++ b/qerror.h
@@ -120,6 +120,9 @@ void assert_no_error(Error *err);
#define QERR_FEATURE_DISABLED \
ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
+#define QERR_GENERIC_ERROR \
+ ERROR_CLASS_GENERIC_ERROR, "An (Errno %d) error has occurred"
+
#define QERR_INVALID_BLOCK_FORMAT \
ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
--
1.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-30 14:47 ` Luiz Capitulino
2012-08-15 7:41 ` [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
` (16 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 25 +++++++++++++++++--------
block.h | 3 ++-
block/qcow2-snapshot.c | 9 ++++++++-
block/qcow2.h | 4 +++-
block/rbd.c | 20 ++++++++++++++------
block/sheepdog.c | 17 +++++++++--------
block_int.h | 3 ++-
qemu-img.c | 2 +-
savevm.c | 2 +-
9 files changed, 57 insertions(+), 28 deletions(-)
diff --git a/block.c b/block.c
index 016858b..8bc49b7 100644
--- a/block.c
+++ b/block.c
@@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_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_set(errp, QERR_NOT_SUPPORTED);
+ ret = -ENOTSUP;
+ }
+
+ return ret;
}
int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block.h b/block.h
index 2e2be11..92e782b 100644
--- a/block.h
+++ b/block.h
@@ -296,7 +296,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..cf86dae 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,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
/* Check that the ID is unique */
if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "non-existing id identifier");
return -EEXIST;
}
@@ -415,6 +420,8 @@ fail:
g_free(sn->name);
g_free(l1_table);
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+
return ret;
}
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 5a0f79f..7bc42f0 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>
@@ -817,12 +818,15 @@ 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_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "some tag identifier");
return -EINVAL; /* we need a name for rbd snapshots */
}
@@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
*/
if (sn_info->id_str[0] != '\0' &&
strcmp(sn_info->id_str, sn_info->name) != 0) {
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "id and tag to equal");
return -EINVAL;
}
if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "shorter than 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_set(errp, QERR_GENERIC_ERROR, ret);
+ return ret;
}
return 0;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a04ad99..fc51e04 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
@@ -1730,7 +1731,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;
@@ -1743,9 +1746,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_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", " not VDI snapshot");
return -EINVAL;
}
@@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
if (ret < 0) {
- error_report("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));
goto cleanup;
}
@@ -1785,7 +1784,6 @@ 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));
goto cleanup;
}
@@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
cleanup:
closesocket(fd);
+ if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
return ret;
}
diff --git a/block_int.h b/block_int.h
index 4452f6f..b76c943 100644
--- a/block_int.h
+++ b/block_int.h
@@ -206,7 +206,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 b41e670..c798d66 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1267,7 +1267,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);
+ ret = bdrv_snapshot_create(bs, &sn, NULL);
if (ret) {
error_report("Could not create snapshot '%s': %d (%s)",
snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 0ea10c9..1b67af6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2165,7 +2165,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() and related functions
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-30 15:07 ` Luiz Capitulino
2012-08-15 7:41 ` [Qemu-devel] [PATCH 04/18] block: add error parameter to bdrv_snapshot_delete() " Pavel Hrdina
` (15 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 | 6 +++++-
block/sheepdog.c | 16 +++++++++-------
block_int.h | 3 ++-
qemu-img.c | 2 +-
savevm.c | 2 +-
9 files changed, 46 insertions(+), 27 deletions(-)
diff --git a/block.c b/block.c
index 8bc49b7..ad25184 100644
--- a/block.c
+++ b/block.c
@@ -2683,29 +2683,33 @@ 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_set(errp, QERR_DEVICE_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_set(errp, QERR_OPEN_FILE_FAILED, bdrv_get_device_name(bs));
+ ret = open_ret;
}
- return ret;
+ } else {
+ error_set(errp, QERR_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 92e782b..11edcd3 100644
--- a/block.h
+++ b/block.h
@@ -299,7 +299,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 cf86dae..8a87b0c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -426,7 +426,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;
@@ -438,13 +440,13 @@ 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_set(errp, QERR_OPEN_FILE_FAILED, 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_set(errp, QERR_NOT_SUPPORTED);
ret = -ENOTSUP;
goto fail;
}
@@ -536,6 +538,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
fail:
g_free(sn_l1_table);
+ if (!error_is_set(errp)) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
return ret;
}
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 7bc42f0..5a73d8a 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -867,12 +867,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_set(errp, QERR_GENERIC_ERROR, r);
+ }
return r;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fc51e04..557ff92 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1799,7 +1799,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;
@@ -1824,13 +1826,12 @@ 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_set(errp, QERR_OPEN_FILE_FAILED, vdi);
goto out;
}
fd = connect_to_sdog(s->addr, s->port);
if (fd < 0) {
- error_report("failed to connect");
ret = fd;
goto out;
}
@@ -1848,7 +1849,8 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
memcpy(&s->inode, buf, sizeof(s->inode));
if (!s->inode.vm_state_size) {
- error_report("Invalid snapshot");
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "existing snapshot");
ret = -ENOENT;
goto out;
}
@@ -1864,9 +1866,9 @@ out:
memcpy(s, old_s, sizeof(BDRVSheepdogState));
g_free(buf);
g_free(old_s);
-
- error_report("failed to open. recover old bdrv_sd_state.");
-
+ if (!error_is_set(errp)) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
return ret;
}
diff --git a/block_int.h b/block_int.h
index b76c943..fa6d72f 100644
--- a/block_int.h
+++ b/block_int.h
@@ -209,7 +209,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 c798d66..1f10ff4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1275,7 +1275,7 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_APPLY:
- ret = bdrv_snapshot_goto(bs, snapshot_name);
+ ret = bdrv_snapshot_goto(bs, snapshot_name, NULL);
if (ret) {
error_report("Could not apply snapshot '%s': %d (%s)",
snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 1b67af6..0931e54 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2256,7 +2256,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 04/18] block: add error parameter to bdrv_snapshot_delete() and related functions
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (2 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 05/18] block: add error parameter to bdrv_snapshot_list() " Pavel Hrdina
` (14 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 26 ++++++++++++++++++--------
block.h | 4 +++-
block/qcow2-snapshot.c | 9 ++++++++-
block/qcow2.h | 4 +++-
block/rbd.c | 6 +++++-
block/sheepdog.c | 4 +++-
block_int.h | 4 +++-
qemu-img.c | 2 +-
savevm.c | 4 ++--
9 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/block.c b/block.c
index ad25184..1480777 100644
--- a/block.c
+++ b/block.c
@@ -2712,16 +2712,26 @@ 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_set(errp, QERR_DEVICE_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_set(errp, QERR_NOT_SUPPORTED);
+ ret = -ENOTSUP;
+ }
+
+ return ret;
}
int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/block.h b/block.h
index 11edcd3..72586f2 100644
--- a/block.h
+++ b/block.h
@@ -301,7 +301,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 8a87b0c..9162a5b 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -544,7 +544,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;
@@ -553,6 +555,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_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "existing snapshot id or tag");
return -ENOENT;
}
sn = s->snapshots[snapshot_index];
@@ -564,6 +568,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
s->nb_snapshots--;
ret = qcow2_write_snapshots(bs);
if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
return ret;
}
@@ -581,6 +586,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_set(errp, QERR_GENERIC_ERROR, ret);
return ret;
}
qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
@@ -588,6 +594,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_set(errp, QERR_GENERIC_ERROR, ret);
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 5a73d8a..448d68d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -857,12 +857,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_set(errp, QERR_GENERIC_ERROR, r);
+ }
return r;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 557ff92..f864071 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1872,7 +1872,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 fa6d72f..4799abd 100644
--- a/block_int.h
+++ b/block_int.h
@@ -211,7 +211,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 1f10ff4..6ac1904 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1283,7 +1283,7 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
- ret = bdrv_snapshot_delete(bs, snapshot_name);
+ ret = bdrv_snapshot_delete(bs, snapshot_name, NULL);
if (ret) {
error_report("Could not delete snapshot '%s': %d (%s)",
snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 0931e54..4cf92d5 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2051,7 +2051,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",
@@ -2299,7 +2299,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 05/18] block: add error parameter to bdrv_snapshot_list() and related functions
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (3 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 04/18] block: add error parameter to bdrv_snapshot_delete() " Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 06/18] block: add error parameter to bdrv_snapshot_find() Pavel Hrdina
` (13 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block.c | 25 +++++++++++++++++--------
block.h | 3 ++-
block/qcow2-snapshot.c | 4 +++-
block/qcow2.h | 4 +++-
block/rbd.c | 4 +++-
block/sheepdog.c | 11 +++++++++--
block_int.h | 3 ++-
qemu-img.c | 2 +-
savevm.c | 4 ++--
9 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/block.c b/block.c
index 1480777..aa2965d 100644
--- a/block.c
+++ b/block.c
@@ -2735,16 +2735,25 @@ 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_set(errp, QERR_DEVICE_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_set(errp, QERR_NOT_SUPPORTED);
+ ret = -ENOTSUP;
+ }
+
+ return ret;
}
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/block.h b/block.h
index 72586f2..da34af0 100644
--- a/block.h
+++ b/block.h
@@ -305,7 +305,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 9162a5b..9708eb5 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -607,7 +607,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;
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 448d68d..d068e56 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -885,7 +885,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;
@@ -902,6 +903,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
} while (snap_count == -ERANGE);
if (snap_count <= 0) {
+ error_set(errp, QERR_OPEN_FILE_FAILED, bdrv_get_device_name(bs));
goto done;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index f864071..0835b11 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1880,7 +1880,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;
@@ -1898,6 +1900,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
fd = connect_to_sdog(s->addr, s->port);
if (fd < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, fd);
ret = fd;
goto out;
}
@@ -1914,6 +1917,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
closesocket(fd);
if (ret) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
goto out;
}
@@ -1925,7 +1929,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_set(errp, QERR_GENERIC_ERROR, fd);
ret = fd;
goto out;
}
@@ -1965,6 +1969,9 @@ out:
g_free(vdi_inuse);
if (ret < 0) {
+ if (!error_is_set(errp)) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
return ret;
}
diff --git a/block_int.h b/block_int.h
index 4799abd..2c1c6e8 100644
--- a/block_int.h
+++ b/block_int.h
@@ -215,7 +215,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 6ac1904..20a9b60 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1090,7 +1090,7 @@ static void dump_snapshots(BlockDriverState *bs)
int nb_sns, i;
char buf[256];
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
if (nb_sns <= 0)
return;
printf("Snapshot list:\n");
diff --git a/savevm.c b/savevm.c
index 4cf92d5..92da274 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2022,7 +2022,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++) {
@@ -2328,7 +2328,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 06/18] block: add error parameter to bdrv_snapshot_find()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (4 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 05/18] block: add error parameter to bdrv_snapshot_list() " Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 07/18] block: add error parameter to del_existing_snapshots() Pavel Hrdina
` (12 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 92da274..55b65cf 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2015,16 +2015,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)) {
@@ -2034,6 +2039,9 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
}
}
g_free(sn_tab);
+ if (ret < 0) {
+ error_set(errp, QERR_OPEN_FILE_FAILED, name);
+ }
return ret;
}
@@ -2049,7 +2057,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) {
@@ -2120,7 +2128,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);
@@ -2218,7 +2226,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) {
@@ -2242,7 +2250,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);
@@ -2348,7 +2356,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 07/18] block: add error parameter to del_existing_snapshots()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (5 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 06/18] block: add error parameter to bdrv_snapshot_find() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 08/18] savevm: add error parameter to qemu_savevm_state_begin() Pavel Hrdina
` (11 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 55b65cf..0d14f71 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2048,22 +2048,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;
}
}
@@ -2148,7 +2144,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 08/18] savevm: add error parameter to qemu_savevm_state_begin()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (6 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 07/18] block: add error parameter to del_existing_snapshots() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 09/18] savevm: add error parameter to qemu_savevm_state_iterate() Pavel Hrdina
` (10 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 653a3c1..dde4dec 100644
--- a/migration.c
+++ b/migration.c
@@ -429,7 +429,7 @@ void migrate_fd_connect(MigrationState *s)
migrate_fd_close);
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 0d14f71..d212cb2 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1555,7 +1555,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;
@@ -1595,12 +1596,14 @@ int qemu_savevm_state_begin(QEMUFile *f,
ret = se->ops->save_live_setup(f, se->opaque);
if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
qemu_savevm_state_cancel(f);
return ret;
}
}
ret = qemu_file_get_error(f);
if (ret != 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
qemu_savevm_state_cancel(f);
}
@@ -1737,7 +1740,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 4669348..bc65c64 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -78,7 +78,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 09/18] savevm: add error parameter to qemu_savevm_state_iterate()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (7 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 08/18] savevm: add error parameter to qemu_savevm_state_begin() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 10/18] savevm: add error parameter to qemu_savevm_state_complete() Pavel Hrdina
` (9 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 dde4dec..1e71374 100644
--- a/migration.c
+++ b/migration.c
@@ -321,7 +321,7 @@ static void migrate_fd_put_ready(void *opaque)
}
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 d212cb2..7d6db32 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1617,7 +1617,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;
@@ -1651,10 +1652,16 @@ int qemu_savevm_state_iterate(QEMUFile *f)
}
}
if (ret != 0) {
+ if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
return ret;
}
ret = qemu_file_get_error(f);
if (ret != 0) {
+ if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
qemu_savevm_state_cancel(f);
}
return ret;
@@ -1745,7 +1752,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 bc65c64..405ce98 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -80,7 +80,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 10/18] savevm: add error parameter to qemu_savevm_state_complete()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (8 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 09/18] savevm: add error parameter to qemu_savevm_state_iterate() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 11/18] savevm: add error parameter to qemu_savevm_state() Pavel Hrdina
` (8 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 1e71374..ec2f267 100644
--- a/migration.c
+++ b/migration.c
@@ -331,7 +331,7 @@ static void migrate_fd_put_ready(void *opaque)
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 7d6db32..0f7d681 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1667,7 +1667,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;
@@ -1691,6 +1692,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_set(errp, QERR_GENERIC_ERROR, ret);
return ret;
}
}
@@ -1720,7 +1722,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_set(errp, QERR_GENERIC_ERROR, ret);
+ }
+
+ return ret;
}
void qemu_savevm_state_cancel(QEMUFile *f)
@@ -1757,7 +1764,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 405ce98..35d6d67 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -82,7 +82,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 11/18] savevm: add error parameter to qemu_savevm_state()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (9 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 10/18] savevm: add error parameter to qemu_savevm_state_complete() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
` (7 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 0f7d681..0d54115 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1741,7 +1741,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 = {
@@ -1749,22 +1750,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) {
@@ -2171,7 +2174,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state()
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (10 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 11/18] savevm: add error parameter to qemu_savevm_state() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-30 17:09 ` Luiz Capitulino
2012-08-15 7:41 ` [Qemu-devel] [PATCH 13/18] qapi: Convert savevm Pavel Hrdina
` (6 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
migration.c | 2 +-
savevm.c | 44 ++++++++++++++++++++++++++++----------------
sysemu.h | 3 ++-
3 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/migration.c b/migration.c
index ec2f267..f048faf 100644
--- a/migration.c
+++ b/migration.c
@@ -88,7 +88,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
void process_incoming_migration(QEMUFile *f)
{
- if (qemu_loadvm_state(f) < 0) {
+ if (qemu_loadvm_state(f, NULL) < 0) {
fprintf(stderr, "load of migration failed\n");
exit(0);
}
diff --git a/savevm.c b/savevm.c
index 0d54115..500eb72 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1916,7 +1916,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);
@@ -1925,21 +1926,26 @@ int qemu_loadvm_state(QEMUFile *f)
unsigned int v;
int ret;
- if (qemu_savevm_state_blocked(NULL)) {
- return -EINVAL;
+ if (qemu_savevm_state_blocked(errp)) {
+ return -ENOTSUP;
}
v = qemu_get_be32(f);
- if (v != QEMU_VM_FILE_MAGIC)
+ if (v != QEMU_VM_FILE_MAGIC) {
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "Unknown vm-state 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_set(errp, QERR_NOT_SUPPORTED);
return -ENOTSUP;
}
- if (v != QEMU_VM_FILE_VERSION)
+ if (v != QEMU_VM_FILE_VERSION) {
+ error_set(errp, QERR_NOT_SUPPORTED);
return -ENOTSUP;
+ }
while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
uint32_t instance_id, version_id, section_id;
@@ -1961,15 +1967,18 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "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_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "savevm: unsupported version %d for '%s' v%d",
+ version_id, idstr, se->version_id);
ret = -EINVAL;
goto out;
}
@@ -1984,8 +1993,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_set(errp, QERR_GENERIC_ERROR, ret);
goto out;
}
break;
@@ -1999,20 +2007,21 @@ int qemu_loadvm_state(QEMUFile *f)
}
}
if (le == NULL) {
- fprintf(stderr, "Unknown savevm section %d\n", section_id);
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "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_set(errp, QERR_GENERIC_ERROR, ret);
goto out;
}
break;
default:
- fprintf(stderr, "Unknown savevm section type %d\n", section_type);
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "Unknown savevm section type %d", section_type);
ret = -EINVAL;
goto out;
}
@@ -2030,6 +2039,9 @@ out:
if (ret == 0) {
ret = qemu_file_get_error(f);
+ if (ret < 0) {
+ error_set(errp, QERR_GENERIC_ERROR, ret);
+ }
}
return ret;
@@ -2297,7 +2309,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 35d6d67..651d96e 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -85,7 +85,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 13/18] qapi: Convert savevm
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (11 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 19:49 ` Eric Blake
2012-08-15 7:41 ` [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm Pavel Hrdina
` (5 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 2 +-
hmp.c | 10 ++++++++++
hmp.h | 1 +
qapi-schema.json | 19 +++++++++++++++++++
qmp-commands.hx | 29 +++++++++++++++++++++++++++++
savevm.c | 25 +++++++++----------------
sysemu.h | 1 -
7 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index f6104b0..32476d1 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -267,7 +267,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
diff --git a/hmp.c b/hmp.c
index a9d5675..9f791e8 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1102,3 +1102,13 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
qmp_closefd(fdname, &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 7dd93bf..0ce1021 100644
--- a/hmp.h
+++ b/hmp.h
@@ -71,5 +71,6 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict);
void hmp_netdev_del(Monitor *mon, const QDict *qdict);
void hmp_getfd(Monitor *mon, const QDict *qdict);
void hmp_closefd(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 53bbe46..5c086f9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2356,3 +2356,22 @@
# Since: 1.2.0
##
{ 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
+
+##
+# @vm-snapshot-save:
+#
+# Create a snapshot of the whole virtual machine. If '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.
+#
+# The VM is automatically stopped and resumed and saving a snapshot can take
+# a long time.
+#
+# @name: tag or id of new or existing snapshot
+#
+# Returns: Nothing on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.2
+##
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 527b9f7..ac7c4c9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1111,6 +1111,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,
+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 or id of new or existing snapshot
+
+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 500eb72..42e496f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2100,7 +2100,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;
@@ -2115,7 +2115,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;
@@ -2126,15 +2125,14 @@ 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_set(errp, QERR_NOT_SUPPORTED);
return;
}
}
bs = bdrv_snapshots();
if (!bs) {
- monitor_printf(mon, "No block device can accept snapshots\n");
+ error_set(errp, QERR_NOT_SUPPORTED);
return;
}
@@ -2155,7 +2153,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);
@@ -2176,21 +2174,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_set(errp, QERR_OPEN_FILE_FAILED, 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;
}
@@ -2201,11 +2198,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 651d96e..d1ceade 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -69,7 +69,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (12 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 13/18] qapi: Convert savevm Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-16 3:31 ` Eric Blake
2012-08-15 7:41 ` [Qemu-devel] [PATCH 15/18] qapi: Convert delvm Pavel Hrdina
` (4 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp-commands.hx | 2 +-
hmp.c | 10 +++++++++
hmp.h | 1 +
monitor.c | 12 -----------
qapi-schema.json | 15 ++++++++++++++
qmp-commands.hx | 25 ++++++++++++++++++++++
savevm.c | 63 ++++++++++++++++++++++++++++----------------------------
sysemu.h | 1 -
vl.c | 7 ++++++-
9 files changed, 90 insertions(+), 46 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 32476d1..f9709d9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -284,7 +284,7 @@ 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
diff --git a/hmp.c b/hmp.c
index 9f791e8..c2db7b2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1112,3 +1112,13 @@ void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
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 0ce1021..b669b66 100644
--- a/hmp.h
+++ b/hmp.h
@@ -72,5 +72,6 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict);
void hmp_getfd(Monitor *mon, const QDict *qdict);
void hmp_closefd(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 dd63f1d..b75b9e0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2330,18 +2330,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)
{
mon_fd_t *monfd;
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c086f9..c477b26 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2375,3 +2375,18 @@
# Since: 1.2
##
{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
+
+##
+# @vm-snapshot-load:
+#
+# Set the whole virtual machine to the snapshot identified by the tag
+# 'tag' or the unique snapshot ID 'id'.
+#
+# @name: tag or id of existing snapshot
+#
+# Returns: Nothing on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.2
+##
+{ 'command': 'vm-snapshot-load', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ac7c4c9..a044345 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1140,6 +1140,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
+'tag' or the unique snapshot ID 'id'.
+
+Arguments:
+
+- "name": tag or id of existing snapshot
+
+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 42e496f..0b95646 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2233,27 +2233,28 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
return;
}
-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_set(errp, QERR_NOT_SUPPORTED);
+ 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_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ "name", "whole VM snapshot");
+ return;
}
/* Verify if there is any device that doesn't support snapshots and is
@@ -2266,30 +2267,28 @@ 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_set(errp, QERR_NOT_SUPPORTED);
+ 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;
}
}
}
@@ -2297,20 +2296,22 @@ 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_set(errp, QERR_OPEN_FILE_FAILED,
+ bdrv_get_device_name(bs_vm_state));
+ 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 d1ceade..3327c49 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -69,7 +69,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 d01256a..190fe66 100644
--- a/vl.c
+++ b/vl.c
@@ -3683,8 +3683,13 @@ 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;
+ error_report("%s", error_get_pretty(errp));
+ error_free(errp);
}
}
--
1.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 15/18] qapi: Convert delvm
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (13 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots Pavel Hrdina
` (3 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 f9709d9..d6cd94c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -299,7 +299,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 c2db7b2..a3fd8ca 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1122,3 +1122,13 @@ void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
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 b669b66..738d9bc 100644
--- a/hmp.h
+++ b/hmp.h
@@ -73,5 +73,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
void hmp_closefd(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 c477b26..feea992 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2390,3 +2390,17 @@
# Since: 1.2
##
{ 'command': 'vm-snapshot-load', 'data': {'name': 'str'} }
+
+##
+# @vm-snapshot-delete:
+#
+# Delete the snapshot identified by 'tag' or 'id'.
+#
+# @name: tag or id of existing snapshot
+#
+# Returns: Nothing on success
+# If an error occurs, GenericError with error message
+#
+# Since: 1.2
+##
+{ 'command': 'vm-snapshot-delete', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a044345..bca4267 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1165,6 +1165,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 or id of existing snapshot
+
+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 0b95646..3f7ec44 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2314,31 +2314,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_set(errp, QERR_NOT_SUPPORTED);
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 3327c49..44afe61 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -69,7 +69,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (14 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 15/18] qapi: Convert delvm Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-16 4:36 ` Eric Blake
2012-08-15 7:41 ` [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional Pavel Hrdina
` (2 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
hmp.c | 33 +++++++++++++++++++++++++++++++++
hmp.h | 1 +
monitor.c | 2 +-
qapi-schema.json | 34 ++++++++++++++++++++++++++++++++++
qmp-commands.hx | 30 ++++++++++++++++++++++++++++++
savevm.c | 51 ++++++++++++++++++++++++---------------------------
sysemu.h | 2 --
7 files changed, 123 insertions(+), 30 deletions(-)
diff --git a/hmp.c b/hmp.c
index a3fd8ca..cf0e036 100644
--- a/hmp.c
+++ b/hmp.c
@@ -613,6 +613,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->tag, sizeof(sn.name));
+ sn.date_sec = list->value->date;
+ sn.date_nsec = sn.date_sec * 1000000000;
+ sn.vm_clock_nsec = list->value->vm_clock;
+ sn.vm_state_size = list->value->vm_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 738d9bc..9b67fc3 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 b75b9e0..db9a8d0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2541,7 +2541,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 feea992..763b51e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1053,6 +1053,40 @@
{ 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
##
+# @SnapshotInfo:
+#
+# Snapshot list. This structure contains list of snapshots for virtual machine.
+#
+# @id: id of the snapshot.
+#
+# @tag: human readable tag of the snapshot.
+#
+# @vm_size: size of the snapshot in Bytes.
+#
+# @date: date and time of the snapshot as unix timestamp.
+#
+# @vm_clock: time in the guest in nsecs.
+#
+# Since: 1.2
+##
+{ 'type': 'SnapshotInfo',
+ 'data': {'id': 'str', 'tag': 'str', 'vm_size': 'int',
+ 'date': 'int', 'vm_clock': 'int'} }
+
+##
+# @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.2
+##
+{ '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 bca4267..3609de8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2465,3 +2465,33 @@ EQMP
.mhandler.cmd_new = qmp_marshal_input_query_cpu_definitions,
},
+SQMP
+query-vm-snapshots
+----------
+
+List available snapshots for VM.
+
+Return an array of json-object, each with the following information:
+
+- "id": id of the snapshot.
+
+- "tag": human readable tag of the snapshot.
+
+- "vm_size": size of the snapshot in Bytes.
+
+- "date": date and time of the snapshot as unix timestamp.
+
+- "vm_clock": time in the guest in nsecs.
+
+Example:
+
+-> { "execute": "query-vm-snapshots" }
+<- {"return": [{"vm_size": 212309067, "tag": "my_snapshot",
+ "vm_clock": 630176706190, "id": "1", "date": 1341999856}]}
+
+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 3f7ec44..ca7448d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2332,34 +2332,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_set(errp, QERR_NOT_SUPPORTED);
+ 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;
@@ -2376,24 +2370,27 @@ 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->tag = g_strdup(sn->name);
+ info->value->vm_size = sn->vm_state_size;
+ info->value->date = sn->date_sec;
+ info->value->vm_clock = 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 44afe61..a77dad2 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -69,8 +69,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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (15 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-16 4:39 ` Eric Blake
2012-08-15 7:41 ` [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter Pavel Hrdina
2012-08-30 17:15 ` [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Luiz Capitulino
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
monitor.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/monitor.c b/monitor.c
index db9a8d0..a538e6a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3769,6 +3769,9 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
val = 1;
} else if (p - beg == 3 && !memcmp(beg, "off", p - beg)) {
val = 0;
+ } else if (*typestr == '?') {
+ typestr++;
+ break;
} else {
monitor_printf(mon, "Expected 'on' or 'off'\n");
goto fail;
--
1.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (16 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional Pavel Hrdina
@ 2012-08-15 7:41 ` Pavel Hrdina
2012-08-16 5:00 ` Eric Blake
2012-08-30 17:15 ` [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Luiz Capitulino
18 siblings, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-15 7:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Hrdina
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 | 14 +++++++-------
hmp.c | 25 ++++++++++++++++++++++++-
qapi-schema.json | 12 +++++++-----
qmp-commands.hx | 16 +++++++++-------
savevm.c | 35 ++++++++++++++---------------------
5 files changed, 61 insertions(+), 41 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d6cd94c..58fd1f2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -264,19 +264,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 = "name:s?,force:b?",
+ .params = "[tag|id] [on|off]",
+ .help = "save a VM snapshot. To replace existing snapshot use force parameter.",
.mhandler.cmd = hmp_vm_snapshot_save,
},
STEXI
@item savevm [@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 tag or ID, it is replaced. More info at
-@ref{vm_snapshots}.
+Create a snapshot of the whole virtual machine. Parameter "name" is optional.
+If @var{tag} is provided, it is used as human readable identifier. If there is
+already a snapshot with the same tag, force argument need to be "yes" to
+replace it. More info at @ref{vm_snapshots}.
ETEXI
{
diff --git a/hmp.c b/hmp.c
index cf0e036..14df8f1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1139,9 +1139,32 @@ void hmp_closefd(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 763b51e..e68b259 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2394,21 +2394,23 @@
##
# @vm-snapshot-save:
#
-# Create a snapshot of the whole virtual machine. If '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.
+# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
+# human readable identifier. If there is already a snapshot with the same tag,
+# force argument need to be "yes" to replace it.
#
# The VM is automatically stopped and resumed and saving a snapshot can take
# a long time.
#
-# @name: tag or id of new or existing snapshot
+# @name: tag of new or existing snapshot
+#
+# @force: specify whether existing snapshot is replaced or not
#
# Returns: Nothing on success
# If an error occurs, GenericError with error message
#
# Since: 1.2
##
-{ '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 3609de8..c86ceb5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1112,9 +1112,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
},
@@ -1122,16 +1122,18 @@ SQMP
vm-snapshot-save
------
-Create a snapshot of the whole virtual machine. If '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.
+Create a snapshot of the whole virtual machine. Provided 'tag' is used as
+human readable identifier. If there is already a snapshot with the same tag,
+force argument need to be "yes" to replace it.
The VM is automatically stopped and resumed and saving a snapshot can take
a long time.
Arguments:
-- "name": tag or id of new or existing snapshot
+- "name": tag of new or existing snapshot
+
+- "force": specify whether existing snapshot is replaced or not
Example:
diff --git a/savevm.c b/savevm.c
index ca7448d..4ec7663 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2100,7 +2100,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;
@@ -2110,10 +2110,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 */
@@ -2153,29 +2151,24 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "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.7.11.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm
2012-08-15 7:41 ` [Qemu-devel] [PATCH 13/18] qapi: Convert savevm Pavel Hrdina
@ 2012-08-15 19:49 ` Eric Blake
2012-08-16 10:16 ` Pavel Hrdina
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2012-08-15 19:49 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3935 bytes --]
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
I'm focusing my review more on the public interface (since that's what
affects libvirt), and therefore glanced through 1 through 12 but did not
pay close attention to them.
> hmp-commands.hx | 2 +-
> hmp.c | 10 ++++++++++
> hmp.h | 1 +
> qapi-schema.json | 19 +++++++++++++++++++
> qmp-commands.hx | 29 +++++++++++++++++++++++++++++
> savevm.c | 25 +++++++++----------------
> sysemu.h | 1 -
> 7 files changed, 69 insertions(+), 18 deletions(-)
>
> +
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> +{
> + const char *name = qdict_get_try_str(qdict, "name");
In the cover letter, you said "and for QMP you have to always provide
name parameter" - but this says 'name' is optional and can still be
empty (id only).
> +++ b/qapi-schema.json
> @@ -2356,3 +2356,22 @@
> # Since: 1.2.0
> ##
> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
> +
> +##
> +# @vm-snapshot-save:
> +#
> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,
'tag' is a misnomer, since you list @name as the parameter 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: tag or id of new or existing snapshot
Here, you document @name as required,
> +#
> +# Returns: Nothing on success
> +# If an error occurs, GenericError with error message
> +#
> +# Since: 1.2
We missed 1.2 hard freeze. This better be 1.3.
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
but here, you listed it as '*name' meaning optional. I'm okay with
leaving name optional, provided that it means that you end up allocating
the next unique id. But if that's the case, then returning nothing is
wrong; this command needs to return { 'id':'int', '*name':'str' }, so
that the user knows what snapshot just got allocated.
> +SQMP
> +vm-snapshot-save
> +------
> +
> +Create a snapshot of the whole virtual machine. If '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.
> +
> +The VM is automatically stopped and resumed and saving a snapshot can take
> +a long time.
I don't like that this command can take a long time. Just today on
#virt IRC, someone was complaining that 'savevm' HMP cannot be canceled.
If we're going to create a new interface, it would be nicer to create a
command that starts the save process and returns immediately, as well as
commands to track progress, allow an early abort, and send an event on
completion. The HMP 'savevm' interface can issue multiple QMP commands
under the hood to continue with it's blocking behavior, but as long as
we are fixing things, I think the QMP interface should be more powerful.
> +
> +Arguments:
> +
> +- "name": tag or id of new or existing snapshot
> +
> +Example:
> +
> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
> +<- { "return": {} }
Again, you need to return the id of the just-created snapshot.
> @@ -2176,21 +2174,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) {
Here's hoping later in the series updates this to make saner decisions.
(Maybe I should peruse the entire series before commenting on
individual patches?)
--
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: 620 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm
2012-08-15 7:41 ` [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm Pavel Hrdina
@ 2012-08-16 3:31 ` Eric Blake
2012-08-16 4:34 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2012-08-16 3:31 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 990 bytes --]
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> hmp-commands.hx | 2 +-
> hmp.c | 10 +++++++++
> hmp.h | 1 +
> monitor.c | 12 -----------
> qapi-schema.json | 15 ++++++++++++++
> qmp-commands.hx | 25 ++++++++++++++++++++++
> savevm.c | 63 ++++++++++++++++++++++++++++----------------------------
> sysemu.h | 1 -
> vl.c | 7 ++++++-
> 9 files changed, 90 insertions(+), 46 deletions(-)
>
> +
> +##
> +# @vm-snapshot-load:
> +#
> +# Set the whole virtual machine to the snapshot identified by the tag
> +# 'tag' or the unique snapshot ID 'id'.
> +#
> +# @name: tag or id of existing snapshot
> +#
> +# Returns: Nothing on success
> +# If an error occurs, GenericError with error message
> +#
> +# Since: 1.2
1.3
--
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: 620 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm
2012-08-16 3:31 ` Eric Blake
@ 2012-08-16 4:34 ` Eric Blake
0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2012-08-16 4:34 UTC (permalink / raw)
To: Eric Blake; +Cc: Pavel Hrdina, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 700 bytes --]
On 08/15/2012 09:31 PM, Eric Blake wrote:
>
>> +
>> +##
>> +# @vm-snapshot-load:
>> +#
>> +# Set the whole virtual machine to the snapshot identified by the tag
>> +# 'tag' or the unique snapshot ID 'id'.
>> +#
>> +# @name: tag or id of existing snapshot
>> +#
>> +# Returns: Nothing on success
>> +# If an error occurs, GenericError with error message
It might also be worth documenting that vm-snapshot-load can only load
online snapshots with associated VM state (those created by savevm), and
not offline snapshots (those created by 'qemu-img snapshot').
--
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: 620 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots
2012-08-15 7:41 ` [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots Pavel Hrdina
@ 2012-08-16 4:36 ` Eric Blake
2012-08-16 10:17 ` Pavel Hrdina
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2012-08-16 4:36 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: Benoît Canet, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> hmp.c | 33 +++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> monitor.c | 2 +-
> qapi-schema.json | 34 ++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 30 ++++++++++++++++++++++++++++++
> savevm.c | 51 ++++++++++++++++++++++++---------------------------
> sysemu.h | 2 --
> 7 files changed, 123 insertions(+), 30 deletions(-)
>
> @@ -1053,6 +1053,40 @@
> { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
>
> ##
> +# @SnapshotInfo:
> +#
We've got competition here:
https://lists.gnu.org/archive/html/qemu-devel/2012-08/msg02961.html
These two patches need to converge into a single design.
> +# Snapshot list. This structure contains list of snapshots for virtual machine.
> +#
> +# @id: id of the snapshot.
> +#
> +# @tag: human readable tag of the snapshot.
> +#
> +# @vm_size: size of the snapshot in Bytes.
As this is a new QMP command, you should use '-', not '_'. Benoit's
patch named this @vm-state-size.
> +#
> +# @date: date and time of the snapshot as unix timestamp.
> +#
> +# @vm_clock: time in the guest in nsecs.
For online internal snapshots (those created by savevm), the vm clock
offset makes sense. But for offline snapshots (those created by
'qemu-img snapshot', with no VM state), there is no vm clock offset.
'qemu-img info' lists 00:00:00.000 as a result, but I wonder if it would
be better to leave this field (and vm-state-size) as #optional and omit
them from the JSON for offline snapshots.
And since qemu refuses to load offline snapshots, it might be worth an
additional field in the JSON that says whether a snapshot is online or
offline, to make it easier for the parsing application to determine
whether it can be loaded or must go through qemu-img while the guest is
offline (then again, keeping vm-state-size as non-optional, and checking
for 0 size, somewhat covers this).
> +#
> +# Since: 1.2
1.3 (entire series...)
--
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: 620 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional
2012-08-15 7:41 ` [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional Pavel Hrdina
@ 2012-08-16 4:39 ` Eric Blake
0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2012-08-16 4:39 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> monitor.c | 3 +++
> 1 file changed, 3 insertions(+)
NACK. See patch 18 why I think you don't need this.
>
> diff --git a/monitor.c b/monitor.c
> index db9a8d0..a538e6a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3769,6 +3769,9 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> val = 1;
> } else if (p - beg == 3 && !memcmp(beg, "off", p - beg)) {
> val = 0;
> + } else if (*typestr == '?') {
> + typestr++;
> + break;
> } else {
> monitor_printf(mon, "Expected 'on' or 'off'\n");
> goto fail;
>
--
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: 620 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter
2012-08-15 7:41 ` [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter Pavel Hrdina
@ 2012-08-16 5:00 ` Eric Blake
2012-08-16 10:19 ` Pavel Hrdina
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2012-08-16 5:00 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3802 bytes --]
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> HMP command "savevm" now takes extra optional force parameter to specifi whether
s/specifi/specify/
> replace existing snapshot or not.
>
> QMP command "vm-snapshot-save" has also extra optional force parameter and
> name parameter isn't optional anymore.
When HMP omits the name, what name are you using in it's place? Either
a nameless snapshot is okay (and QMP must expose it), or it is an error
(and HMP must now be passing a reasonable name).
/me reads patch
Oh, there was always a name; the default name was a timestamp, and you
moved the timestamp creation into HMP.
> +++ b/hmp-commands.hx
> @@ -264,19 +264,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 = "name:s?,force:b?",
> + .params = "[tag|id] [on|off]",
Rather than sticking 'force' as an optional parameter at the end,
instead you want to add '-f' as a flag at the beginning.
.args_type = "force:-b,name:s?",
.params = "[-f] name"
By doing this, you no longer need 17/18.
> + .help = "save a VM snapshot. To replace existing snapshot use force parameter.",
> .mhandler.cmd = hmp_vm_snapshot_save,
> },
>
> STEXI
> @item savevm [@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 tag or ID, it is replaced. More info at
> -@ref{vm_snapshots}.
> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.
> +If @var{tag} is provided, it is used as human readable identifier. If there is
> +already a snapshot with the same tag, force argument need to be "yes" to
s/force argument need/the force argument needs/
> +++ b/qapi-schema.json
> @@ -2394,21 +2394,23 @@
> ##
> # @vm-snapshot-save:
> #
> -# Create a snapshot of the whole virtual machine. If '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.
> +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
> +# human readable identifier. If there is already a snapshot with the same tag,
> +# force argument need to be "yes" to replace it.
s/need to be "yes"/needs to be 'true'/
> #
> # The VM is automatically stopped and resumed and saving a snapshot can take
> # a long time.
> #
> -# @name: tag or id of new or existing snapshot
> +# @name: tag of new or existing snapshot
Given your new semantics, @name must be tag when @force is false; but
the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1
regardless of the name, all while keeping the old tag. Does the new HMP
code do an id lookup, and pass in the correct @name to the QMP code?
Should the QMP code allow the same semantics of being able to pass
@force=true and @name=id instead of the normal creation of @name=tag?
> +#
> +# @force: specify whether existing snapshot is replaced or not
Based on the JSON below, this needs an #optional marking, as well as
mentioning that the default is false.
> #
> # Returns: Nothing on success
> # If an error occurs, GenericError with error message
Knowing that a creation failed due to a snapshot already existing by the
same name or id might be worth a distinct error class (do we already
have an error class that we could reuse?)
--
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: 620 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm
2012-08-15 19:49 ` Eric Blake
@ 2012-08-16 10:16 ` Pavel Hrdina
0 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-16 10:16 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On 08/15/2012 09:49 PM, Eric Blake wrote:
> On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
> I'm focusing my review more on the public interface (since that's what
> affects libvirt), and therefore glanced through 1 through 12 but did not
> pay close attention to them.
>
>> hmp-commands.hx | 2 +-
>> hmp.c | 10 ++++++++++
>> hmp.h | 1 +
>> qapi-schema.json | 19 +++++++++++++++++++
>> qmp-commands.hx | 29 +++++++++++++++++++++++++++++
>> savevm.c | 25 +++++++++----------------
>> sysemu.h | 1 -
>> 7 files changed, 69 insertions(+), 18 deletions(-)
>>
>> +
>> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
>> +{
>> + const char *name = qdict_get_try_str(qdict, "name");
> In the cover letter, you said "and for QMP you have to always provide
> name parameter" - but this says 'name' is optional and can still be
> empty (id only).
I said in cover letter that the last two patches introduce this
functionality.
>> +++ b/qapi-schema.json
>> @@ -2356,3 +2356,22 @@
>> # Since: 1.2.0
>> ##
>> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
>> +
>> +##
>> +# @vm-snapshot-save:
>> +#
>> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,
> 'tag' is a misnomer, since you list @name as the parameter 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: tag or id of new or existing snapshot
> Here, you document @name as required,
>
>> +#
>> +# Returns: Nothing on success
>> +# If an error occurs, GenericError with error message
>> +#
>> +# Since: 1.2
> We missed 1.2 hard freeze. This better be 1.3.
>
>> +##
>> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
> but here, you listed it as '*name' meaning optional. I'm okay with
> leaving name optional, provided that it means that you end up allocating
> the next unique id. But if that's the case, then returning nothing is
> wrong; this command needs to return { 'id':'int', '*name':'str' }, so
> that the user knows what snapshot just got allocated.
If you apply whole patch-series, then you have to for QMP always provide
@name.
As I said above, this behaviour is introduced by last two patches.
>> +SQMP
>> +vm-snapshot-save
>> +------
>> +
>> +Create a snapshot of the whole virtual machine. If '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.
>> +
>> +The VM is automatically stopped and resumed and saving a snapshot can take
>> +a long time.
> I don't like that this command can take a long time. Just today on
> #virt IRC, someone was complaining that 'savevm' HMP cannot be canceled.
> If we're going to create a new interface, it would be nicer to create a
> command that starts the save process and returns immediately, as well as
> commands to track progress, allow an early abort, and send an event on
> completion. The HMP 'savevm' interface can issue multiple QMP commands
> under the hood to continue with it's blocking behavior, but as long as
> we are fixing things, I think the QMP interface should be more powerful.
Well, I could try to do it...with this I'll probably introduce new
command vm-snapshot-save-cancel or vm-snaphsot-cancel as we have migrate
and migrate-cancel.
>> +
>> +Arguments:
>> +
>> +- "name": tag or id of new or existing snapshot
>> +
>> +Example:
>> +
>> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
>> +<- { "return": {} }
> Again, you need to return the id of the just-created snapshot.
>
>> @@ -2176,21 +2174,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) {
> Here's hoping later in the series updates this to make saner decisions.
> (Maybe I should peruse the entire series before commenting on
> individual patches?)
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots
2012-08-16 4:36 ` Eric Blake
@ 2012-08-16 10:17 ` Pavel Hrdina
0 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-16 10:17 UTC (permalink / raw)
To: Eric Blake; +Cc: Benoît Canet, qemu-devel
On 08/16/2012 06:36 AM, Eric Blake wrote:
> On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
>> hmp.c | 33 +++++++++++++++++++++++++++++++++
>> hmp.h | 1 +
>> monitor.c | 2 +-
>> qapi-schema.json | 34 ++++++++++++++++++++++++++++++++++
>> qmp-commands.hx | 30 ++++++++++++++++++++++++++++++
>> savevm.c | 51 ++++++++++++++++++++++++---------------------------
>> sysemu.h | 2 --
>> 7 files changed, 123 insertions(+), 30 deletions(-)
>>
>> @@ -1053,6 +1053,40 @@
>> { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
>>
>> ##
>> +# @SnapshotInfo:
>> +#
> We've got competition here:
> https://lists.gnu.org/archive/html/qemu-devel/2012-08/msg02961.html
>
> These two patches need to converge into a single design.
I can use his design.
>> +# Snapshot list. This structure contains list of snapshots for virtual machine.
>> +#
>> +# @id: id of the snapshot.
>> +#
>> +# @tag: human readable tag of the snapshot.
>> +#
>> +# @vm_size: size of the snapshot in Bytes.
> As this is a new QMP command, you should use '-', not '_'. Benoit's
> patch named this @vm-state-size.
>
>> +#
>> +# @date: date and time of the snapshot as unix timestamp.
>> +#
>> +# @vm_clock: time in the guest in nsecs.
> For online internal snapshots (those created by savevm), the vm clock
> offset makes sense. But for offline snapshots (those created by
> 'qemu-img snapshot', with no VM state), there is no vm clock offset.
> 'qemu-img info' lists 00:00:00.000 as a result, but I wonder if it would
> be better to leave this field (and vm-state-size) as #optional and omit
> them from the JSON for offline snapshots.
>
> And since qemu refuses to load offline snapshots, it might be worth an
> additional field in the JSON that says whether a snapshot is online or
> offline, to make it easier for the parsing application to determine
> whether it can be loaded or must go through qemu-img while the guest is
> offline (then again, keeping vm-state-size as non-optional, and checking
> for 0 size, somewhat covers this).
As you said, this is covered and I think that checking for 0 size is
good enough.
>> +#
>> +# Since: 1.2
> 1.3 (entire series...)
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter
2012-08-16 5:00 ` Eric Blake
@ 2012-08-16 10:19 ` Pavel Hrdina
0 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-08-16 10:19 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On 08/16/2012 07:00 AM, Eric Blake wrote:
> On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
>> HMP command "savevm" now takes extra optional force parameter to specifi whether
> s/specifi/specify/
>
>> replace existing snapshot or not.
>>
>> QMP command "vm-snapshot-save" has also extra optional force parameter and
>> name parameter isn't optional anymore.
> When HMP omits the name, what name are you using in it's place? Either
> a nameless snapshot is okay (and QMP must expose it), or it is an error
> (and HMP must now be passing a reasonable name).
>
> /me reads patch
>
> Oh, there was always a name; the default name was a timestamp, and you
> moved the timestamp creation into HMP.
>
>
>> +++ b/hmp-commands.hx
>> @@ -264,19 +264,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 = "name:s?,force:b?",
>> + .params = "[tag|id] [on|off]",
> Rather than sticking 'force' as an optional parameter at the end,
> instead you want to add '-f' as a flag at the beginning.
>
> .args_type = "force:-b,name:s?",
> .params = "[-f] name"
>
> By doing this, you no longer need 17/18.
Thanks, I'll rewrite it for v2.
>> + .help = "save a VM snapshot. To replace existing snapshot use force parameter.",
>> .mhandler.cmd = hmp_vm_snapshot_save,
>> },
>>
>> STEXI
>> @item savevm [@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 tag or ID, it is replaced. More info at
>> -@ref{vm_snapshots}.
>> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.
>> +If @var{tag} is provided, it is used as human readable identifier. If there is
>> +already a snapshot with the same tag, force argument need to be "yes" to
> s/force argument need/the force argument needs/
>
>> +++ b/qapi-schema.json
>> @@ -2394,21 +2394,23 @@
>> ##
>> # @vm-snapshot-save:
>> #
>> -# Create a snapshot of the whole virtual machine. If '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.
>> +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
>> +# human readable identifier. If there is already a snapshot with the same tag,
>> +# force argument need to be "yes" to replace it.
> s/need to be "yes"/needs to be 'true'/
>
>> #
>> # The VM is automatically stopped and resumed and saving a snapshot can take
>> # a long time.
>> #
>> -# @name: tag or id of new or existing snapshot
>> +# @name: tag of new or existing snapshot
> Given your new semantics, @name must be tag when @force is false; but
> the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1
> regardless of the name, all while keeping the old tag. Does the new HMP
> code do an id lookup, and pass in the correct @name to the QMP code?
> Should the QMP code allow the same semantics of being able to pass
> @force=true and @name=id instead of the normal creation of @name=tag?
You can override existing snapshot specified by id. I'll fix the
documentation.
>> +#
>> +# @force: specify whether existing snapshot is replaced or not
> Based on the JSON below, this needs an #optional marking, as well as
> mentioning that the default is false.
>
>> #
>> # Returns: Nothing on success
>> # If an error occurs, GenericError with error message
> Knowing that a creation failed due to a snapshot already existing by the
> same name or id might be worth a distinct error class (do we already
> have an error class that we could reuse?)
>
Regarding the Luiz's new error handling, for new errors we should use
one error class.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR
2012-08-15 7:41 ` [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR Pavel Hrdina
@ 2012-08-30 12:11 ` Luiz Capitulino
2012-09-06 8:34 ` Pavel Hrdina
0 siblings, 1 reply; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-30 12:11 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Wed, 15 Aug 2012 09:41:42 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> qerror.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/qerror.h b/qerror.h
> index d0a76a4..7e0bae7 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -120,6 +120,9 @@ void assert_no_error(Error *err);
> #define QERR_FEATURE_DISABLED \
> ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>
> +#define QERR_GENERIC_ERROR \
> + ERROR_CLASS_GENERIC_ERROR, "An (Errno %d) error has occurred"
> +
You should use error_setg() instead:
http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04980.html
There usage examples in the series introducing it. It would be better
to wait for it to be merged before you use it though, as it's always
possible for people to ask for changes.
> #define QERR_INVALID_BLOCK_FORMAT \
> ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-15 7:41 ` [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
@ 2012-08-30 14:47 ` Luiz Capitulino
2012-08-31 6:26 ` Markus Armbruster
2012-09-06 9:07 ` Pavel Hrdina
0 siblings, 2 replies; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-30 14:47 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: Kevin Wolf, qemu-devel, Markus Armbruster
On Wed, 15 Aug 2012 09:41:43 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block.c | 25 +++++++++++++++++--------
> block.h | 3 ++-
> block/qcow2-snapshot.c | 9 ++++++++-
> block/qcow2.h | 4 +++-
> block/rbd.c | 20 ++++++++++++++------
> block/sheepdog.c | 17 +++++++++--------
> block_int.h | 3 ++-
> qemu-img.c | 2 +-
> savevm.c | 2 +-
> 9 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/block.c b/block.c
> index 016858b..8bc49b7 100644
> --- a/block.c
> +++ b/block.c
> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
We should only use QERR_ macros for the errors listed in the ErrorClass enum
(except GenericError), all other errors should generally use error_setg(), like
this:
error_setg(errp, "device '%s' has no medium);
> + ret = -ENOMEDIUM;
And, usually, we should get rid of errno propagation. There are two cases here:
1. errno is propagated up so that upper layers can print a decent error
message to the user.
In this case, it's safe to eliminate errno. error_setg() will store a
decent message already and the Error object can be propagated up.
2. errno is propagated up so that upper layers can distinguish among
error causes and take different actions accordingly.
Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only
used to communicate the error to the user). However, I'm pretty sure that
such usage exists in qemu and the error API will break it, as most of our
errors are generic.
I see two solutions to this problem:
A. Add specific errors to ErrorClass. I don't like this very much,
as it's possible that such errors are going to be useful only internally.
B. Add two new functions:
void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...);
int error_get_errno(const Error **err);
So that we can maintain errno when it's used to communicate
error cause among functions.
> + } 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_set(errp, QERR_NOT_SUPPORTED);
> + ret = -ENOTSUP;
> + }
> +
> + return ret;
> }
>
> int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 2e2be11..92e782b 100644
> --- a/block.h
> +++ b/block.h
> @@ -296,7 +296,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..cf86dae 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,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>
> /* Check that the ID is unique */
> if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + "name", "non-existing id identifier");
> return -EEXIST;
> }
>
> @@ -415,6 +420,8 @@ fail:
> g_free(sn->name);
> g_free(l1_table);
>
> + error_set(errp, QERR_GENERIC_ERROR, ret);
> +
> return ret;
> }
>
> 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 5a0f79f..7bc42f0 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>
>
> @@ -817,12 +818,15 @@ 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_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + "name", "some tag identifier");
> return -EINVAL; /* we need a name for rbd snapshots */
> }
>
> @@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
> */
> if (sn_info->id_str[0] != '\0' &&
> strcmp(sn_info->id_str, sn_info->name) != 0) {
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + "name", "id and tag to equal");
> return -EINVAL;
> }
>
> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + "name", "shorter than 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_set(errp, QERR_GENERIC_ERROR, ret);
> + return ret;
You should always be careful when dropping error_report(), most of the time you
also have to change callers. In this case, bdrv_snapshot_create() is called
by qemu-img, which should be changed to print the error from the Error object.
> }
>
> return 0;
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index a04ad99..fc51e04 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
>
> @@ -1730,7 +1731,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;
> @@ -1743,9 +1746,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_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + "name", " not VDI snapshot");
> return -EINVAL;
> }
>
> @@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
> s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
> if (ret < 0) {
> - error_report("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));
> goto cleanup;
> }
>
> @@ -1785,7 +1784,6 @@ 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));
> goto cleanup;
> }
>
> @@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>
> cleanup:
> closesocket(fd);
> + if (ret < 0) {
> + error_set(errp, QERR_GENERIC_ERROR, ret);
> + }
> return ret;
> }
>
> diff --git a/block_int.h b/block_int.h
> index 4452f6f..b76c943 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -206,7 +206,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 b41e670..c798d66 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1267,7 +1267,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);
> + ret = bdrv_snapshot_create(bs, &sn, NULL);
> if (ret) {
> error_report("Could not create snapshot '%s': %d (%s)",
> snapshot_name, ret, strerror(-ret));
> diff --git a/savevm.c b/savevm.c
> index 0ea10c9..1b67af6 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2165,7 +2165,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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() and related functions
2012-08-15 7:41 ` [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
@ 2012-08-30 15:07 ` Luiz Capitulino
0 siblings, 0 replies; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-30 15:07 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Wed, 15 Aug 2012 09:41:44 +0200
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 | 6 +++++-
> block/sheepdog.c | 16 +++++++++-------
> block_int.h | 3 ++-
> qemu-img.c | 2 +-
> savevm.c | 2 +-
> 9 files changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8bc49b7..ad25184 100644
> --- a/block.c
> +++ b/block.c
> @@ -2683,29 +2683,33 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
> + ret = -ENOMEDIUM;
Most of the comments I made in 02/18 apply for this patch (and probably
the next ones too), so to summarize:
1. The QERR_ macros are deprecated, we should use error_setg() instead
2. As a general rule, only the Error object should be propagated up (ie.
we shouldn't propagate the Error object _and_ errno). It's debatable if
we can get rid of errno, if we can't then we'll have to extend the error API
to embed errno.
We have to discuss this with block layer guys, preferably before doing
large series.
One more comment below.
> + } 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_set(errp, QERR_OPEN_FILE_FAILED, bdrv_get_device_name(bs));
> + ret = open_ret;
> }
> - return ret;
> + } else {
> + error_set(errp, QERR_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 92e782b..11edcd3 100644
> --- a/block.h
> +++ b/block.h
> @@ -299,7 +299,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 cf86dae..8a87b0c 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -426,7 +426,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;
> @@ -438,13 +440,13 @@ 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_set(errp, QERR_OPEN_FILE_FAILED, 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_set(errp, QERR_NOT_SUPPORTED);
> ret = -ENOTSUP;
> goto fail;
> }
> @@ -536,6 +538,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>
> fail:
> g_free(sn_l1_table);
> + if (!error_is_set(errp)) {
> + error_set(errp, QERR_GENERIC_ERROR, ret);
> + }
> return ret;
> }
>
> 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 7bc42f0..5a73d8a 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -867,12 +867,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_set(errp, QERR_GENERIC_ERROR, r);
> + }
The Right Thing to do here is to add an Error argument to rbd_snap_rollback(),
as it knows the error cause. An alternative could be to do something like:
error_setg(errp, "can't roll back: %s\n", strerror(-r));
This could even be done by the higher layers. As I said above, I think
it's better to discuss with the block layer guys first how the block layer
should be "qapimized".
> return r;
> }
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fc51e04..557ff92 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1799,7 +1799,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;
> @@ -1824,13 +1826,12 @@ 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_set(errp, QERR_OPEN_FILE_FAILED, vdi);
> goto out;
> }
>
> fd = connect_to_sdog(s->addr, s->port);
> if (fd < 0) {
> - error_report("failed to connect");
> ret = fd;
> goto out;
> }
> @@ -1848,7 +1849,8 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> memcpy(&s->inode, buf, sizeof(s->inode));
>
> if (!s->inode.vm_state_size) {
> - error_report("Invalid snapshot");
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> + "name", "existing snapshot");
> ret = -ENOENT;
> goto out;
> }
> @@ -1864,9 +1866,9 @@ out:
> memcpy(s, old_s, sizeof(BDRVSheepdogState));
> g_free(buf);
> g_free(old_s);
> -
> - error_report("failed to open. recover old bdrv_sd_state.");
> -
> + if (!error_is_set(errp)) {
> + error_set(errp, QERR_GENERIC_ERROR, ret);
> + }
> return ret;
> }
>
> diff --git a/block_int.h b/block_int.h
> index b76c943..fa6d72f 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -209,7 +209,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 c798d66..1f10ff4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1275,7 +1275,7 @@ static int img_snapshot(int argc, char **argv)
> break;
>
> case SNAPSHOT_APPLY:
> - ret = bdrv_snapshot_goto(bs, snapshot_name);
> + ret = bdrv_snapshot_goto(bs, snapshot_name, NULL);
> if (ret) {
> error_report("Could not apply snapshot '%s': %d (%s)",
> snapshot_name, ret, strerror(-ret));
> diff --git a/savevm.c b/savevm.c
> index 1b67af6..0931e54 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2256,7 +2256,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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state()
2012-08-15 7:41 ` [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
@ 2012-08-30 17:09 ` Luiz Capitulino
2012-09-06 8:33 ` Pavel Hrdina
0 siblings, 1 reply; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-30 17:09 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Wed, 15 Aug 2012 09:41:53 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> migration.c | 2 +-
> savevm.c | 44 ++++++++++++++++++++++++++++----------------
> sysemu.h | 3 ++-
> 3 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index ec2f267..f048faf 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -88,7 +88,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
>
> void process_incoming_migration(QEMUFile *f)
> {
> - if (qemu_loadvm_state(f) < 0) {
> + if (qemu_loadvm_state(f, NULL) < 0) {
> fprintf(stderr, "load of migration failed\n");
> exit(0);
> }
> diff --git a/savevm.c b/savevm.c
> index 0d54115..500eb72 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1916,7 +1916,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);
> @@ -1925,21 +1926,26 @@ int qemu_loadvm_state(QEMUFile *f)
> unsigned int v;
> int ret;
>
> - if (qemu_savevm_state_blocked(NULL)) {
> - return -EINVAL;
> + if (qemu_savevm_state_blocked(errp)) {
> + return -ENOTSUP;
> }
>
> v = qemu_get_be32(f);
> - if (v != QEMU_VM_FILE_MAGIC)
> + if (v != QEMU_VM_FILE_MAGIC) {
> + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> + "Unknown vm-state 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_set(errp, QERR_NOT_SUPPORTED);
> return -ENOTSUP;
> }
> - if (v != QEMU_VM_FILE_VERSION)
> + if (v != QEMU_VM_FILE_VERSION) {
> + error_set(errp, QERR_NOT_SUPPORTED);
> return -ENOTSUP;
> + }
>
> while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
> uint32_t instance_id, version_id, section_id;
> @@ -1961,15 +1967,18 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> + "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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> + "savevm: unsupported version %d for '%s' v%d",
> + version_id, idstr, se->version_id);
> ret = -EINVAL;
> goto out;
> }
> @@ -1984,8 +1993,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_set(errp, QERR_GENERIC_ERROR, ret);
> goto out;
> }
> break;
> @@ -1999,20 +2007,21 @@ int qemu_loadvm_state(QEMUFile *f)
> }
> }
> if (le == NULL) {
> - fprintf(stderr, "Unknown savevm section %d\n", section_id);
> + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> + "Unknown savevm section %d", section_id);
You sure that this error message will be printed to the terminal? This has
to be done by the caller.
> 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_set(errp, QERR_GENERIC_ERROR, ret);
> goto out;
> }
> break;
> default:
> - fprintf(stderr, "Unknown savevm section type %d\n", section_type);
> + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> + "Unknown savevm section type %d", section_type);
> ret = -EINVAL;
> goto out;
> }
> @@ -2030,6 +2039,9 @@ out:
>
> if (ret == 0) {
> ret = qemu_file_get_error(f);
> + if (ret < 0) {
> + error_set(errp, QERR_GENERIC_ERROR, ret);
> + }
> }
>
> return ret;
> @@ -2297,7 +2309,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 35d6d67..651d96e 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -85,7 +85,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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
` (17 preceding siblings ...)
2012-08-15 7:41 ` [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter Pavel Hrdina
@ 2012-08-30 17:15 ` Luiz Capitulino
18 siblings, 0 replies; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-30 17:15 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: qemu-devel
On Wed, 15 Aug 2012 09:41:41 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:
> This patch series convert these commands into qapi and intruduce QMP commands
> vm-snapshot-save, vm-snapshot-load, vm-snapshot-delete and query-vm-snapshots.
> It also rewrite error report for function used by these commands.
Unfortunately, most of the error conversions are wrong. I've commented on
them, but the most important thing here is to decide how we should propagate
the Error object in the block layer and what to do with its errno usage.
It's better to discuss this first before doing large changes. It might also be
worth it to split this series and work on error propagation first.
I've CC'ed the block layer guys in one of my reviews to this series.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-30 14:47 ` Luiz Capitulino
@ 2012-08-31 6:26 ` Markus Armbruster
2012-08-31 12:18 ` Kevin Wolf
2012-08-31 13:09 ` Luiz Capitulino
2012-09-06 9:07 ` Pavel Hrdina
1 sibling, 2 replies; 41+ messages in thread
From: Markus Armbruster @ 2012-08-31 6:26 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Kevin Wolf, Pavel Hrdina, qemu-devel
Luiz Capitulino <lcapitulino@redhat.com> writes:
> On Wed, 15 Aug 2012 09:41:43 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> block.c | 25 +++++++++++++++++--------
>> block.h | 3 ++-
>> block/qcow2-snapshot.c | 9 ++++++++-
>> block/qcow2.h | 4 +++-
>> block/rbd.c | 20 ++++++++++++++------
>> block/sheepdog.c | 17 +++++++++--------
>> block_int.h | 3 ++-
>> qemu-img.c | 2 +-
>> savevm.c | 2 +-
>> 9 files changed, 57 insertions(+), 28 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 016858b..8bc49b7 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
>
> We should only use QERR_ macros for the errors listed in the ErrorClass enum
> (except GenericError), all other errors should generally use error_setg(), like
> this:
>
> error_setg(errp, "device '%s' has no medium);
>
>> + ret = -ENOMEDIUM;
>
> And, usually, we should get rid of errno propagation. There are two cases here:
The block layer consistently[*] uses -errno return values. Its
consistency is valuable, and I'm a bit reluctant to break it. Maybe a
new rule "returns -errno, except when it has an Error ** argument" could
work. I'd like to hear Kevin's advice on this.
>
> 1. errno is propagated up so that upper layers can print a decent error
> message to the user.
>
> In this case, it's safe to eliminate errno. error_setg() will store a
> decent message already and the Error object can be propagated up.
>
> 2. errno is propagated up so that upper layers can distinguish among
> error causes and take different actions accordingly.
>
> Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only
> used to communicate the error to the user). However, I'm pretty sure that
> such usage exists in qemu and the error API will break it, as most of our
> errors are generic.
>
> I see two solutions to this problem:
>
> A. Add specific errors to ErrorClass. I don't like this very much,
> as it's possible that such errors are going to be useful only internally.
Let's not reinvent errno, poorly.
> B. Add two new functions:
>
> void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...);
> int error_get_errno(const Error **err);
>
> So that we can maintain errno when it's used to communicate
> error cause among functions.
Better.
What's ErrorClass doing there?
[...]
[*] Almost; it's still QEMU after all.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-31 6:26 ` Markus Armbruster
@ 2012-08-31 12:18 ` Kevin Wolf
2012-08-31 13:13 ` Luiz Capitulino
2012-08-31 13:09 ` Luiz Capitulino
1 sibling, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2012-08-31 12:18 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Pavel Hrdina, qemu-devel, Luiz Capitulino
Am 31.08.2012 08:26, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> On Wed, 15 Aug 2012 09:41:43 +0200
>> Pavel Hrdina <phrdina@redhat.com> wrote:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>> block.c | 25 +++++++++++++++++--------
>>> block.h | 3 ++-
>>> block/qcow2-snapshot.c | 9 ++++++++-
>>> block/qcow2.h | 4 +++-
>>> block/rbd.c | 20 ++++++++++++++------
>>> block/sheepdog.c | 17 +++++++++--------
>>> block_int.h | 3 ++-
>>> qemu-img.c | 2 +-
>>> savevm.c | 2 +-
>>> 9 files changed, 57 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 016858b..8bc49b7 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
>>
>> We should only use QERR_ macros for the errors listed in the ErrorClass enum
>> (except GenericError), all other errors should generally use error_setg(), like
>> this:
>>
>> error_setg(errp, "device '%s' has no medium);
>>
>>> + ret = -ENOMEDIUM;
>>
>> And, usually, we should get rid of errno propagation. There are two cases here:
>
> The block layer consistently[*] uses -errno return values. Its
> consistency is valuable, and I'm a bit reluctant to break it. Maybe a
> new rule "returns -errno, except when it has an Error ** argument" could
> work. I'd like to hear Kevin's advice on this.
I'd rather not remove existing -errno returns if there's not good reason
why we can't provide them (or can't do so easily at least).
Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-31 6:26 ` Markus Armbruster
2012-08-31 12:18 ` Kevin Wolf
@ 2012-08-31 13:09 ` Luiz Capitulino
1 sibling, 0 replies; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-31 13:09 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, Pavel Hrdina, qemu-devel
On Fri, 31 Aug 2012 08:26:38 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> > On Wed, 15 Aug 2012 09:41:43 +0200
> > Pavel Hrdina <phrdina@redhat.com> wrote:
> >
> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >> ---
> >> block.c | 25 +++++++++++++++++--------
> >> block.h | 3 ++-
> >> block/qcow2-snapshot.c | 9 ++++++++-
> >> block/qcow2.h | 4 +++-
> >> block/rbd.c | 20 ++++++++++++++------
> >> block/sheepdog.c | 17 +++++++++--------
> >> block_int.h | 3 ++-
> >> qemu-img.c | 2 +-
> >> savevm.c | 2 +-
> >> 9 files changed, 57 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 016858b..8bc49b7 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
> >
> > We should only use QERR_ macros for the errors listed in the ErrorClass enum
> > (except GenericError), all other errors should generally use error_setg(), like
> > this:
> >
> > error_setg(errp, "device '%s' has no medium);
> >
> >> + ret = -ENOMEDIUM;
> >
> > And, usually, we should get rid of errno propagation. There are two cases here:
>
> The block layer consistently[*] uses -errno return values. Its
> consistency is valuable, and I'm a bit reluctant to break it. Maybe a
> new rule "returns -errno, except when it has an Error ** argument" could
> work. I'd like to hear Kevin's advice on this.
>
> >
> > 1. errno is propagated up so that upper layers can print a decent error
> > message to the user.
> >
> > In this case, it's safe to eliminate errno. error_setg() will store a
> > decent message already and the Error object can be propagated up.
> >
> > 2. errno is propagated up so that upper layers can distinguish among
> > error causes and take different actions accordingly.
> >
> > Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only
> > used to communicate the error to the user). However, I'm pretty sure that
> > such usage exists in qemu and the error API will break it, as most of our
> > errors are generic.
> >
> > I see two solutions to this problem:
> >
> > A. Add specific errors to ErrorClass. I don't like this very much,
> > as it's possible that such errors are going to be useful only internally.
>
> Let's not reinvent errno, poorly.
>
> > B. Add two new functions:
> >
> > void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...);
> > int error_get_errno(const Error **err);
> >
> > So that we can maintain errno when it's used to communicate
> > error cause among functions.
>
> Better.
>
> What's ErrorClass doing there?
There might be cases where we will have to report an error other than GenericError,
we could have error_sete_generic(), but this is starting to get weird :)
>
> [...]
>
>
> [*] Almost; it's still QEMU after all.
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-31 12:18 ` Kevin Wolf
@ 2012-08-31 13:13 ` Luiz Capitulino
0 siblings, 0 replies; 41+ messages in thread
From: Luiz Capitulino @ 2012-08-31 13:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Pavel Hrdina, Markus Armbruster, qemu-devel
On Fri, 31 Aug 2012 14:18:55 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 31.08.2012 08:26, schrieb Markus Armbruster:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >> On Wed, 15 Aug 2012 09:41:43 +0200
> >> Pavel Hrdina <phrdina@redhat.com> wrote:
> >>
> >>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >>> ---
> >>> block.c | 25 +++++++++++++++++--------
> >>> block.h | 3 ++-
> >>> block/qcow2-snapshot.c | 9 ++++++++-
> >>> block/qcow2.h | 4 +++-
> >>> block/rbd.c | 20 ++++++++++++++------
> >>> block/sheepdog.c | 17 +++++++++--------
> >>> block_int.h | 3 ++-
> >>> qemu-img.c | 2 +-
> >>> savevm.c | 2 +-
> >>> 9 files changed, 57 insertions(+), 28 deletions(-)
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index 016858b..8bc49b7 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
> >>
> >> We should only use QERR_ macros for the errors listed in the ErrorClass enum
> >> (except GenericError), all other errors should generally use error_setg(), like
> >> this:
> >>
> >> error_setg(errp, "device '%s' has no medium);
> >>
> >>> + ret = -ENOMEDIUM;
> >>
> >> And, usually, we should get rid of errno propagation. There are two cases here:
> >
> > The block layer consistently[*] uses -errno return values. Its
> > consistency is valuable, and I'm a bit reluctant to break it. Maybe a
> > new rule "returns -errno, except when it has an Error ** argument" could
> > work. I'd like to hear Kevin's advice on this.
>
> I'd rather not remove existing -errno returns if there's not good reason
> why we can't provide them (or can't do so easily at least).
Yes, I agree. Let me evaluate this calmly. I'll try to come with a better
proposal soon.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state()
2012-08-30 17:09 ` Luiz Capitulino
@ 2012-09-06 8:33 ` Pavel Hrdina
0 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-09-06 8:33 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
On 08/30/2012 07:09 PM, Luiz Capitulino wrote:
> On Wed, 15 Aug 2012 09:41:53 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> migration.c | 2 +-
>> savevm.c | 44 ++++++++++++++++++++++++++++----------------
>> sysemu.h | 3 ++-
>> 3 files changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index ec2f267..f048faf 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -88,7 +88,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
>>
>> void process_incoming_migration(QEMUFile *f)
>> {
>> - if (qemu_loadvm_state(f) < 0) {
>> + if (qemu_loadvm_state(f, NULL) < 0) {
>> fprintf(stderr, "load of migration failed\n");
>> exit(0);
>> }
>> diff --git a/savevm.c b/savevm.c
>> index 0d54115..500eb72 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -1916,7 +1916,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);
>> @@ -1925,21 +1926,26 @@ int qemu_loadvm_state(QEMUFile *f)
>> unsigned int v;
>> int ret;
>>
>> - if (qemu_savevm_state_blocked(NULL)) {
>> - return -EINVAL;
>> + if (qemu_savevm_state_blocked(errp)) {
>> + return -ENOTSUP;
>> }
>>
>> v = qemu_get_be32(f);
>> - if (v != QEMU_VM_FILE_MAGIC)
>> + if (v != QEMU_VM_FILE_MAGIC) {
>> + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> + "Unknown vm-state 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_set(errp, QERR_NOT_SUPPORTED);
>> return -ENOTSUP;
>> }
>> - if (v != QEMU_VM_FILE_VERSION)
>> + if (v != QEMU_VM_FILE_VERSION) {
>> + error_set(errp, QERR_NOT_SUPPORTED);
>> return -ENOTSUP;
>> + }
>>
>> while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
>> uint32_t instance_id, version_id, section_id;
>> @@ -1961,15 +1967,18 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> + "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_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> + "savevm: unsupported version %d for '%s' v%d",
>> + version_id, idstr, se->version_id);
>> ret = -EINVAL;
>> goto out;
>> }
>> @@ -1984,8 +1993,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_set(errp, QERR_GENERIC_ERROR, ret);
>> goto out;
>> }
>> break;
>> @@ -1999,20 +2007,21 @@ int qemu_loadvm_state(QEMUFile *f)
>> }
>> }
>> if (le == NULL) {
>> - fprintf(stderr, "Unknown savevm section %d\n", section_id);
>> + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> + "Unknown savevm section %d", section_id);
> You sure that this error message will be printed to the terminal? This has
> to be done by the caller.
While loading snapshot yes, but while migrating no. I think, that we
should rewrite migration error propagation too.
>
>> 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_set(errp, QERR_GENERIC_ERROR, ret);
>> goto out;
>> }
>> break;
>> default:
>> - fprintf(stderr, "Unknown savevm section type %d\n", section_type);
>> + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> + "Unknown savevm section type %d", section_type);
>> ret = -EINVAL;
>> goto out;
>> }
>> @@ -2030,6 +2039,9 @@ out:
>>
>> if (ret == 0) {
>> ret = qemu_file_get_error(f);
>> + if (ret < 0) {
>> + error_set(errp, QERR_GENERIC_ERROR, ret);
>> + }
>> }
>>
>> return ret;
>> @@ -2297,7 +2309,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 35d6d67..651d96e 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -85,7 +85,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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR
2012-08-30 12:11 ` Luiz Capitulino
@ 2012-09-06 8:34 ` Pavel Hrdina
0 siblings, 0 replies; 41+ messages in thread
From: Pavel Hrdina @ 2012-09-06 8:34 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
On 08/30/2012 02:11 PM, Luiz Capitulino wrote:
> On Wed, 15 Aug 2012 09:41:42 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> qerror.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/qerror.h b/qerror.h
>> index d0a76a4..7e0bae7 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -120,6 +120,9 @@ void assert_no_error(Error *err);
>> #define QERR_FEATURE_DISABLED \
>> ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>>
>> +#define QERR_GENERIC_ERROR \
>> + ERROR_CLASS_GENERIC_ERROR, "An (Errno %d) error has occurred"
>> +
> You should use error_setg() instead:
I'll fix it for whole patch-series.
>
> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04980.html
>
> There usage examples in the series introducing it. It would be better
> to wait for it to be merged before you use it though, as it's always
> possible for people to ask for changes.
>
>> #define QERR_INVALID_BLOCK_FORMAT \
>> ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
>>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-08-30 14:47 ` Luiz Capitulino
2012-08-31 6:26 ` Markus Armbruster
@ 2012-09-06 9:07 ` Pavel Hrdina
2012-09-10 17:03 ` Luiz Capitulino
1 sibling, 1 reply; 41+ messages in thread
From: Pavel Hrdina @ 2012-09-06 9:07 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Kevin Wolf, qemu-devel, Markus Armbruster
On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
> On Wed, 15 Aug 2012 09:41:43 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> block.c | 25 +++++++++++++++++--------
>> block.h | 3 ++-
>> block/qcow2-snapshot.c | 9 ++++++++-
>> block/qcow2.h | 4 +++-
>> block/rbd.c | 20 ++++++++++++++------
>> block/sheepdog.c | 17 +++++++++--------
>> block_int.h | 3 ++-
>> qemu-img.c | 2 +-
>> savevm.c | 2 +-
>> 9 files changed, 57 insertions(+), 28 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 016858b..8bc49b7 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
> We should only use QERR_ macros for the errors listed in the ErrorClass enum
> (except GenericError), all other errors should generally use error_setg(), like
> this:
>
> error_setg(errp, "device '%s' has no medium);
I agree that we should use error_setg() for all GenericError, but I
think that would be better have macros for errors which are used
many-times . For example:
#define QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM "Device '%s' has no medium"
and use error_setg(errp, QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM,
bdrv_get_device_name(bs));
I think that the consistency of error messages is important and an error
message should be the same for every error occurrence.
>
>> + ret = -ENOMEDIUM;
> And, usually, we should get rid of errno propagation. There are two cases here:
>
> 1. errno is propagated up so that upper layers can print a decent error
> message to the user.
>
> In this case, it's safe to eliminate errno. error_setg() will store a
> decent message already and the Error object can be propagated up.
>
> 2. errno is propagated up so that upper layers can distinguish among
> error causes and take different actions accordingly.
>
> Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only
> used to communicate the error to the user). However, I'm pretty sure that
> such usage exists in qemu and the error API will break it, as most of our
> errors are generic.
>
> I see two solutions to this problem:
>
> A. Add specific errors to ErrorClass. I don't like this very much,
> as it's possible that such errors are going to be useful only internally.
>
> B. Add two new functions:
>
> void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...);
> int error_get_errno(const Error **err);
>
> So that we can maintain errno when it's used to communicate
> error cause among functions.
>
>> + } 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_set(errp, QERR_NOT_SUPPORTED);
>> + ret = -ENOTSUP;
>> + }
>> +
>> + return ret;
>> }
>>
>> int bdrv_snapshot_goto(BlockDriverState *bs,
>> diff --git a/block.h b/block.h
>> index 2e2be11..92e782b 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -296,7 +296,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..cf86dae 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,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>
>> /* Check that the ID is unique */
>> if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
>> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> + "name", "non-existing id identifier");
>> return -EEXIST;
>> }
>>
>> @@ -415,6 +420,8 @@ fail:
>> g_free(sn->name);
>> g_free(l1_table);
>>
>> + error_set(errp, QERR_GENERIC_ERROR, ret);
>> +
>> return ret;
>> }
>>
>> 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 5a0f79f..7bc42f0 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>
>>
>> @@ -817,12 +818,15 @@ 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_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> + "name", "some tag identifier");
>> return -EINVAL; /* we need a name for rbd snapshots */
>> }
>>
>> @@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
>> */
>> if (sn_info->id_str[0] != '\0' &&
>> strcmp(sn_info->id_str, sn_info->name) != 0) {
>> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> + "name", "id and tag to equal");
>> return -EINVAL;
>> }
>>
>> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
>> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> + "name", "shorter than 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_set(errp, QERR_GENERIC_ERROR, ret);
>> + return ret;
> You should always be careful when dropping error_report(), most of the time you
> also have to change callers. In this case, bdrv_snapshot_create() is called
> by qemu-img, which should be changed to print the error from the Error object.
>
>> }
>>
>> return 0;
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index a04ad99..fc51e04 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
>>
>> @@ -1730,7 +1731,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;
>> @@ -1743,9 +1746,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_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> + "name", " not VDI snapshot");
>> return -EINVAL;
>> }
>>
>> @@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
>> s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
>> if (ret < 0) {
>> - error_report("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));
>> goto cleanup;
>> }
>>
>> @@ -1785,7 +1784,6 @@ 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));
>> goto cleanup;
>> }
>>
>> @@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>
>> cleanup:
>> closesocket(fd);
>> + if (ret < 0) {
>> + error_set(errp, QERR_GENERIC_ERROR, ret);
>> + }
>> return ret;
>> }
>>
>> diff --git a/block_int.h b/block_int.h
>> index 4452f6f..b76c943 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -206,7 +206,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 b41e670..c798d66 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1267,7 +1267,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);
>> + ret = bdrv_snapshot_create(bs, &sn, NULL);
>> if (ret) {
>> error_report("Could not create snapshot '%s': %d (%s)",
>> snapshot_name, ret, strerror(-ret));
>> diff --git a/savevm.c b/savevm.c
>> index 0ea10c9..1b67af6 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2165,7 +2165,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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
2012-09-06 9:07 ` Pavel Hrdina
@ 2012-09-10 17:03 ` Luiz Capitulino
0 siblings, 0 replies; 41+ messages in thread
From: Luiz Capitulino @ 2012-09-10 17:03 UTC (permalink / raw)
To: Pavel Hrdina; +Cc: Kevin Wolf, qemu-devel, Markus Armbruster
On Thu, 06 Sep 2012 11:07:54 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:
> On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
> > On Wed, 15 Aug 2012 09:41:43 +0200
> > Pavel Hrdina <phrdina@redhat.com> wrote:
> >
> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >> ---
> >> block.c | 25 +++++++++++++++++--------
> >> block.h | 3 ++-
> >> block/qcow2-snapshot.c | 9 ++++++++-
> >> block/qcow2.h | 4 +++-
> >> block/rbd.c | 20 ++++++++++++++------
> >> block/sheepdog.c | 17 +++++++++--------
> >> block_int.h | 3 ++-
> >> qemu-img.c | 2 +-
> >> savevm.c | 2 +-
> >> 9 files changed, 57 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 016858b..8bc49b7 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -2661,16 +2661,25 @@ 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_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
> > We should only use QERR_ macros for the errors listed in the ErrorClass enum
> > (except GenericError), all other errors should generally use error_setg(), like
> > this:
> >
> > error_setg(errp, "device '%s' has no medium);
>
> I agree that we should use error_setg() for all GenericError, but I
> think that would be better have macros for errors which are used
> many-times . For example:
>
> #define QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM "Device '%s' has no medium"
>
> and use error_setg(errp, QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM,
> bdrv_get_device_name(bs));
Hmm, no. The best way to refactor code that use the same error message
like the one above is to move the whole operation (with error handling
included) to a wrapper and use the wrapper instead.
Adding macros for error messages is a Bad Thing because soon or later
someone will create a new macro for a very similar but different error
message (which will lead to macro explosion) or will use an existing
macro that doesn't fit well to the failure being reported.
>
> I think that the consistency of error messages is important and an error
> message should be the same for every error occurrence.
> >
> >> + ret = -ENOMEDIUM;
> > And, usually, we should get rid of errno propagation. There are two cases here:
> >
> > 1. errno is propagated up so that upper layers can print a decent error
> > message to the user.
> >
> > In this case, it's safe to eliminate errno. error_setg() will store a
> > decent message already and the Error object can be propagated up.
> >
> > 2. errno is propagated up so that upper layers can distinguish among
> > error causes and take different actions accordingly.
> >
> > Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only
> > used to communicate the error to the user). However, I'm pretty sure that
> > such usage exists in qemu and the error API will break it, as most of our
> > errors are generic.
> >
> > I see two solutions to this problem:
> >
> > A. Add specific errors to ErrorClass. I don't like this very much,
> > as it's possible that such errors are going to be useful only internally.
> >
> > B. Add two new functions:
> >
> > void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...);
> > int error_get_errno(const Error **err);
> >
> > So that we can maintain errno when it's used to communicate
> > error cause among functions.
> >
> >> + } 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_set(errp, QERR_NOT_SUPPORTED);
> >> + ret = -ENOTSUP;
> >> + }
> >> +
> >> + return ret;
> >> }
> >>
> >> int bdrv_snapshot_goto(BlockDriverState *bs,
> >> diff --git a/block.h b/block.h
> >> index 2e2be11..92e782b 100644
> >> --- a/block.h
> >> +++ b/block.h
> >> @@ -296,7 +296,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..cf86dae 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,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> >>
> >> /* Check that the ID is unique */
> >> if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
> >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> >> + "name", "non-existing id identifier");
> >> return -EEXIST;
> >> }
> >>
> >> @@ -415,6 +420,8 @@ fail:
> >> g_free(sn->name);
> >> g_free(l1_table);
> >>
> >> + error_set(errp, QERR_GENERIC_ERROR, ret);
> >> +
> >> return ret;
> >> }
> >>
> >> 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 5a0f79f..7bc42f0 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>
> >>
> >> @@ -817,12 +818,15 @@ 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_set(errp, QERR_INVALID_PARAMETER_VALUE,
> >> + "name", "some tag identifier");
> >> return -EINVAL; /* we need a name for rbd snapshots */
> >> }
> >>
> >> @@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
> >> */
> >> if (sn_info->id_str[0] != '\0' &&
> >> strcmp(sn_info->id_str, sn_info->name) != 0) {
> >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> >> + "name", "id and tag to equal");
> >> return -EINVAL;
> >> }
> >>
> >> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> >> + "name", "shorter than 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_set(errp, QERR_GENERIC_ERROR, ret);
> >> + return ret;
> > You should always be careful when dropping error_report(), most of the time you
> > also have to change callers. In this case, bdrv_snapshot_create() is called
> > by qemu-img, which should be changed to print the error from the Error object.
> >
> >> }
> >>
> >> return 0;
> >> diff --git a/block/sheepdog.c b/block/sheepdog.c
> >> index a04ad99..fc51e04 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
> >>
> >> @@ -1730,7 +1731,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;
> >> @@ -1743,9 +1746,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_set(errp, QERR_INVALID_PARAMETER_VALUE,
> >> + "name", " not VDI snapshot");
> >> return -EINVAL;
> >> }
> >>
> >> @@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> >> ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
> >> s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
> >> if (ret < 0) {
> >> - error_report("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));
> >> goto cleanup;
> >> }
> >>
> >> @@ -1785,7 +1784,6 @@ 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));
> >> goto cleanup;
> >> }
> >>
> >> @@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> >>
> >> cleanup:
> >> closesocket(fd);
> >> + if (ret < 0) {
> >> + error_set(errp, QERR_GENERIC_ERROR, ret);
> >> + }
> >> return ret;
> >> }
> >>
> >> diff --git a/block_int.h b/block_int.h
> >> index 4452f6f..b76c943 100644
> >> --- a/block_int.h
> >> +++ b/block_int.h
> >> @@ -206,7 +206,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 b41e670..c798d66 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -1267,7 +1267,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);
> >> + ret = bdrv_snapshot_create(bs, &sn, NULL);
> >> if (ret) {
> >> error_report("Could not create snapshot '%s': %d (%s)",
> >> snapshot_name, ret, strerror(-ret));
> >> diff --git a/savevm.c b/savevm.c
> >> index 0ea10c9..1b67af6 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -2165,7 +2165,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] 41+ messages in thread
end of thread, other threads:[~2012-09-10 17:02 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-15 7:41 [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 01/18] qerror: introduce QERR_GENERIC_ERROR Pavel Hrdina
2012-08-30 12:11 ` Luiz Capitulino
2012-09-06 8:34 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions Pavel Hrdina
2012-08-30 14:47 ` Luiz Capitulino
2012-08-31 6:26 ` Markus Armbruster
2012-08-31 12:18 ` Kevin Wolf
2012-08-31 13:13 ` Luiz Capitulino
2012-08-31 13:09 ` Luiz Capitulino
2012-09-06 9:07 ` Pavel Hrdina
2012-09-10 17:03 ` Luiz Capitulino
2012-08-15 7:41 ` [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() " Pavel Hrdina
2012-08-30 15:07 ` Luiz Capitulino
2012-08-15 7:41 ` [Qemu-devel] [PATCH 04/18] block: add error parameter to bdrv_snapshot_delete() " Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 05/18] block: add error parameter to bdrv_snapshot_list() " Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 06/18] block: add error parameter to bdrv_snapshot_find() Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 07/18] block: add error parameter to del_existing_snapshots() Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 08/18] savevm: add error parameter to qemu_savevm_state_begin() Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 09/18] savevm: add error parameter to qemu_savevm_state_iterate() Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 10/18] savevm: add error parameter to qemu_savevm_state_complete() Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 11/18] savevm: add error parameter to qemu_savevm_state() Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state() Pavel Hrdina
2012-08-30 17:09 ` Luiz Capitulino
2012-09-06 8:33 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 13/18] qapi: Convert savevm Pavel Hrdina
2012-08-15 19:49 ` Eric Blake
2012-08-16 10:16 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm Pavel Hrdina
2012-08-16 3:31 ` Eric Blake
2012-08-16 4:34 ` Eric Blake
2012-08-15 7:41 ` [Qemu-devel] [PATCH 15/18] qapi: Convert delvm Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots Pavel Hrdina
2012-08-16 4:36 ` Eric Blake
2012-08-16 10:17 ` Pavel Hrdina
2012-08-15 7:41 ` [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional Pavel Hrdina
2012-08-16 4:39 ` Eric Blake
2012-08-15 7:41 ` [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter Pavel Hrdina
2012-08-16 5:00 ` Eric Blake
2012-08-16 10:19 ` Pavel Hrdina
2012-08-30 17:15 ` [Qemu-devel] [PATCH 00/18] qapi: Convert savevm, loadvm, delvm and info snapshots Luiz Capitulino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.