qapi: Convert savevm, loadvm, delvm and info snapshots
v2 cover qapi: Convert savevm, loadvm, delvm and info snapshots
This patch series convert these commands into qapi and introduce QMP commands vm-snapshot-save, vm-snapshot-load, vm-snapshot-delete and query-vm-snapshots. It also rewrite error report for functions used by these commands.
Last patch introduce new functionality of savevm and vm-snapshot-save that you cannot override existing snapshot without using 'force' parameter and for QMP you have to always provide name parameter.
We should also merge functionality of migrations and vm-snapshots and make some clean-ups of this code. I could do it as another patch. With this rewrite we could make vm-snapshots asynchronous.
Pavel Hrdina (17):
error: introduce handle_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
vm-snapshot-save: add force parameter
block.c | 106 ++++++++++++------ block.h | 13 ++- block/qcow2-snapshot.c | 41 ++++++- block/qcow2.h | 16 ++- block/rbd.c | 33 ++++-- block/sheepdog.c | 45 +++++--- block_int.h | 13 ++- error.c | 8 ++ error.h | 6 + hmp-commands.hx | 24 ++-- hmp.c | 84 ++++++++++++++ hmp.h | 4 + migration.c | 14 +-- monitor.c | 14 +-- qapi-schema.json | 64 +++++++++++ qemu-img.c | 28 ++--- qmp-commands.hx | 115 +++++++++++++++++++ savevm.c | 297 +++++++++++++++++++++++++------------------------ sysemu.h | 17 ++- vl.c | 6 +- 20 files changed, 663 insertions(+), 285 deletions(-)
Signature · 1 line
1.8.0.2
PATCH v2 1/17 error: introduce handle_error
Signed-off-by Pavel Hrdina <[email protected]>
--- error.c | 8 ++++++++ error.h | 6 ++++++ 2 files changed, 14 insertions(+)
error.c +8 -0
@@ -113,3 +113,11 @@ void error_propagate(Error **dst_err, Error *local_err)error_free (local_err); } } + +void handle_error (Error **errp) +{ +if (error_is_set (errp)) { +error_report ("%s" ,error_get_pretty (*errp)); +error_free (*errp); + } +}
error.h +6 -0
@@ -77,4 +77,10 @@ void error_propagate(Error **dst_err, Error *local_err);*/ void error_free (Error *err); +/** +* Print an error object as pretty string to current monitor or on stderr, then +* free the errot object. +*/ +void handle_error (Error **errp); +#endif
Signature · 1 line
1.8.0.2
PATCH v2 8/17 savevm: add error parameter to qemu_savevm_state_begin()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- migration.c | 2 +- savevm.c | 7 +++++-- sysemu.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-)
migration.c +1 -1
@@ -451,7 +451,7 @@ void migrate_fd_connect(MigrationState *s) s->file =qemu_fopen_ops_buffered (s);DPRINTF ("beginning savevm\n" ); - ret =qemu_savevm_state_begin (s->file, &s->params); + ret =qemu_savevm_state_begin (s->file, &s->params,NULL );if (ret <0 ) {DPRINTF ("failed, %d\n" , ret);migrate_fd_error (s);
savevm.c +5 -2
@@ -1601,7 +1601,8 @@ bool qemu_savevm_state_blocked(Error **errp) }int qemu_savevm_state_begin (QEMUFile *f, -const MigrationParams *params) +const MigrationParams *params, +Error **errp) {SaveStateEntry *se;int ret;
@@ -1641,12 +1642,14 @@ int qemu_savevm_state_begin(QEMUFile *f, ret = se->ops->save_live_setup (f, se->opaque);if (ret <0 ) { +error_setg (errp,"Failed to begin vmstate save." );qemu_savevm_state_cancel (f);return ret; } } ret =qemu_file_get_error (f);if (ret !=0 ) { +error_setg (errp,"%s" ,strerror (errno));qemu_savevm_state_cancel (f); }
@@ -1783,7 +1786,7 @@ static int qemu_savevm_state(QEMUFile *f) goto out; } - ret =qemu_savevm_state_begin (f, ¶ms); + ret =qemu_savevm_state_begin (f, ¶ms,NULL );if (ret <0 ) goto out;
sysemu.h +2 -1
@@ -74,7 +74,8 @@ void qemu_announce_self(void);bool qemu_savevm_state_blocked (Error **errp);int qemu_savevm_state_begin (QEMUFile *f, -const MigrationParams *params); +const MigrationParams *params, +Error **errp);int qemu_savevm_state_iterate (QEMUFile *f);int qemu_savevm_state_complete (QEMUFile *f);void qemu_savevm_state_cancel (QEMUFile *f);
Signature · 1 line
1.8.0.2
PATCH v2 7/17 block: add error parameter to del_existing_snapshots()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- savevm.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
savevm.c +4 -8
@@ -2094,22 +2094,18 @@ static int bdrv_snapshot_find(BlockDriverState *bs,/* * Deletes snapshots of a given name in all opened images. */ -static int del_existing_snapshots (Monitor *mon,const char *name) +static int del_existing_snapshots (const char *name, +Error **errp) {BlockDriverState *bs;QEMUSnapshotInfo sn1, *snapshot = &sn1; -int ret; bs =NULL ;while ((bs =bdrv_next (bs))) {if (bdrv_can_snapshot (bs) &&bdrv_snapshot_find (bs, snapshot, name,NULL ) >=0 ) { - ret =bdrv_snapshot_delete (bs, name,NULL ); -if (ret <0 ) { -monitor_printf (mon, -"Error while deleting snapshot on '%s'\n" , -bdrv_get_device_name (bs)); +if (bdrv_snapshot_delete (bs, name, errp) <0 ) {return -1 ; } }
@@ -2194,7 +2190,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) }/* Delete old snapshots of the same name */ -if (name &&del_existing_snapshots (mon, name) <0 ) { +if (name &&del_existing_snapshots (name,NULL ) <0 ) { goto the_end; }
Signature · 1 line
1.8.0.2
PATCH v2 3/17 block: add error parameter to bdrv_snapshot_goto() and related functions
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- block.c | 27 ++++++++++++++++----------- block.h | 3 ++- block/qcow2-snapshot.c | 15 ++++++++++++--- block/qcow2.h | 4 +++- block/rbd.c | 6 +++++- block/sheepdog.c | 14 +++++++------- block_int.h | 3 ++- qemu-img.c | 6 +----- savevm.c | 2 +- 9 files changed, 49 insertions(+), 31 deletions(-)
block.c +16 -11
@@ -3098,29 +3098,34 @@ int bdrv_snapshot_create(BlockDriverState *bs, }int bdrv_snapshot_goto (BlockDriverState *bs, -const char *snapshot_id) +const char *snapshot_id, +Error **errp) {BlockDriver *drv = bs->drv;int ret, open_ret; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_goto) -return drv->bdrv_snapshot_goto (bs, snapshot_id); - -if (bs->file) { +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ; + }else if (drv->bdrv_snapshot_goto) { + ret = drv->bdrv_snapshot_goto (bs, snapshot_id, errp); + }else if (bs->file) { drv->bdrv_close (bs); - ret =bdrv_snapshot_goto (bs->file, snapshot_id); + ret =bdrv_snapshot_goto (bs->file, snapshot_id, errp); open_ret = drv->bdrv_open (bs, bs->open_flags);if (open_ret <0 ) {bdrv_delete (bs->file); bs->drv =NULL ; -return open_ret; +error_setg (errp,"Failed to open '%s'." ,bdrv_get_device_name (bs)); + ret = open_ret; } -return ret; + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; } -return -ENOTSUP ; +return ret; }int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id)
block.h +2 -1
@@ -324,7 +324,8 @@ int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, Error **errp);int bdrv_snapshot_goto (BlockDriverState *bs, -const char *snapshot_id); +const char *snapshot_id, +Error **errp);int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id);int bdrv_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_info);
block/qcow2-snapshot.c +12 -3
@@ -428,7 +428,9 @@ fail: }/* copy the snapshot 'snapshot_name' into the current disk image */ -int qcow2_snapshot_goto (BlockDriverState *bs,const char *snapshot_id) +int qcow2_snapshot_goto (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {BDRVQcowState *s = bs->opaque;QCowSnapshot *sn;
@@ -440,13 +442,14 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)/* Search the snapshot */ snapshot_index =find_snapshot_by_id_or_name (bs, snapshot_id);if (snapshot_index <0 ) { +error_setg (errp,"Failed to find snapshot '%s' by id or name." , + snapshot_id);return -ENOENT ; } sn = &s->snapshots[snapshot_index];if (sn->disk_size != bs->total_sectors *BDRV_SECTOR_SIZE ) { -error_report ("qcow2: Loading snapshots with different disk " -"size is not implemented" ); +error_setg (errp,"Not supported." ); ret = -ENOTSUP ; goto fail; }
@@ -458,6 +461,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)*/ ret =qcow2_grow_l1_table (bs, sn->l1_size, true);if (ret <0 ) { +error_setg (errp,"Failed to grow L1 table." ); goto fail; }
@@ -476,18 +480,21 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) ret =bdrv_pread (bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);if (ret <0 ) { +error_setg (errp,"Failed to load data into L1 table." ); goto fail; } ret =qcow2_update_snapshot_refcount (bs, sn->l1_table_offset, sn->l1_size,1 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; } ret =bdrv_pwrite_sync (bs->file, s->l1_table_offset, sn_l1_table, cur_l1_bytes);if (ret <0 ) { +error_setg (errp,"Failed to save L1 table." ); goto fail; }
@@ -513,6 +520,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) }if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; }
@@ -525,6 +533,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)*/ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,0 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; }
block/qcow2.h +3 -1
@@ -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);
block/rbd.c +5 -1
@@ -858,12 +858,16 @@ static int qemu_rbd_snap_remove(BlockDriverState *bs, }static int qemu_rbd_snap_rollback (BlockDriverState *bs, -const char *snapshot_name) +const char *snapshot_name, +Error **errp) {BDRVRBDState *s = bs->opaque;int r; r =rbd_snap_rollback (s->image, snapshot_name); +if (r <0 ) { +error_setg (errp,"Failed to rollback snapshot." ); + }return r; }
block/sheepdog.c +7 -7
@@ -1808,7 +1808,9 @@ cleanup:return ret; } -static int sd_snapshot_goto (BlockDriverState *bs,const char *snapshot_id) +static int sd_snapshot_goto (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {BDRVSheepdogState *s = bs->opaque;BDRVSheepdogState *old_s;
@@ -1833,14 +1835,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) ret =find_vdi_name (s, vdi, snapid, tag, &vid,1 );if (ret) { -error_report ("Failed to find_vdi_name" ); +error_setg (errp,"Failed to find VDI snapshot '%s'." , vdi); goto out; } fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { -error_report ("failed to connect" ); ret = fd; +error_setg (errp,"Failed to connect to sdog." ); goto out; }
@@ -1851,13 +1853,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)closesocket (fd);if (ret) { +error_setg (errp,"Failed to open '%s'." , vdi); goto out; }memcpy (&s->inode, buf,sizeof (s->inode));if (!s->inode.vm_state_size) { -error_report ("Invalid snapshot" ); +error_setg (errp,"Invalid snapshot '%s'." , snapshot_id); ret = -ENOENT ; goto out; }
@@ -1873,9 +1876,6 @@ out:memcpy (s, old_s,sizeof (BDRVSheepdogState));g_free (buf);g_free (old_s); - -error_report ("failed to open. recover old bdrv_sd_state." ); -return ret; }
block_int.h +2 -1
@@ -150,7 +150,8 @@ struct BlockDriver { QEMUSnapshotInfo *sn_info, Error **errp);int (*bdrv_snapshot_goto)(BlockDriverState *bs, -const char *snapshot_id); +const char *snapshot_id, +Error **errp);int (*bdrv_snapshot_delete)(BlockDriverState *bs,const char *snapshot_id);int (*bdrv_snapshot_list)(BlockDriverState *bs,QEMUSnapshotInfo **psn_info);
qemu-img.c +1 -5
@@ -1564,11 +1564,7 @@ static int img_snapshot(int argc, char **argv)break ;case SNAPSHOT_APPLY : - ret =bdrv_snapshot_goto (bs, snapshot_name); -if (ret) { -error_report ("Could not apply snapshot '%s': %d (%s)" , - snapshot_name, ret,strerror (-ret)); - } + ret =bdrv_snapshot_goto (bs, snapshot_name, &err);break ;case SNAPSHOT_DELETE :
savevm.c +1 -1
@@ -2301,7 +2301,7 @@ int load_vmstate(const char *name) bs =NULL ;while ((bs =bdrv_next (bs))) {if (bdrv_can_snapshot (bs)) { - ret =bdrv_snapshot_goto (bs, name); + ret =bdrv_snapshot_goto (bs, name,NULL );if (ret <0 ) {error_report ("Error %d while activating snapshot '%s' on '%s'" , ret, name,bdrv_get_device_name (bs));
Signature · 1 line
1.8.0.2
PATCH v2 2/17 block: add error parameter to bdrv_snapshot_create() and related functions
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- block.c | 26 ++++++++++++++++++-------- block.h | 3 ++- block/qcow2-snapshot.c | 11 ++++++++++- block/qcow2.h | 4 +++- block/rbd.c | 17 +++++++++++------ block/sheepdog.c | 18 ++++++++++-------- block_int.h | 3 ++- qemu-img.c | 8 +++----- savevm.c | 2 +- 9 files changed, 60 insertions(+), 32 deletions(-)
block.c +18 -8
@@ -3075,16 +3075,26 @@ BlockDriverState *bdrv_snapshots(void) }int bdrv_snapshot_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info) +QEMUSnapshotInfo *sn_info, +Error **errp) {BlockDriver *drv = bs->drv; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_create) -return drv->bdrv_snapshot_create (bs, sn_info); -if (bs->file) -return bdrv_snapshot_create (bs->file, sn_info); -return -ENOTSUP ; +int ret; + +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ; + }else if (drv->bdrv_snapshot_create) { + ret = drv->bdrv_snapshot_create (bs, sn_info, errp); + }else if (bs->file) { + ret =bdrv_snapshot_create (bs->file, sn_info, errp); + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; + } + +return ret; } int bdrv_snapshot_goto(BlockDriverState *bs,
block.h +2 -1
@@ -321,7 +321,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);int bdrv_is_snapshot (BlockDriverState *bs);BlockDriverState *bdrv_snapshots (void );int bdrv_snapshot_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info); +QEMUSnapshotInfo *sn_info, +Error **errp);int bdrv_snapshot_goto (BlockDriverState *bs,const char *snapshot_id);int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id);
block/qcow2-snapshot.c +10 -1
@@ -25,6 +25,7 @@#include "qemu-common.h" #include "block_int.h" #include "block/qcow2.h" +#include "qerror.h" typedef struct QEMU_PACKED QCowSnapshotHeader {/* header is 8 byte aligned */
@@ -312,7 +313,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name) }/* if no id is provided, a new one is constructed */ -int qcow2_snapshot_create (BlockDriverState *bs,QEMUSnapshotInfo *sn_info) +int qcow2_snapshot_create (BlockDriverState *bs, +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVQcowState *s = bs->opaque;QCowSnapshot *new_snapshot_list =NULL ;
@@ -331,6 +334,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)/* Check that the ID is unique */ if (find_snapshot_by_id (bs, sn_info->id_str) >=0 ) { +error_setg (errp,"Parameter 'name' has to be unique ID." );return -EEXIST ; }
@@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) l1_table_offset =qcow2_alloc_clusters (bs, s->l1_size *sizeof (uint64_t ));if (l1_table_offset <0 ) { ret = l1_table_offset; +error_setg (errp,"Failed to allocate L1 table." ); goto fail; }
@@ -362,6 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) ret =bdrv_pwrite (bs->file, sn->l1_table_offset, l1_table, s->l1_size *sizeof (uint64_t ));if (ret <0 ) { +error_setg (errp,"Failed to save L1 table." ); goto fail; }
@@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)*/ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,1 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; } ret =bdrv_flush (bs);if (ret <0 ) { +error_setg (errp,"Failed to flush data." ); goto fail; }
@@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)if (ret <0 ) {g_free (s->snapshots); s->snapshots = old_snapshot_list; +error_setg (errp,"Failed to write new snapshots." ); goto fail; }
block/qcow2.h +3 -1
@@ -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);
block/rbd.c +11 -6
@@ -16,6 +16,7 @@#include "qemu-common.h" #include "qemu-error.h" #include "block_int.h" +#include "qerror.h" #include <rbd/librbd.h>
@@ -811,12 +812,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset) }static int qemu_rbd_snap_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info) +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVRBDState *s = bs->opaque; -int r; +int ret;if (sn_info->name[0 ] =='\0' ) { +error_setg (errp,"Parameter 'name' cannot be empty." );return -EINVAL ;/* we need a name for rbd snapshots */ }
@@ -826,17 +829,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,*/ if (sn_info->id_str[0 ] !='\0' &&strcmp (sn_info->id_str, sn_info->name) !=0 ) { +error_setg (errp,"ID and name have to be equal." );return -EINVAL ; }if (strlen (sn_info->name) >=sizeof (sn_info->id_str)) { +error_setg (errp,"Parameter 'name' has to be shorter that 127 chars." );return -ERANGE ; } - r =rbd_snap_create (s->image, sn_info->name); -if (r <0 ) { -error_report ("failed to create snap: %s" ,strerror (-r)); -return r; + ret =rbd_snap_create (s->image, sn_info->name); +if (ret <0 ) { +error_setg (errp,"Failed to create snapshot." ); +return ret; }return 0 ;
block/sheepdog.c +10 -8
@@ -17,6 +17,7 @@#include "qemu_socket.h" #include "block_int.h" #include "bitops.h" +#include "qerror.h" #define SD_PROTO_VER 0x01
@@ -1735,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)return 0 ; } -static int sd_snapshot_create (BlockDriverState *bs,QEMUSnapshotInfo *sn_info) +static int sd_snapshot_create (BlockDriverState *bs, +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVSheepdogState *s = bs->opaque;int ret, fd;
@@ -1748,9 +1751,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) s->name, sn_info->vm_state_size, s->is_snapshot);if (s->is_snapshot) { -error_report ("You can't create a snapshot of a snapshot VDI, " -"%s (%" PRIu32")." , s->name, s->inode.vdi_id); - +error_setg (errp,"You can't create a snapshot '%s' of a VDI snapshot." , + sn_info->name);return -EINVAL ; }
@@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { ret = fd; +error_setg (errp,"Failed to connect to sdog." ); goto cleanup; } ret =write_object (fd, (char *)&s->inode,vid_to_vdi_oid (s->inode.vdi_id), s->inode.nr_copies, datalen,0 , false, s->cache_enabled);if (ret <0 ) { -error_report ("failed to write snapshot's inode." ); +error_setg (errp,"Failed to write snapshot's inode." ); goto cleanup; } ret =do_sd_create (s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,1 , s->addr, s->port);if (ret <0 ) { -error_report ("failed to create inode for snapshot. %s" , -strerror (errno)); +error_setg (errp,"Failed to create inode for snapshot." ); goto cleanup; }
@@ -1793,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) s->inode.nr_copies, datalen, 0, s->cache_enabled);if (ret <0 ) { -error_report ("failed to read new inode info. %s" ,strerror (errno)); +error_setg (errp,"Failed to read new inode info." ); goto cleanup; }
block_int.h +2 -1
@@ -147,7 +147,8 @@ struct BlockDriver { const uint8_t *buf, int nb_sectors);int (*bdrv_snapshot_create)(BlockDriverState *bs, -QEMUSnapshotInfo *sn_info); +QEMUSnapshotInfo *sn_info, +Error **errp);int (*bdrv_snapshot_goto)(BlockDriverState *bs,const char *snapshot_id);int (*bdrv_snapshot_delete)(BlockDriverState *bs,const char *snapshot_id);
qemu-img.c +3 -5
@@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)int c, ret =0 , bdrv_oflags;int action =0 ;qemu_timeval tv; +Error *err =NULL ; bdrv_oflags =BDRV_O_FLAGS |BDRV_O_RDWR ;/* Parse commandline parameters */
@@ -1559,11 +1560,7 @@ static int img_snapshot(int argc, char **argv) sn.date_sec = tv.tv_sec; sn.date_nsec = tv.tv_usec *1000 ; - ret =bdrv_snapshot_create (bs, &sn); -if (ret) { -error_report ("Could not create snapshot '%s': %d (%s)" , - snapshot_name, ret,strerror (-ret)); - } + ret =bdrv_snapshot_create (bs, &sn, &err);break ;case SNAPSHOT_APPLY :
@@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)/* Cleanup */ bdrv_delete (bs); +handle_error (&err);if (ret) {return 1 ; }
savevm.c +1 -1
@@ -2211,7 +2211,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)if (bdrv_can_snapshot (bs1)) {/* Write VM state size only to the image that contains the state */ sn->vm_state_size = (bs == bs1 ? vm_state_size :0 ); - ret =bdrv_snapshot_create (bs1, sn); + ret =bdrv_snapshot_create (bs1, sn,NULL );if (ret <0 ) {monitor_printf (mon,"Error while creating snapshot on '%s'\n" ,bdrv_get_device_name (bs1));
Signature · 1 line
1.8.0.2
PATCH v2 5/17 block: add error parameter to bdrv_snapshot_list() and related functions
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- block.c | 26 ++++++++++++++++++-------- block.h | 3 ++- block/qcow2-snapshot.c | 5 ++++- block/qcow2.h | 4 +++- block/rbd.c | 4 +++- block/sheepdog.c | 9 +++++++-- block_int.h | 3 ++- qemu-img.c | 8 ++++++-- savevm.c | 4 ++-- 9 files changed, 47 insertions(+), 19 deletions(-)
block.c +18 -8
@@ -3152,16 +3152,26 @@ int bdrv_snapshot_delete(BlockDriverState *bs, }int bdrv_snapshot_list (BlockDriverState *bs, -QEMUSnapshotInfo **psn_info) +QEMUSnapshotInfo **psn_info, +Error **errp) {BlockDriver *drv = bs->drv; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_list) -return drv->bdrv_snapshot_list (bs, psn_info); -if (bs->file) -return bdrv_snapshot_list (bs->file, psn_info); -return -ENOTSUP ; +int ret; + +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ; + }else if (drv->bdrv_snapshot_list) { + ret = drv->bdrv_snapshot_list (bs, psn_info, errp); + }else if (bs->file) { + ret =bdrv_snapshot_list (bs->file, psn_info, errp); + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; + } + +return ret; } int bdrv_snapshot_load_tmp(BlockDriverState *bs,
block.h +2 -1
@@ -330,7 +330,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs,const char *snapshot_id, Error **errp);int bdrv_snapshot_list (BlockDriverState *bs, -QEMUSnapshotInfo **psn_info); +QEMUSnapshotInfo **psn_info, +Error **errp);int bdrv_snapshot_load_tmp (BlockDriverState *bs,const char *snapshot_name);char *bdrv_snapshot_dump (char *buf,int buf_size,QEMUSnapshotInfo *sn);
block/qcow2-snapshot.c +4 -1
@@ -614,7 +614,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs,return 0 ; } -int qcow2_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_tab) +int qcow2_snapshot_list (BlockDriverState *bs, +QEMUSnapshotInfo **psn_tab, +Error **errp) {BDRVQcowState *s = bs->opaque;QEMUSnapshotInfo *sn_tab, *sn_info;
@@ -623,6 +625,7 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)if (!s->nb_snapshots) { *psn_tab =NULL ; +error_setg (errp,"No snapshot list find." );return s->nb_snapshots; }
block/qcow2.h +3 -1
@@ -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);
block/rbd.c +3 -1
@@ -876,7 +876,8 @@ static int qemu_rbd_snap_rollback(BlockDriverState *bs, }static int qemu_rbd_snap_list (BlockDriverState *bs, -QEMUSnapshotInfo **psn_tab) +QEMUSnapshotInfo **psn_tab, +Error **errp) {BDRVRBDState *s = bs->opaque;QEMUSnapshotInfo *sn_info, *sn_tab =NULL ;
@@ -893,6 +894,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs, }while (snap_count == -ERANGE );if (snap_count <=0 ) { +error_setg (errp,"No snapshot list find." ); goto done; }
block/sheepdog.c +7 -2
@@ -1887,7 +1887,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,return 0 ; } -static int sd_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_tab) +static int sd_snapshot_list (BlockDriverState *bs, +QEMUSnapshotInfo **psn_tab, +Error **errp) {BDRVSheepdogState *s = bs->opaque;SheepdogReq req;
@@ -1905,6 +1907,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { +error_setg (errp,"Failed to connect to sdog." ); ret = fd; goto out; }
@@ -1921,6 +1924,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)closesocket (fd);if (ret) { +error_setg (errp,"No snapshot list find." ); goto out; }
@@ -1932,7 +1936,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { -error_report ("failed to connect" ); +error_setg (errp,"Failed to connect to sdog." ); ret = fd; goto out; }
@@ -1973,6 +1977,7 @@ out:g_free (vdi_inuse);if (ret <0 ) { +error_setg (errp,"%s" ,strerror (errno));return ret; }
block_int.h +2 -1
@@ -156,7 +156,8 @@ struct BlockDriver {const char *snapshot_id, Error **errp);int (*bdrv_snapshot_list)(BlockDriverState *bs, -QEMUSnapshotInfo **psn_info); +QEMUSnapshotInfo **psn_info, +Error **errp);int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,const char *snapshot_name);int (*bdrv_get_info)(BlockDriverState *bs,BlockDriverInfo *bdi);
qemu-img.c +6 -2
@@ -1099,8 +1099,10 @@ static void dump_snapshots(BlockDriverState *bs)QEMUSnapshotInfo *sn_tab, *sn;int nb_sns, i;char buf[256 ]; +Error *err =NULL ; - nb_sns =bdrv_snapshot_list (bs, &sn_tab); + nb_sns =bdrv_snapshot_list (bs, &sn_tab, &err); +handle_error (&err);if (nb_sns <=0 )return ;printf ("Snapshot list:\n" );
@@ -1134,7 +1136,9 @@ static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)int i, sn_count;QEMUSnapshotInfo *sn_tab =NULL ;SnapshotInfoList *info_list, *cur_item =NULL ; - sn_count =bdrv_snapshot_list (bs, &sn_tab); +Error *err =NULL ; + sn_count =bdrv_snapshot_list (bs, &sn_tab, &err); +handle_error (&err);for (i =0 ; i < sn_count; i++) { info->has_snapshots = true;
savevm.c +2 -2
@@ -2068,7 +2068,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,int nb_sns, i, ret; ret = -ENOENT ; - nb_sns =bdrv_snapshot_list (bs, &sn_tab); + nb_sns =bdrv_snapshot_list (bs, &sn_tab,NULL );if (nb_sns <0 )return ret;for (i =0 ; i < nb_sns; i++) {
@@ -2373,7 +2373,7 @@ void do_info_snapshots(Monitor *mon)return ; } - nb_sns =bdrv_snapshot_list (bs, &sn_tab); + nb_sns =bdrv_snapshot_list (bs, &sn_tab,NULL );if (nb_sns <0 ) {monitor_printf (mon,"bdrv_snapshot_list: error %d\n" , nb_sns);return ;
Signature · 1 line
1.8.0.2
PATCH v2 6/17 block: add error parameter to bdrv_snapshot_find()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- savevm.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
savevm.c +18 -10
@@ -2061,16 +2061,21 @@ out:return ret; } -static int bdrv_snapshot_find (BlockDriverState *bs,QEMUSnapshotInfo *sn_info, -const char *name) +static int bdrv_snapshot_find (BlockDriverState *bs, +QEMUSnapshotInfo *sn_info, +const char *name, +Error **errp) {QEMUSnapshotInfo *sn_tab, *sn;int nb_sns, i, ret; + + nb_sns =bdrv_snapshot_list (bs, &sn_tab, errp); +if (error_is_set (errp)) { +return nb_sns; + } + ret = -ENOENT ; - nb_sns =bdrv_snapshot_list (bs, &sn_tab,NULL ); -if (nb_sns <0 ) -return ret;for (i =0 ; i < nb_sns; i++) { sn = &sn_tab[i];if (!strcmp (sn->id_str, name) || !strcmp (sn->name, name)) {
@@ -2080,6 +2085,9 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, } }g_free (sn_tab); +if (ret <0 ) { +error_setg (errp,"Failed to find snapshot '%s'." , name); + }return ret; }
@@ -2095,7 +2103,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) bs =NULL ;while ((bs =bdrv_next (bs))) {if (bdrv_can_snapshot (bs) && -bdrv_snapshot_find (bs, snapshot, name) >=0 ) +bdrv_snapshot_find (bs, snapshot, name,NULL ) >=0 ) { ret =bdrv_snapshot_delete (bs, name,NULL );if (ret <0 ) {
@@ -2166,7 +2174,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) sn->vm_clock_nsec =qemu_get_clock_ns (vm_clock);if (name) { - ret =bdrv_snapshot_find (bs, old_sn, name); + ret =bdrv_snapshot_find (bs, old_sn, name,NULL );if (ret >=0 ) {pstrcpy (sn->name,sizeof (sn->name), old_sn->name);pstrcpy (sn->id_str,sizeof (sn->id_str), old_sn->id_str);
@@ -2263,7 +2271,7 @@ int load_vmstate(const char *name) }/* Don't even try to load empty VM states */ - ret =bdrv_snapshot_find (bs_vm_state, &sn, name); + ret =bdrv_snapshot_find (bs_vm_state, &sn, name,NULL );if (ret <0 ) {return ret; }else if (sn.vm_state_size ==0 ) {
@@ -2287,7 +2295,7 @@ int load_vmstate(const char *name)return -ENOTSUP ; } - ret =bdrv_snapshot_find (bs, &sn, name); + ret =bdrv_snapshot_find (bs, &sn, name,NULL );if (ret <0 ) {error_report ("Device '%s' does not have the requested snapshot '%s'" ,bdrv_get_device_name (bs), name);
@@ -2393,7 +2401,7 @@ void do_info_snapshots(Monitor *mon)while ((bs1 =bdrv_next (bs1))) {if (bdrv_can_snapshot (bs1) && bs1 != bs) { - ret =bdrv_snapshot_find (bs1, sn_info, sn->id_str); + ret =bdrv_snapshot_find (bs1, sn_info, sn->id_str,NULL );if (ret <0 ) { available =0 ;break ;
Signature · 1 line
1.8.0.2
PATCH v2 12/17 savevm: add error parameter to qemu_loadvm_state()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- migration.c | 8 ++++---- savevm.c | 39 +++++++++++++++++++++++---------------- sysemu.h | 3 ++- 3 files changed, 29 insertions(+), 21 deletions(-)
migration.c +4 -4
@@ -86,13 +86,13 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)static void process_incoming_migration_co (void *opaque) {QEMUFile *f = opaque; -int ret; +Error **errp =NULL ; - ret =qemu_loadvm_state (f); +qemu_loadvm_state (f, errp);qemu_set_fd_handler (qemu_get_fd (f),NULL ,NULL ,NULL );qemu_fclose (f); -if (ret <0 ) { -fprintf (stderr,"load of migration failed\n" ); +if (error_is_set (errp)) { +handle_error (errp);exit (0 ); }qemu_announce_self ();
savevm.c +23 -16
@@ -1962,7 +1962,8 @@ typedef struct LoadStateEntry {int version_id; } LoadStateEntry; -int qemu_loadvm_state (QEMUFile *f) +int qemu_loadvm_state (QEMUFile *f, +Error **errp) { QLIST_HEAD(, LoadStateEntry) loadvm_handlers =QLIST_HEAD_INITIALIZER (loadvm_handlers);
@@ -1971,21 +1972,25 @@ int qemu_loadvm_state(QEMUFile *f)unsigned int v;int ret; -if (qemu_savevm_state_blocked (NULL )) { -return -EINVAL ; +if (qemu_savevm_state_blocked (errp)) { +return -ENOTSUP ; } v =qemu_get_be32 (f); -if (v !=QEMU_VM_FILE_MAGIC ) +if (v !=QEMU_VM_FILE_MAGIC ) { +error_setg (errp,"Unknown vmstate file magic" );return -EINVAL ; + } v =qemu_get_be32 (f);if (v ==QEMU_VM_FILE_VERSION_COMPAT ) { -fprintf (stderr,"SaveVM v2 format is obsolete and don't work anymore\n" ); +error_setg (errp,"Not supported." );return -ENOTSUP ; } -if (v !=QEMU_VM_FILE_VERSION ) +if (v !=QEMU_VM_FILE_VERSION ) { +error_setg (errp,"Not supported." );return -ENOTSUP ; + }while ((section_type =qemu_get_byte (f)) !=QEMU_VM_EOF ) {uint32_t instance_id, version_id, section_id;
@@ -2007,15 +2012,16 @@ int qemu_loadvm_state(QEMUFile *f)/* Find savevm section */ se =find_se (idstr, instance_id);if (se ==NULL ) { -fprintf (stderr,"Unknown savevm section or instance '%s' %d\n" , idstr, instance_id); +error_setg (errp,"Unknown savevm section or instance '%s' %d" , + idstr, instance_id); ret = -EINVAL ; goto out; }/* Validate version */ if (version_id > se->version_id) { -fprintf (stderr,"savevm: unsupported version %d for '%s' v%d\n" , - version_id, idstr, se->version_id); +error_setg (errp,"savevm: unsupported version %d for '%s' v%d" , + version_id, idstr, se->version_id); ret = -EINVAL ; goto out; }
@@ -2030,8 +2036,7 @@ int qemu_loadvm_state(QEMUFile *f) ret =vmstate_load (f, le->se, le->version_id);if (ret <0 ) { -fprintf (stderr,"qemu: warning: error while loading state for instance 0x%x of device '%s'\n" , - instance_id, idstr); +error_setg (errp,"Failed to load vmstate." ); goto out; }break ;
@@ -2045,20 +2050,19 @@ int qemu_loadvm_state(QEMUFile *f) } }if (le ==NULL ) { -fprintf (stderr,"Unknown savevm section %d\n" , section_id); +error_setg (errp,"Unknown savevm section %d" , section_id); ret = -EINVAL ; goto out; } ret =vmstate_load (f, le->se, le->version_id);if (ret <0 ) { -fprintf (stderr,"qemu: warning: error while loading state section id %d\n" , - section_id); +error_setg (errp,"Failed to load vmstate." ); goto out; }break ;default : -fprintf (stderr,"Unknown savevm section type %d\n" , section_type); +error_setg (errp,"Unknown savevm section type %d" , section_type); ret = -EINVAL ; goto out; }
@@ -2076,6 +2080,9 @@ out:if (ret ==0 ) { ret =qemu_file_get_error (f); +if (ret <0 ) { +error_set (errp,QERR_GENERIC_ERROR , ret); + } }return ret;
@@ -2342,7 +2349,7 @@ int load_vmstate(const char *name) }qemu_system_reset (VMRESET_SILENT ); - ret =qemu_loadvm_state (f); + ret =qemu_loadvm_state (f,NULL );qemu_fclose (f);if (ret <0 ) {
sysemu.h +2 -1
@@ -81,7 +81,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,int qemu_savevm_state_complete (QEMUFile *f,Error **errp);void qemu_savevm_state_cancel (QEMUFile *f); -int qemu_loadvm_state (QEMUFile *f); +int qemu_loadvm_state (QEMUFile *f, +Error **errp);/* SLIRP */ void do_info_slirp (Monitor *mon);
Signature · 1 line
1.8.0.2
PATCH v2 4/17 block: add error parameter to bdrv_snapshot_delete() and related functions
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- block.c | 27 +++++++++++++++++++-------- block.h | 4 +++- block/qcow2-snapshot.c | 10 +++++++++- block/qcow2.h | 4 +++- block/rbd.c | 6 +++++- block/sheepdog.c | 4 +++- block_int.h | 4 +++- qemu-img.c | 6 +----- savevm.c | 4 ++-- 9 files changed, 48 insertions(+), 21 deletions(-)
block.c +19 -8
@@ -3128,16 +3128,27 @@ int bdrv_snapshot_goto(BlockDriverState *bs,return ret; } -int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id) +int bdrv_snapshot_delete (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {BlockDriver *drv = bs->drv; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_delete) -return drv->bdrv_snapshot_delete (bs, snapshot_id); -if (bs->file) -return bdrv_snapshot_delete (bs->file, snapshot_id); -return -ENOTSUP ; +int ret; + +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ; + }else if (drv->bdrv_snapshot_delete) { + ret = drv->bdrv_snapshot_delete (bs, snapshot_id, errp); + }else if (bs->file) { + ret =bdrv_snapshot_delete (bs->file, snapshot_id, errp); + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; + } + +return ret; } int bdrv_snapshot_list(BlockDriverState *bs,
block.h +3 -1
@@ -326,7 +326,9 @@ int bdrv_snapshot_create(BlockDriverState *bs,int bdrv_snapshot_goto (BlockDriverState *bs,const char *snapshot_id,Error **errp); -int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id); +int bdrv_snapshot_delete (BlockDriverState *bs, +const char *snapshot_id, +Error **errp);int bdrv_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs,
block/qcow2-snapshot.c +9 -1
@@ -550,7 +550,9 @@ fail:return ret; } -int qcow2_snapshot_delete (BlockDriverState *bs,const char *snapshot_id) +int qcow2_snapshot_delete (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {BDRVQcowState *s = bs->opaque;QCowSnapshot sn;
@@ -559,6 +561,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)/* Search the snapshot */ snapshot_index =find_snapshot_by_id_or_name (bs, snapshot_id);if (snapshot_index <0 ) { +error_setg (errp,"Failed to find snapshot '%s' by id or name." , + snapshot_id);return -ENOENT ; } sn = s->snapshots[snapshot_index];
@@ -570,6 +574,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) s->nb_snapshots--; ret =qcow2_write_snapshots (bs);if (ret <0 ) { +error_setg (errp,"Failed to remove snapshot '%s' from snapshot list" , + snapshot_id);return ret; }
@@ -587,6 +593,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) ret =qcow2_update_snapshot_refcount (bs, sn.l1_table_offset, sn.l1_size,-1 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." );return ret; }qcow2_free_clusters (bs, sn.l1_table_offset, sn.l1_size *sizeof (uint64_t ));
@@ -594,6 +601,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)/* must update the copied flag on the current cluster offsets */ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,0 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." );return ret; }
block/qcow2.h +3 -1
@@ -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);
block/rbd.c +5 -1
@@ -848,12 +848,16 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, }static int qemu_rbd_snap_remove (BlockDriverState *bs, -const char *snapshot_name) +const char *snapshot_name, +Error **errp) {BDRVRBDState *s = bs->opaque;int r; r =rbd_snap_remove (s->image, snapshot_name); +if (r <0 ) { +error_setg (errp,"Failed to remove snapshot." ); + }return r; }
block/sheepdog.c +3 -1
@@ -1879,7 +1879,9 @@ out:return ret; } -static int sd_snapshot_delete (BlockDriverState *bs,const char *snapshot_id) +static int sd_snapshot_delete (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {/* FIXME: Delete specified snapshot id. */ return 0 ;
block_int.h +3 -1
@@ -152,7 +152,9 @@ struct BlockDriver {int (*bdrv_snapshot_goto)(BlockDriverState *bs,const char *snapshot_id,Error **errp); -int (*bdrv_snapshot_delete)(BlockDriverState *bs,const char *snapshot_id); +int (*bdrv_snapshot_delete)(BlockDriverState *bs, +const char *snapshot_id, +Error **errp);int (*bdrv_snapshot_list)(BlockDriverState *bs,QEMUSnapshotInfo **psn_info); int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
qemu-img.c +1 -5
@@ -1568,11 +1568,7 @@ static int img_snapshot(int argc, char **argv)break ;case SNAPSHOT_DELETE : - ret =bdrv_snapshot_delete (bs, snapshot_name); -if (ret) { -error_report ("Could not delete snapshot '%s': %d (%s)" , - snapshot_name, ret,strerror (-ret)); - } + ret =bdrv_snapshot_delete (bs, snapshot_name, &err);break ; }
savevm.c +2 -2
@@ -2097,7 +2097,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)if (bdrv_can_snapshot (bs) &&bdrv_snapshot_find (bs, snapshot, name) >=0 ) { - ret =bdrv_snapshot_delete (bs, name); + ret =bdrv_snapshot_delete (bs, name,NULL );if (ret <0 ) { monitor_printf(mon, "Error while deleting snapshot on '%s'\n",
@@ -2344,7 +2344,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) bs1 =NULL ;while ((bs1 =bdrv_next (bs1))) {if (bdrv_can_snapshot (bs1)) { - ret =bdrv_snapshot_delete (bs1, name); + ret =bdrv_snapshot_delete (bs1, name,NULL );if (ret <0 ) {if (ret == -ENOTSUP ) monitor_printf(mon,
Signature · 1 line
1.8.0.2
PATCH v2 9/17 savevm: add error parameter to qemu_savevm_state_iterate()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- migration.c | 2 +- savevm.c | 11 +++++++++-- sysemu.h | 3 ++- 3 files changed, 12 insertions(+), 4 deletions(-)
migration.c +1 -1
@@ -341,7 +341,7 @@ void migrate_fd_put_ready(MigrationState *s) }DPRINTF ("iterate\n" ); - ret =qemu_savevm_state_iterate (s->file); + ret =qemu_savevm_state_iterate (s->file,NULL );if (ret <0 ) {migrate_fd_error (s); }else if (ret ==1 ) {
savevm.c +9 -2
@@ -1663,7 +1663,8 @@ int qemu_savevm_state_begin(QEMUFile *f,* 0 : We haven't finished, caller have to go again * 1 : We have finished, we can go to complete phase */ -int qemu_savevm_state_iterate (QEMUFile *f) +int qemu_savevm_state_iterate (QEMUFile *f, +Error **errp) {SaveStateEntry *se;int ret =1 ;
@@ -1697,10 +1698,16 @@ int qemu_savevm_state_iterate(QEMUFile *f) } }if (ret !=0 ) { +if (ret <0 ) { +error_setg (errp,"Failed to iterate vmstate save." ); + }return ret; } ret =qemu_file_get_error (f);if (ret !=0 ) { +if (ret <0 ) { +error_setg (errp,"%s" ,strerror (errno)); + }qemu_savevm_state_cancel (f); }return ret;
@@ -1791,7 +1798,7 @@ static int qemu_savevm_state(QEMUFile *f) goto out;do { - ret =qemu_savevm_state_iterate (f); + ret =qemu_savevm_state_iterate (f,NULL );if (ret <0 ) goto out; }while (ret ==0 );
sysemu.h +2 -1
@@ -76,7 +76,8 @@ bool qemu_savevm_state_blocked(Error **errp);int qemu_savevm_state_begin (QEMUFile *f,const MigrationParams *params,Error **errp); -int qemu_savevm_state_iterate (QEMUFile *f); +int qemu_savevm_state_iterate (QEMUFile *f, +Error **errp);int qemu_savevm_state_complete (QEMUFile *f);void qemu_savevm_state_cancel (QEMUFile *f);int qemu_loadvm_state (QEMUFile *f);
Signature · 1 line
1.8.0.2
PATCH v2 11/17 savevm: add error parameter to qemu_savevm_state()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- savevm.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
savevm.c +11 -8
@@ -1787,7 +1787,8 @@ void qemu_savevm_state_cancel(QEMUFile *f) } } -static int qemu_savevm_state (QEMUFile *f) +static int qemu_savevm_state (QEMUFile *f, +Error **errp) {int ret;MigrationParams params = {
@@ -1795,22 +1796,24 @@ static int qemu_savevm_state(QEMUFile *f) .shared = 0 }; -if (qemu_savevm_state_blocked (NULL )) { +if (qemu_savevm_state_blocked (errp)) { ret = -EINVAL ; goto out; } - ret =qemu_savevm_state_begin (f, ¶ms,NULL ); -if (ret <0 ) + ret =qemu_savevm_state_begin (f, ¶ms, errp); +if (ret <0 ) { goto out; + }do { - ret =qemu_savevm_state_iterate (f,NULL ); -if (ret <0 ) + ret =qemu_savevm_state_iterate (f, errp); +if (ret <0 ) { goto out; + } }while (ret ==0 ); - ret =qemu_savevm_state_complete (f,NULL ); + ret =qemu_savevm_state_complete (f, errp); out:if (ret ==0 ) {
@@ -2217,7 +2220,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)monitor_printf (mon,"Could not open VM state file\n" ); goto the_end; } - ret =qemu_savevm_state (f); + ret =qemu_savevm_state (f,NULL ); vm_state_size =qemu_ftell (f);qemu_fclose (f);if (ret <0 ) {
Signature · 1 line
1.8.0.2
PATCH v2 16/17 qapi: Convert info snapshots
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- hmp.c | 33 +++++++++++++++++++++++++++++++++ hmp.h | 1 + monitor.c | 2 +- qapi-schema.json | 13 +++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ savevm.c | 52 +++++++++++++++++++++++++--------------------------- sysemu.h | 2 -- 7 files changed, 107 insertions(+), 30 deletions(-)
hmp.c +33 -0
@@ -628,6 +628,39 @@ void hmp_info_block_jobs(Monitor *mon) } } +void hmp_info_snapshots (Monitor *mon) +{ +SnapshotInfoList *list; +Error *err =NULL ; +char buf[256 ]; +QEMUSnapshotInfo sn; + + list =qmp_query_vm_snapshots (&err); + +if (error_is_set (&err)) { +hmp_handle_error (mon, &err); +return ; + } + +if (!list) { +monitor_printf (mon,"There is no snapshot available.\n" ); +return ; + } +monitor_printf (mon,"%s\n" ,bdrv_snapshot_dump (buf,sizeof (buf),NULL )); +while (list) { +memcpy (&(sn.id_str), list->value->id,sizeof (sn.id_str)); +memcpy (&(sn.name), list->value->name,sizeof (sn.name)); + sn.date_sec = list->value->date_sec; + sn.date_nsec = list->value->date_nsec; + sn.vm_clock_nsec = list->value->vm_clock_nsec; + sn.vm_state_size = list->value->vm_state_size; +monitor_printf (mon,"%s\n" ,bdrv_snapshot_dump (buf,sizeof (buf), &sn)); + list = list->next; + } + +qapi_free_SnapshotInfoList (list); +} +void hmp_quit (Monitor *mon,const QDict *qdict) {monitor_suspend (mon);
hmp.h +1 -0
@@ -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);
monitor.c +1 -1
@@ -2596,7 +2596,7 @@ static mon_cmd_t info_cmds[] = { .args_type = "", .params = "", .help = "show the currently saved VM snapshots", - .mhandler.info = do_info_snapshots, + .mhandler.info = hmp_info_snapshots, }, { .name = "status",
qapi-schema.json +13 -0
@@ -1224,6 +1224,19 @@ { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] } ## +# @query-vm-snapshots: +# +# List available snapshots for VM. +# +# Returns: an array of @SnapshotInfo describing each snapshot or an empty array +# on success +# If an error occurs, GenericError with error message +# +# Since: 1.3 +## +{ 'command': 'query-vm-snapshots', 'returns': ['SnapshotInfo'] } + +## # @quit: # # This command will cause the QEMU process to exit gracefully. While every
qmp-commands.hx +34 -0
@@ -2732,3 +2732,37 @@ EQMP .args_type = "", .mhandler.cmd_new = qmp_marshal_input_query_target, }, + +SQMP +query-vm-snapshots +---------- + +List available snapshots for VM. + +Return an array of json-object, each with the following information: + +- "id": unique snapshot id + +- "name": user choosen name + +- "vm-state-size": size of the VM state + +- "date-sec": UTC date of the snapshot + +- "date-nsec": date in nano seconds + +- "vm-clock-nsec": VM clock relative to boot in nano seconds + +Example: + +-> { "execute": "query-vm-snapshots" } +<- {"return": [{"vm-clock-nsec": 36420253254, "name": "my_snapshot", + "date-sec": 1345120008, "date-nsec": 151984000, "id": "6", + "vm-state-size": 166515500}]} + +EQMP + { + .name = "query-vm-snapshots", + .args_type = "", + .mhandler.cmd_new = qmp_marshal_input_query_vm_snapshots, + },
savevm.c +25 -27
83 lines not on this page · raw message
sysemu.h +0 -2
@@ -65,8 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);void qemu_add_machine_init_done_notifier (Notifier *notify); -void do_info_snapshots (Monitor *mon); -void qemu_announce_self (void );bool qemu_savevm_state_blocked (Error **errp);
Signature · 1 line
1.8.0.2
PATCH v2 10/17 savevm: add error parameter to qemu_savevm_state_complete()
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- migration.c | 2 +- savevm.c | 13 ++++++++++--- sysemu.h | 3 ++- 3 files changed, 13 insertions(+), 5 deletions(-)
migration.c +1 -1
@@ -353,7 +353,7 @@ void migrate_fd_put_ready(MigrationState *s)qemu_system_wakeup_request (QEMU_WAKEUP_REASON_OTHER );vm_stop_force_state (RUN_STATE_FINISH_MIGRATE ); -if (qemu_savevm_state_complete (s->file) <0 ) { +if (qemu_savevm_state_complete (s->file,NULL ) <0 ) {migrate_fd_error (s); }else {migrate_fd_completed (s);
savevm.c +10 -3
@@ -1713,7 +1713,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,return ret; } -int qemu_savevm_state_complete (QEMUFile *f) +int qemu_savevm_state_complete (QEMUFile *f, +Error **errp) {SaveStateEntry *se;int ret;
@@ -1737,6 +1738,7 @@ int qemu_savevm_state_complete(QEMUFile *f) ret = se->ops->save_live_complete (f, se->opaque);trace_savevm_section_end (se->section_id);if (ret <0 ) { +error_setg (errp,"Failed to complete vmstate save." );return ret; } }
@@ -1766,7 +1768,12 @@ int qemu_savevm_state_complete(QEMUFile *f)qemu_put_byte (f,QEMU_VM_EOF ); -return qemu_file_get_error (f); + ret =qemu_file_get_error (f); +if (ret <0 ) { +error_setg (errp,"%s" ,strerror (errno)); + } + +return ret; }void qemu_savevm_state_cancel (QEMUFile *f)
@@ -1803,7 +1810,7 @@ static int qemu_savevm_state(QEMUFile *f) goto out; }while (ret ==0 ); - ret =qemu_savevm_state_complete (f); + ret =qemu_savevm_state_complete (f,NULL ); out:if (ret ==0 ) {
sysemu.h +2 -1
@@ -78,7 +78,8 @@ int qemu_savevm_state_begin(QEMUFile *f, Error **errp);int qemu_savevm_state_iterate (QEMUFile *f,Error **errp); -int qemu_savevm_state_complete (QEMUFile *f); +int qemu_savevm_state_complete (QEMUFile *f, +Error **errp);void qemu_savevm_state_cancel (QEMUFile *f);int qemu_loadvm_state (QEMUFile *f);
Signature · 1 line
1.8.0.2
PATCH v2 14/17 qapi: Convert loadvm
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- hmp-commands.hx | 4 ++-- hmp.c | 9 ++++++++ hmp.h | 1 + monitor.c | 12 ----------- qapi-schema.json | 15 +++++++++++++ qmp-commands.hx | 25 ++++++++++++++++++++++ savevm.c | 65 ++++++++++++++++++++++++++++---------------------------- sysemu.h | 1 - vl.c | 6 +++++- 9 files changed, 90 insertions(+), 48 deletions(-)
hmp-commands.hx +2 -2
@@ -327,14 +327,14 @@ ETEXI .args_type = "name:s", .params = "tag|id", .help = "restore a VM snapshot from its tag or id", - .mhandler.cmd = do_loadvm, + .mhandler.cmd = hmp_vm_snapshot_load, }, STEXI @item loadvm @var{tag}|@var{id} @findex loadvm Set the whole virtual machine to the snapshot identified by the tag -@var{tag} or the unique snapshot ID @var{id}. +@var{tag} or the unique snapshot ID @var{id}. It must be snapshot of whole VM. ETEXI {
hmp.c +9 -0
@@ -1344,3 +1344,12 @@ void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)qmp_vm_snapshot_save (!!name, name, &err);hmp_handle_error (mon, &err); } + +void hmp_vm_snapshot_load (Monitor *mon,const QDict *qdict) +{ +const char *name =qdict_get_str (qdict,"name" ); +Error *err =NULL ; + +qmp_vm_snapshot_load (name, &err); +hmp_handle_error (mon, &err); +}
hmp.h +1 -0
@@ -81,5 +81,6 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);void hmp_nbd_server_add (Monitor *mon,const QDict *qdict);void hmp_nbd_server_stop (Monitor *mon,const QDict *qdict);void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict); +void hmp_vm_snapshot_load (Monitor *mon,const QDict *qdict);#endif
monitor.c +0 -12
@@ -2065,18 +2065,6 @@ void qmp_closefd(const char *fdname, Error **errp)error_set (errp,QERR_FD_NOT_FOUND , fdname); } -static void do_loadvm (Monitor *mon,const QDict *qdict) -{ -int saved_vm_running =runstate_is_running (); -const char *name =qdict_get_str (qdict,"name" ); - -vm_stop (RUN_STATE_RESTORE_VM ); - -if (load_vmstate (name) ==0 && saved_vm_running) { -vm_start (); - } -} -int monitor_get_fd (Monitor *mon,const char *fdname,Error **errp) {mon_fd_t *monfd;
qapi-schema.json +15 -0
@@ -3036,3 +3036,18 @@ # Since: 1.4.0 ## { 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } + +## +# @vm-snapshot-load: +# +# Set the whole virtual machine to the snapshot identified by the tag +# or the unique snapshot id. It must be snapshot of whole VM. +# +# @name: tag|id of existing snapshot +# +# Returns: Nothing on success +# If an error occurs, GenericError with error message +# +# Since: 1.3 +## +{ 'command': 'vm-snapshot-load', 'data': {'name': 'str'} }
qmp-commands.hx +25 -0
@@ -1377,6 +1377,31 @@ Example: EQMP { + .name = "vm-snapshot-load", + .args_type = "name:s", + .params = "name", + .help = "restore a VM snapshot from its tag or id", + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_load + }, + +SQMP +vm-snapshot-load +------ + +Set the whole virtual machine to the snapshot identified by the tag +or the unique snapshot id. It must be snapshot of whole VM. + +Arguments: + +- "name": tag|id of existing snapshot (json-string) + +Example: + +-> { "execute": "vm-snapshot-load", "arguments": { "name": "my_snapshot" } } +<- { "return": {} } + +EQMP + { .name = "qmp_capabilities", .args_type = "", .params = "",
savevm.c +33 -32
117 lines not on this page · raw message
sysemu.h +0 -1
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);void qemu_add_machine_init_done_notifier (Notifier *notify); -int load_vmstate (const char *name);void do_delvm (Monitor *mon,const QDict *qdict);void do_info_snapshots (Monitor *mon);
vl.c +5 -1
@@ -3976,8 +3976,12 @@ int main(int argc, char **argv, char **envp)qemu_system_reset (VMRESET_SILENT );if (loadvm) { -if (load_vmstate (loadvm) <0 ) { +Error *errp =NULL ; +qmp_vm_snapshot_load (loadvm, &errp); + +if (error_is_set (&errp)) { autostart =0 ; +handle_error (&errp); } }
Signature · 1 line
1.8.0.2
PATCH v2 17/17 vm-snapshot-save: add force parameter
In reply to Pavel HrdinaHMP 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 <[email protected]>
--- hmp-commands.hx | 16 ++++++++-------- hmp.c | 25 ++++++++++++++++++++++++- qapi-schema.json | 9 ++++++--- qmp-commands.hx | 15 +++++++++------ savevm.c | 34 +++++++++++++--------------------- 5 files changed, 60 insertions(+), 39 deletions(-)
hmp-commands.hx +8 -8
@@ -307,19 +307,19 @@ ETEXI { .name = "savevm", - .args_type = "name:s?", - .params = "[tag|id]", - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .args_type = "force:-b,name:s?,", + .params = "[-f] [tag|id]", + .help = "save a VM snapshot. To replace existing snapshot use force flag.", .mhandler.cmd = hmp_vm_snapshot_save, }, STEXI -@item savevm [@var{tag}|@var{id}] +@item savevm [@var{-f}] [@var{tag}|@var{id}] @findex savevm -Create a snapshot of the whole virtual machine. If @var{tag} is -provided, it is used as human readable identifier. If there is already -a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at -@ref{vm_snapshots}. +Create a snapshot of the whole virtual machine. Paramtere "name" is optional. +If @var{tag} is provided, it is used as human readable identifier. If there is +already a snapshot with the same @var{tag} or @var{id}, @var{-f} flag needs to +be specified. More info at @ref{vm_snapshots}. ETEXI {
hmp.c +24 -1
@@ -1372,9 +1372,32 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict) {const char *name =qdict_get_try_str (qdict,"name" ); +char new_name[256 ]; +bool force =qdict_get_try_bool (qdict,"force" ,0 );Error *err =NULL ; +#ifdef _WIN32 +struct _timeb tb; +struct tm *ptm; +#else +struct timeval tv; +struct tm tm; +#endif + +if (!name) { +#ifdef _WIN32 +time_t t = tb.time; + ptm =localtime (&t); +strftime (new_name,sizeof (new_name),"vm-%Y%m%d%H%M%S" , ptm); +#else +/* cast below needed for OpenBSD where tv_sec is still 'long' */ +localtime_r ((const time_t *)&tv.tv_sec, &tm); +strftime (new_name,sizeof (new_name),"vm-%Y%m%d%H%M%S" , &tm); +#endif + }else { +pstrcpy (new_name,sizeof (new_name), name); + } -qmp_vm_snapshot_save (!!name, name, &err); +qmp_vm_snapshot_save (new_name, !!force, force, &err);hmp_handle_error (mon, &err); }
qapi-schema.json +6 -3
@@ -3036,19 +3036,22 @@ # # Create a snapshot of the whole virtual machine. If tag is provided as @name, # it is used as human readable identifier. If there is already a snapshot -# with the same tag or ID, it is replaced. +# with the same tag or id, the force argument needs to be true to replace it. # # The VM is automatically stopped and resumed and saving a snapshot can take # a long time. # -# @name: #optional tag of new snapshot or tag|id of existing snapshot +# @name: tag of new snapshot or tag|id of existing snapshot +# +# @force: #optional specify whether existing snapshot is replaced or not, +# default is false # # Returns: Nothing on success # If an error occurs, GenericError with error message # # Since: 1.4.0 ## -{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } +{ 'command': 'vm-snapshot-save', 'data': {'name': 'str', '*force': 'bool'} } ## # @vm-snapshot-load:
qmp-commands.hx +9 -6
@@ -1349,9 +1349,9 @@ Example: EQMP { .name = "vm-snapshot-save", - .args_type = "name:s?", - .params = "name", - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .args_type = "name:s,force:b?", + .params = "name [force]", + .help = "save a VM snapshot. To replace existing snapshot use force parameter.", .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save },
@@ -1360,15 +1360,18 @@ vm-snapshot-save ------ Create a snapshot of the whole virtual machine. If tag is provided as name, -it is used as human readable identifier. If there is already a snapshot -with the same tag or id, it is replaced. +it is used as human readable identifier. If there is already a snapshot with +the same tag, the force argument needs to be true to replace it. The VM is automatically stopped and resumed and saving a snapshot can take a long time. Arguments: -- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional) +- "name": tag of new snapshot or tag|id of existing snapshot (json-string) + +- "force": specify whether existing snapshot is replaced or not, + default is false (json-bool, optional) Example:
savevm.c +13 -21
@@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,return 0 ; } -void qmp_vm_snapshot_save (bool has_name,const char *name,Error **errp) +void qmp_vm_snapshot_save (const char *name,bool has_force,bool force,Error **errp) {BlockDriverState *bs, *bs1;QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2151,10 +2151,8 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)uint64_t vm_state_size;#ifdef _WIN32struct _timeb tb; -struct tm *ptm;#else struct timeval tv; -struct tm tm;#endif /* Verify if there is a device that doesn't support snapshots and is writable */
@@ -2195,29 +2193,23 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)#endif sn->vm_clock_nsec =qemu_get_clock_ns (vm_clock); -if (has_name) { - ret =bdrv_snapshot_find (bs, old_sn, name,NULL ); -if (ret >=0 ) { + ret =bdrv_snapshot_find (bs, old_sn, name,NULL ); +if (ret >=0 ) { +if (has_force && force) {pstrcpy (sn->name,sizeof (sn->name), old_sn->name);pstrcpy (sn->id_str,sizeof (sn->id_str), old_sn->id_str); + +del_existing_snapshots (name, errp); +if (error_is_set (errp)) { + goto the_end; + } }else { -pstrcpy (sn->name,sizeof (sn->name), name); +error_setg (errp,"Snapshot '%s' exist. For override specify " +"'force=yes'." , name); + goto the_end; } }else { -#ifdef _WIN32 -time_t t = tb.time; - ptm =localtime (&t); -strftime (sn->name,sizeof (sn->name),"vm-%Y%m%d%H%M%S" , ptm); -#else -/* cast below needed for OpenBSD where tv_sec is still 'long' */ -localtime_r ((const time_t *)&tv.tv_sec, &tm); -strftime (sn->name,sizeof (sn->name),"vm-%Y%m%d%H%M%S" , &tm); -#endif - } - -/* Delete old snapshots of the same name */ -if (has_name &&del_existing_snapshots (name, errp) <0 ) { - goto the_end; +pstrcpy (sn->name,sizeof (sn->name), name); }/* save the VM state */
Signature · 1 line
1.8.0.2
PATCH v2 13/17 qapi: Convert savevm
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- hmp-commands.hx | 4 ++-- hmp.c | 9 +++++++++ hmp.h | 1 + qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 29 +++++++++++++++++++++++++++++ savevm.c | 26 ++++++++++---------------- sysemu.h | 1 - 7 files changed, 70 insertions(+), 19 deletions(-)
hmp-commands.hx +2 -2
@@ -310,7 +310,7 @@ ETEXI .args_type = "name:s?", .params = "[tag|id]", .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", - .mhandler.cmd = do_savevm, + .mhandler.cmd = hmp_vm_snapshot_save, }, STEXI
@@ -318,7 +318,7 @@ STEXI @findex savevm Create a snapshot of the whole virtual machine. If @var{tag} is provided, it is used as human readable identifier. If there is already -a snapshot with the same tag or ID, it is replaced. More info at +a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at @ref{vm_snapshots}. ETEXI
hmp.c +9 -0
@@ -1335,3 +1335,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)qmp_nbd_server_stop (&errp);hmp_handle_error (mon, &errp); } + +void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict) +{ +const char *name =qdict_get_try_str (qdict,"name" ); +Error *err =NULL ; + +qmp_vm_snapshot_save (!!name, name, &err); +hmp_handle_error (mon, &err); +}
hmp.h +1 -0
@@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict);void hmp_nbd_server_start (Monitor *mon,const QDict *qdict);void hmp_nbd_server_add (Monitor *mon,const QDict *qdict);void hmp_nbd_server_stop (Monitor *mon,const QDict *qdict); +void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict);#endif
qapi-schema.json +19 -0
@@ -3017,3 +3017,22 @@ # Since: 1.3.0 ## { 'command': 'nbd-server-stop' } + +## +# @vm-snapshot-save: +# +# Create a snapshot of the whole virtual machine. If tag is provided as @name, +# it is used as human readable identifier. If there is already a snapshot +# with the same tag or ID, it is replaced. +# +# The VM is automatically stopped and resumed and saving a snapshot can take +# a long time. +# +# @name: #optional tag of new snapshot or tag|id of existing snapshot +# +# Returns: Nothing on success +# If an error occurs, GenericError with error message +# +# Since: 1.4.0 +## +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
qmp-commands.hx +29 -0
@@ -1348,6 +1348,35 @@ Example: EQMP { + .name = "vm-snapshot-save", + .args_type = "name:s?", + .params = "name", + .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save + }, + +SQMP +vm-snapshot-save +------ + +Create a snapshot of the whole virtual machine. If tag is provided as name, +it is used as human readable identifier. If there is already a snapshot +with the same tag or id, it is replaced. + +The VM is automatically stopped and resumed and saving a snapshot can take +a long time. + +Arguments: + +- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional) + +Example: + +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } +<- { "return": {} } + +EQMP + { .name = "qmp_capabilities", .args_type = "", .params = "",
savevm.c +10 -16
@@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,return 0 ; } -void do_savevm (Monitor *mon,const QDict *qdict) +void qmp_vm_snapshot_save (bool has_name,const char *name,Error **errp) {BlockDriverState *bs, *bs1;QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2156,7 +2156,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)struct timeval tv;struct tm tm;#endif -const char *name =qdict_get_try_str (qdict,"name" );/* Verify if there is a device that doesn't support snapshots and is writable */ bs =NULL ;
@@ -2167,15 +2166,15 @@ void do_savevm(Monitor *mon, const QDict *qdict) }if (!bdrv_can_snapshot (bs)) { -monitor_printf (mon,"Device '%s' is writable but does not support snapshots.\n" , -bdrv_get_device_name (bs)); +error_setg (errp,"Device '%s' is writable but does not support " +"snapshots." ,bdrv_get_device_name (bs));return ; } } bs =bdrv_snapshots ();if (!bs) { -monitor_printf (mon,"No block device can accept snapshots\n" ); +error_setg (errp,"No block device can accept snapshots." );return ; }
@@ -2196,7 +2195,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)#endif sn->vm_clock_nsec =qemu_get_clock_ns (vm_clock); -if (name) { +if (has_name) { ret =bdrv_snapshot_find (bs, old_sn, name,NULL );if (ret >=0 ) {pstrcpy (sn->name,sizeof (sn->name), old_sn->name);
@@ -2217,21 +2216,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) }/* Delete old snapshots of the same name */ -if (name &&del_existing_snapshots (name,NULL ) <0 ) { +if (has_name &&del_existing_snapshots (name, errp) <0 ) { goto the_end; }/* save the VM state */ f =qemu_fopen_bdrv (bs,1 );if (!f) { -monitor_printf (mon,"Could not open VM state file\n" ); +error_setg (errp,"Failed to open '%s' file." ,bdrv_get_device_name (bs)); goto the_end; } - ret =qemu_savevm_state (f,NULL ); + ret =qemu_savevm_state (f, errp); vm_state_size =qemu_ftell (f);qemu_fclose (f); -if (ret <0 ) { -monitor_printf (mon,"Error %d while writing VM\n" , ret); +if (error_is_set (errp)) { goto the_end; }
@@ -2242,11 +2240,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)if (bdrv_can_snapshot (bs1)) {/* Write VM state size only to the image that contains the state */ sn->vm_state_size = (bs == bs1 ? vm_state_size :0 ); - ret =bdrv_snapshot_create (bs1, sn,NULL ); -if (ret <0 ) { -monitor_printf (mon,"Error while creating snapshot on '%s'\n" , -bdrv_get_device_name (bs1)); - } +bdrv_snapshot_create (bs1, sn, errp); } }
sysemu.h +0 -1
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);void qemu_add_machine_init_done_notifier (Notifier *notify); -void do_savevm (Monitor *mon,const QDict *qdict);int load_vmstate (const char *name);void do_delvm (Monitor *mon,const QDict *qdict);void do_info_snapshots (Monitor *mon);
Signature · 1 line
1.8.0.2
PATCH v2 15/17 qapi: Convert delvm
In reply to Pavel HrdinaSigned-off-by Pavel Hrdina <[email protected]>
--- 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(-)
hmp-commands.hx +1 -1
@@ -342,7 +342,7 @@ ETEXI .args_type = "name:s", .params = "tag|id", .help = "delete a VM snapshot from its tag or id", - .mhandler.cmd = do_delvm, + .mhandler.cmd = hmp_vm_snapshot_delete, }, STEXI
hmp.c +10 -0
@@ -1353,3 +1353,13 @@ void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)qmp_vm_snapshot_load (name, &err);hmp_handle_error (mon, &err); } + +void hmp_vm_snapshot_delete (Monitor *mon,const QDict *qdict) +{ +const char *name =qdict_get_str (qdict,"name" ); +Error *err =NULL ; + +qmp_vm_snapshot_delete (name, &err); + +hmp_handle_error (mon, &err); +}
hmp.h +1 -0
@@ -82,5 +82,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);void hmp_nbd_server_stop (Monitor *mon,const QDict *qdict);void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict);void hmp_vm_snapshot_load (Monitor *mon,const QDict *qdict); +void hmp_vm_snapshot_delete (Monitor *mon,const QDict *qdict);#endif
qapi-schema.json +14 -0
@@ -3051,3 +3051,17 @@ # Since: 1.3 ## { 'command': 'vm-snapshot-load', 'data': {'name': 'str'} } + +## +# @vm-snapshot-delete: +# +# Delete the snapshot identified by tag or id. +# +# @name: tag|id of existing snapshot +# +# Returns: Nothing on success +# If an error occurs, GenericError with error message +# +# Since: 1.3 +## +{ 'command': 'vm-snapshot-delete', 'data': {'name': 'str'} }
qmp-commands.hx +24 -0
@@ -1402,6 +1402,30 @@ Example: EQMP { + .name = "vm-snapshot-delete", + .args_type = "name:s", + .params = "tag|id", + .help = "delete a VM snapshot from its tag or id", + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_delete + }, + +SQMP +vm-snapshot-delete +------ + +Delete the snapshot identified by tag or id. + +Arguments: + +- "name": tag|id of existing snapshot (json-string) + +Example: + +-> { "execute": "vm-snapshot-delete", "arguments": { "name": "my_snapshot" } } +<- { "return": {} } + +EQMP + { .name = "qmp_capabilities", .args_type = "", .params = "",
savevm.c +3 -14
@@ -2355,31 +2355,20 @@ void qmp_vm_snapshot_load(const char *name, Error **errp) } } -void do_delvm (Monitor *mon,const QDict *qdict) +void qmp_vm_snapshot_delete (const char *name,Error **errp) {BlockDriverState *bs, *bs1; -int ret; -const char *name =qdict_get_str (qdict,"name" ); bs =bdrv_snapshots ();if (!bs) { -monitor_printf (mon,"No block device supports snapshots\n" ); +error_setg (errp,"No block device supports snapshots." );return ; } bs1 =NULL ;while ((bs1 =bdrv_next (bs1))) {if (bdrv_can_snapshot (bs1)) { - ret =bdrv_snapshot_delete (bs1, name,NULL ); -if (ret <0 ) { -if (ret == -ENOTSUP ) -monitor_printf (mon, -"Snapshots not supported on device '%s'\n" , -bdrv_get_device_name (bs1)); -else -monitor_printf (mon,"Error %d while deleting snapshot on " -"'%s'\n" , ret,bdrv_get_device_name (bs1)); - } +bdrv_snapshot_delete (bs1, name, errp); } } }
sysemu.h +0 -1
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);void qemu_add_machine_init_done_notifier (Notifier *notify); -void do_delvm (Monitor *mon,const QDict *qdict);void do_info_snapshots (Monitor *mon);void qemu_announce_self (void );
Signature · 1 line
1.8.0.2
Re: [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error
In reply to Pavel HrdinaQuoted 11 lines · Pavel Hrdina
Signed-off-by Pavel Hrdina <[email protected]>--- error.c | 8 ++++++++ error.h | 6 ++++++ 2 files changed, 14 insertions(+)+/**
+ * Print an error object as pretty string to current monitor or on stderr, then
+ * free the errot object.
s/errot/error/
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 01/17] error: introduce handle_error
In reply to Pavel HrdinaQuoted 11 lines · On Thu, 13 Dec 2012 16:40:35 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- error.c | 8 ++++++++ error.h | 6 ++++++ 2 files changed, 14 insertions(+)error.c +0
-0
error.c void error_propagate(Error **dst_err, Error *local_err) · line 122
Quoted 10 lines
error.c +7
-0@@ -113,3 +113,11 @@ void error_propagate(Error **dst_err, Error *local_err)error_free (local_err); } } + +void handle_error (Error **errp) +{ +if (error_is_set (errp)) { +error_report ("%s" ,error_get_pretty (*errp)); +error_free (*errp); + }
This is not good for a few reasons. The most important ones are that we most probably shouldn't be using error_report() in new qapi code. The other reason is that this function doesn't actually handle the error.
I've added a similar function in hmp.c but just to save some typing, but we shouldn't do this elsewhere.
Quoted 16 lines
error.c +1
-0+}error.h +6
-0@@ -77,4 +77,10 @@ void error_propagate(Error **dst_err, Error *local_err);*/ void error_free (Error *err); +/** +* Print an error object as pretty string to current monitor or on stderr, then +* free the errot object. +*/ +void handle_error (Error **errp); +#endif
Re: [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions
In reply to Pavel HrdinaQuoted 39 lines · On Thu, 13 Dec 2012 16:40:36 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- block.c | 26 ++++++++++++++++++-------- block.h | 3 ++- block/qcow2-snapshot.c | 11 ++++++++++- block/qcow2.h | 4 +++- block/rbd.c | 17 +++++++++++------ block/sheepdog.c | 18 ++++++++++-------- block_int.h | 3 ++- qemu-img.c | 8 +++----- savevm.c | 2 +- 9 files changed, 60 insertions(+), 32 deletions(-)block.c +8
-8@@ -3075,16 +3075,26 @@ BlockDriverState *bdrv_snapshots(void) }int bdrv_snapshot_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info) +QEMUSnapshotInfo *sn_info, +Error **errp) {BlockDriver *drv = bs->drv; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_create) -return drv->bdrv_snapshot_create (bs, sn_info); -if (bs->file) -return bdrv_snapshot_create (bs->file, sn_info); -return -ENOTSUP ; +int ret; + +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ;
block.c BlockDriverState *bdrv_snapshots(void) · line 3097
Quoted 10 lines
block.c +10
-0+ }else if (drv->bdrv_snapshot_create) { + ret = drv->bdrv_snapshot_create (bs, sn_info, errp); + }else if (bs->file) { + ret =bdrv_snapshot_create (bs->file, sn_info, errp); + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; + } + +return ret;
This function returns an error code _and_ an error object. This is fine if you drop the error code in a later patch, but you don't do it in this series.
IMO, we shouldn't return both unless there's a strong reason to do so (ie. you need the error code and a very specific error message, or you plan to drop the error code in the future). If this is the case here, please tell us.
When converting functions that return an error code, I check if any caller of the function check for a specific error code. If no caller does this, then I just drop the error code (but in a different patch).
If a caller actually checks the error code, you generally have two options:
1. See if checking the error code is actually required, if it's not then just drop it
2. Don't add the Error object to the function returning the error code. Add it to the callers instead
We (block layer guys and me) had a discussion about this some weeks ago, I don't exactly remember if what I described above matches our conclusions, so I'm CC'ing them.
Quoted 22 lines
block.c +0
-0} int bdrv_snapshot_goto(BlockDriverState *bs,block.h +2
-1@@ -321,7 +321,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);int bdrv_is_snapshot (BlockDriverState *bs);BlockDriverState *bdrv_snapshots (void );int bdrv_snapshot_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info); +QEMUSnapshotInfo *sn_info, +Error **errp);int bdrv_snapshot_goto (BlockDriverState *bs,const char *snapshot_id);int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id);block/qcow2-snapshot.c +0
-0
block/qcow2-snapshot.c line 28
Quoted 4 lines
block/qcow2-snapshot.c +1
-0@@ -25,6 +25,7 @@#include "qemu-common.h" #include "block_int.h" #include "block/qcow2.h" +#include "qerror.h"
I think you want to include error.h. qerror.h is legacy.
Quoted 14 lines
block/qcow2-snapshot.c +3
-1typedef 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 ;
block/qcow2-snapshot.c int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) · line 337
Quoted 4 lines
block/qcow2-snapshot.c +1
-0@@ -331,6 +334,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)/* Check that the ID is unique */ if (find_snapshot_by_id (bs, sn_info->id_str) >=0 ) { +error_setg (errp,"Parameter 'name' has to be unique ID." );
Here you're converting qcow2_snapshot_create() to propagate the error, and I see that you do it for more functions below.
Basically, this kind of conversion should be split into one per-patch or even one per-series depending on the complexity.
A good series converting do_foo() to take an Error object would do:
1. Add an Error object to do_foo() and fill it properly. Callers of do_foo() should pass NULL for the new argument
2. For each caller of do_foo() create a new patch, which changes the caller to pass an Error object to do_foo() and use it internally
3. If do_foo() returns an error code, drop it
Quoted 238 lines
block/qcow2-snapshot.c +5
-0return -EEXIST ; }@@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) l1_table_offset =qcow2_alloc_clusters (bs, s->l1_size *sizeof (uint64_t ));if (l1_table_offset <0 ) { ret = l1_table_offset; +error_setg (errp,"Failed to allocate L1 table." ); goto fail; }@@ -362,6 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) ret =bdrv_pwrite (bs->file, sn->l1_table_offset, l1_table, s->l1_size *sizeof (uint64_t ));if (ret <0 ) { +error_setg (errp,"Failed to save L1 table." ); goto fail; }@@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)*/ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,1 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; } ret =bdrv_flush (bs);if (ret <0 ) { +error_setg (errp,"Failed to flush data." ); goto fail; }@@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)if (ret <0 ) {g_free (s->snapshots); s->snapshots = old_snapshot_list; +error_setg (errp,"Failed to write new snapshots." ); goto fail; }block/qcow2.h +3
-1@@ -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);block/rbd.c +11
-6@@ -16,6 +16,7 @@#include "qemu-common.h" #include "qemu-error.h" #include "block_int.h" +#include "qerror.h" #include <rbd/librbd.h> @@ -811,12 +812,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset) }static int qemu_rbd_snap_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info) +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVRBDState *s = bs->opaque; -int r; +int ret;if (sn_info->name[0 ] =='\0' ) { +error_setg (errp,"Parameter 'name' cannot be empty." );return -EINVAL ;/* we need a name for rbd snapshots */ }@@ -826,17 +829,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,*/ if (sn_info->id_str[0 ] !='\0' &&strcmp (sn_info->id_str, sn_info->name) !=0 ) { +error_setg (errp,"ID and name have to be equal." );return -EINVAL ; }if (strlen (sn_info->name) >=sizeof (sn_info->id_str)) { +error_setg (errp,"Parameter 'name' has to be shorter that 127 chars." );return -ERANGE ; } - r =rbd_snap_create (s->image, sn_info->name); -if (r <0 ) { -error_report ("failed to create snap: %s" ,strerror (-r)); -return r; + ret =rbd_snap_create (s->image, sn_info->name); +if (ret <0 ) { +error_setg (errp,"Failed to create snapshot." ); +return ret; }return 0 ;block/sheepdog.c +10
-8@@ -17,6 +17,7 @@#include "qemu_socket.h" #include "block_int.h" #include "bitops.h" +#include "qerror.h" #define SD_PROTO_VER 0x01@@ -1735,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)return 0 ; } -static int sd_snapshot_create (BlockDriverState *bs,QEMUSnapshotInfo *sn_info) +static int sd_snapshot_create (BlockDriverState *bs, +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVSheepdogState *s = bs->opaque;int ret, fd;@@ -1748,9 +1751,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) s->name, sn_info->vm_state_size, s->is_snapshot);if (s->is_snapshot) { -error_report ("You can't create a snapshot of a snapshot VDI, " -"%s (%" PRIu32")." , s->name, s->inode.vdi_id); - +error_setg (errp,"You can't create a snapshot '%s' of a VDI snapshot." , + sn_info->name);return -EINVAL ; }@@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { ret = fd; +error_setg (errp,"Failed to connect to sdog." ); goto cleanup; } ret =write_object (fd, (char *)&s->inode,vid_to_vdi_oid (s->inode.vdi_id), s->inode.nr_copies, datalen,0 , false, s->cache_enabled);if (ret <0 ) { -error_report ("failed to write snapshot's inode." ); +error_setg (errp,"Failed to write snapshot's inode." ); goto cleanup; } ret =do_sd_create (s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,1 , s->addr, s->port);if (ret <0 ) { -error_report ("failed to create inode for snapshot. %s" , -strerror (errno)); +error_setg (errp,"Failed to create inode for snapshot." ); goto cleanup; }@@ -1793,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) s->inode.nr_copies, datalen, 0, s->cache_enabled);if (ret <0 ) { -error_report ("failed to read new inode info. %s" ,strerror (errno)); +error_setg (errp,"Failed to read new inode info." ); goto cleanup; }block_int.h +2
-1@@ -147,7 +147,8 @@ struct BlockDriver { const uint8_t *buf, int nb_sectors);int (*bdrv_snapshot_create)(BlockDriverState *bs, -QEMUSnapshotInfo *sn_info); +QEMUSnapshotInfo *sn_info, +Error **errp);int (*bdrv_snapshot_goto)(BlockDriverState *bs,const char *snapshot_id);int (*bdrv_snapshot_delete)(BlockDriverState *bs,const char *snapshot_id);qemu-img.c +3
-5@@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)int c, ret =0 , bdrv_oflags;int action =0 ;qemu_timeval tv; +Error *err =NULL ; bdrv_oflags =BDRV_O_FLAGS |BDRV_O_RDWR ;/* Parse commandline parameters */ @@ -1559,11 +1560,7 @@ static int img_snapshot(int argc, char **argv) sn.date_sec = tv.tv_sec; sn.date_nsec = tv.tv_usec *1000 ; - ret =bdrv_snapshot_create (bs, &sn); -if (ret) { -error_report ("Could not create snapshot '%s': %d (%s)" , - snapshot_name, ret,strerror (-ret)); - } + ret =bdrv_snapshot_create (bs, &sn, &err);break ;case SNAPSHOT_APPLY :@@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)/* Cleanup */ bdrv_delete (bs); +handle_error (&err);if (ret) {return 1 ; }savevm.c +1
-1@@ -2211,7 +2211,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)if (bdrv_can_snapshot (bs1)) {/* Write VM state size only to the image that contains the state */ sn->vm_state_size = (bs == bs1 ? vm_state_size :0 ); - ret =bdrv_snapshot_create (bs1, sn); + ret =bdrv_snapshot_create (bs1, sn,NULL );if (ret <0 ) {monitor_printf (mon,"Error while creating snapshot on '%s'\n" ,bdrv_get_device_name (bs1));
Re: [Qemu-devel] [PATCH v2 03/17] block: add error parameter to bdrv_snapshot_goto() and related functions
In reply to Pavel HrdinaQuoted 1 line · On Thu, 13 Dec 2012 16:40:37 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>
The same comments I made in the previous patch apply here: you're keeping the error code and converting callers to propagate error in the same patch (which makes this difficult to review).
Quoted 293 lines
--- block.c | 27 ++++++++++++++++----------- block.h | 3 ++- block/qcow2-snapshot.c | 15 ++++++++++++--- block/qcow2.h | 4 +++- block/rbd.c | 6 +++++- block/sheepdog.c | 14 +++++++------- block_int.h | 3 ++- qemu-img.c | 6 +----- savevm.c | 2 +- 9 files changed, 49 insertions(+), 31 deletions(-)block.c +16
-11@@ -3098,29 +3098,34 @@ int bdrv_snapshot_create(BlockDriverState *bs, }int bdrv_snapshot_goto (BlockDriverState *bs, -const char *snapshot_id) +const char *snapshot_id, +Error **errp) {BlockDriver *drv = bs->drv;int ret, open_ret; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_goto) -return drv->bdrv_snapshot_goto (bs, snapshot_id); - -if (bs->file) { +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ; + }else if (drv->bdrv_snapshot_goto) { + ret = drv->bdrv_snapshot_goto (bs, snapshot_id, errp); + }else if (bs->file) { drv->bdrv_close (bs); - ret =bdrv_snapshot_goto (bs->file, snapshot_id); + ret =bdrv_snapshot_goto (bs->file, snapshot_id, errp); open_ret = drv->bdrv_open (bs, bs->open_flags);if (open_ret <0 ) {bdrv_delete (bs->file); bs->drv =NULL ; -return open_ret; +error_setg (errp,"Failed to open '%s'." ,bdrv_get_device_name (bs)); + ret = open_ret; } -return ret; + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; } -return -ENOTSUP ; +return ret; }int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id)block.h +2
-1@@ -324,7 +324,8 @@ int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, Error **errp);int bdrv_snapshot_goto (BlockDriverState *bs, -const char *snapshot_id); +const char *snapshot_id, +Error **errp);int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id);int bdrv_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_info);block/qcow2-snapshot.c +12
-3@@ -428,7 +428,9 @@ fail: }/* copy the snapshot 'snapshot_name' into the current disk image */ -int qcow2_snapshot_goto (BlockDriverState *bs,const char *snapshot_id) +int qcow2_snapshot_goto (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {BDRVQcowState *s = bs->opaque;QCowSnapshot *sn;@@ -440,13 +442,14 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)/* Search the snapshot */ snapshot_index =find_snapshot_by_id_or_name (bs, snapshot_id);if (snapshot_index <0 ) { +error_setg (errp,"Failed to find snapshot '%s' by id or name." , + snapshot_id);return -ENOENT ; } sn = &s->snapshots[snapshot_index];if (sn->disk_size != bs->total_sectors *BDRV_SECTOR_SIZE ) { -error_report ("qcow2: Loading snapshots with different disk " -"size is not implemented" ); +error_setg (errp,"Not supported." ); ret = -ENOTSUP ; goto fail; }@@ -458,6 +461,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)*/ ret =qcow2_grow_l1_table (bs, sn->l1_size, true);if (ret <0 ) { +error_setg (errp,"Failed to grow L1 table." ); goto fail; }@@ -476,18 +480,21 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) ret =bdrv_pread (bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);if (ret <0 ) { +error_setg (errp,"Failed to load data into L1 table." ); goto fail; } ret =qcow2_update_snapshot_refcount (bs, sn->l1_table_offset, sn->l1_size,1 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; } ret =bdrv_pwrite_sync (bs->file, s->l1_table_offset, sn_l1_table, cur_l1_bytes);if (ret <0 ) { +error_setg (errp,"Failed to save L1 table." ); goto fail; }@@ -513,6 +520,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) }if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; }@@ -525,6 +533,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)*/ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,0 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; }block/qcow2.h +3
-1@@ -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);block/rbd.c +5
-1@@ -858,12 +858,16 @@ static int qemu_rbd_snap_remove(BlockDriverState *bs, }static int qemu_rbd_snap_rollback (BlockDriverState *bs, -const char *snapshot_name) +const char *snapshot_name, +Error **errp) {BDRVRBDState *s = bs->opaque;int r; r =rbd_snap_rollback (s->image, snapshot_name); +if (r <0 ) { +error_setg (errp,"Failed to rollback snapshot." ); + }return r; }block/sheepdog.c +7
-7@@ -1808,7 +1808,9 @@ cleanup:return ret; } -static int sd_snapshot_goto (BlockDriverState *bs,const char *snapshot_id) +static int sd_snapshot_goto (BlockDriverState *bs, +const char *snapshot_id, +Error **errp) {BDRVSheepdogState *s = bs->opaque;BDRVSheepdogState *old_s;@@ -1833,14 +1835,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) ret =find_vdi_name (s, vdi, snapid, tag, &vid,1 );if (ret) { -error_report ("Failed to find_vdi_name" ); +error_setg (errp,"Failed to find VDI snapshot '%s'." , vdi); goto out; } fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { -error_report ("failed to connect" ); ret = fd; +error_setg (errp,"Failed to connect to sdog." ); goto out; }@@ -1851,13 +1853,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)closesocket (fd);if (ret) { +error_setg (errp,"Failed to open '%s'." , vdi); goto out; }memcpy (&s->inode, buf,sizeof (s->inode));if (!s->inode.vm_state_size) { -error_report ("Invalid snapshot" ); +error_setg (errp,"Invalid snapshot '%s'." , snapshot_id); ret = -ENOENT ; goto out; }@@ -1873,9 +1876,6 @@ out:memcpy (s, old_s,sizeof (BDRVSheepdogState));g_free (buf);g_free (old_s); - -error_report ("failed to open. recover old bdrv_sd_state." ); -return ret; }block_int.h +2
-1@@ -150,7 +150,8 @@ struct BlockDriver { QEMUSnapshotInfo *sn_info, Error **errp);int (*bdrv_snapshot_goto)(BlockDriverState *bs, -const char *snapshot_id); +const char *snapshot_id, +Error **errp);int (*bdrv_snapshot_delete)(BlockDriverState *bs,const char *snapshot_id);int (*bdrv_snapshot_list)(BlockDriverState *bs,QEMUSnapshotInfo **psn_info);qemu-img.c +1
-5@@ -1564,11 +1564,7 @@ static int img_snapshot(int argc, char **argv)break ;case SNAPSHOT_APPLY : - ret =bdrv_snapshot_goto (bs, snapshot_name); -if (ret) { -error_report ("Could not apply snapshot '%s': %d (%s)" , - snapshot_name, ret,strerror (-ret)); - } + ret =bdrv_snapshot_goto (bs, snapshot_name, &err);break ;case SNAPSHOT_DELETE :savevm.c +1
-1@@ -2301,7 +2301,7 @@ int load_vmstate(const char *name) bs =NULL ;while ((bs =bdrv_next (bs))) {if (bdrv_can_snapshot (bs)) { - ret =bdrv_snapshot_goto (bs, name); + ret =bdrv_snapshot_goto (bs, name,NULL );if (ret <0 ) {error_report ("Error %d while activating snapshot '%s' on '%s'" , ret, name,bdrv_get_device_name (bs));
Re: [Qemu-devel] [PATCH v2 07/17] block: add error parameter to del_existing_snapshots()
In reply to Pavel HrdinaQuoted 36 lines · On Thu, 13 Dec 2012 16:40:41 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- savevm.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)savevm.c +3
-7@@ -2094,22 +2094,18 @@ static int bdrv_snapshot_find(BlockDriverState *bs,/* * Deletes snapshots of a given name in all opened images. */ -static int del_existing_snapshots (Monitor *mon,const char *name) +static int del_existing_snapshots (const char *name, +Error **errp) {BlockDriverState *bs;QEMUSnapshotInfo sn1, *snapshot = &sn1; -int ret; bs =NULL ;while ((bs =bdrv_next (bs))) {if (bdrv_can_snapshot (bs) &&bdrv_snapshot_find (bs, snapshot, name,NULL ) >=0 ) { - ret =bdrv_snapshot_delete (bs, name,NULL ); -if (ret <0 ) { -monitor_printf (mon, -"Error while deleting snapshot on '%s'\n" , -bdrv_get_device_name (bs)); +if (bdrv_snapshot_delete (bs, name, errp) <0 ) {return -1 ; } }
savevm.c void do_savevm(Monitor *mon, const QDict *qdict) · line 2193
Quoted 5 lines
savevm.c +1
-1@@ -2194,7 +2190,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) }/* Delete old snapshots of the same name */ -if (name &&del_existing_snapshots (mon, name) <0 ) { +if (name &&del_existing_snapshots (name,NULL ) <0 ) {
Where do you print the error message returned by del_existing_snapshots()? Actually, you seem to be leaking the error object...
Quoted 3 lines
savevm.c +0
-0goto the_end; }
Re: [Qemu-devel] [PATCH v2 08/17] savevm: add error parameter to qemu_savevm_state_begin()
In reply to Pavel HrdinaQuoted 35 lines · On Thu, 13 Dec 2012 16:40:42 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- migration.c | 2 +- savevm.c | 7 +++++-- sysemu.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-)migration.c +1
-1@@ -451,7 +451,7 @@ void migrate_fd_connect(MigrationState *s) s->file =qemu_fopen_ops_buffered (s);DPRINTF ("beginning savevm\n" ); - ret =qemu_savevm_state_begin (s->file, &s->params); + ret =qemu_savevm_state_begin (s->file, &s->params,NULL );if (ret <0 ) {DPRINTF ("failed, %d\n" , ret);migrate_fd_error (s);savevm.c +2
-1@@ -1601,7 +1601,8 @@ bool qemu_savevm_state_blocked(Error **errp) }int qemu_savevm_state_begin (QEMUFile *f, -const MigrationParams *params) +const MigrationParams *params, +Error **errp) {SaveStateEntry *se;int ret;
savevm.c int qemu_savevm_state_begin(QEMUFile *f, · line 1645
Quoted 4 lines
savevm.c +1
-0@@ -1641,12 +1642,14 @@ int qemu_savevm_state_begin(QEMUFile *f, ret = se->ops->save_live_setup (f, se->opaque);if (ret <0 ) { +error_setg (errp,"Failed to begin vmstate save." );
If possible, it would be nice to say why.
savevm.c int qemu_savevm_state_begin(QEMUFile *f, · line 1652
Quoted 7 lines
savevm.c +1
-0qemu_savevm_state_cancel (f);return ret; } } ret =qemu_file_get_error (f);if (ret !=0 ) { +error_setg (errp,"%s" ,strerror (errno));
We have error_setg_errno().
Quoted 26 lines
savevm.c +1
-1qemu_savevm_state_cancel (f); }@@ -1783,7 +1786,7 @@ static int qemu_savevm_state(QEMUFile *f) goto out; } - ret =qemu_savevm_state_begin (f, ¶ms); + ret =qemu_savevm_state_begin (f, ¶ms,NULL );if (ret <0 ) goto out;sysemu.h +2
-1@@ -74,7 +74,8 @@ void qemu_announce_self(void);bool qemu_savevm_state_blocked (Error **errp);int qemu_savevm_state_begin (QEMUFile *f, -const MigrationParams *params); +const MigrationParams *params, +Error **errp);int qemu_savevm_state_iterate (QEMUFile *f);int qemu_savevm_state_complete (QEMUFile *f);void qemu_savevm_state_cancel (QEMUFile *f);
Re: [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state()
In reply to Pavel HrdinaQuoted 16 lines · On Thu, 13 Dec 2012 16:40:46 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- migration.c | 8 ++++---- savevm.c | 39 +++++++++++++++++++++++---------------- sysemu.h | 3 ++- 3 files changed, 29 insertions(+), 21 deletions(-)migration.c +0
-1@@ -86,13 +86,13 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)static void process_incoming_migration_co (void *opaque) {QEMUFile *f = opaque; -int ret;
migration.c void qemu_start_incoming_migration(const char *uri, Error **errp) · line 95
Quoted 10 lines
migration.c +4
-3+Error **errp =NULL ; - ret =qemu_loadvm_state (f); +qemu_loadvm_state (f, errp);qemu_set_fd_handler (qemu_get_fd (f),NULL ,NULL ,NULL );qemu_fclose (f); -if (ret <0 ) { -fprintf (stderr,"load of migration failed\n" ); +if (error_is_set (errp)) { +handle_error (errp);
You can just print to stderr here, as qemu is going to exit.
Quoted 18 lines
migration.c +0
-0exit (0 ); }qemu_announce_self ();savevm.c +2
-1@@ -1962,7 +1962,8 @@ typedef struct LoadStateEntry {int version_id; } LoadStateEntry; -int qemu_loadvm_state (QEMUFile *f) +int qemu_loadvm_state (QEMUFile *f, +Error **errp) { QLIST_HEAD(, LoadStateEntry) loadvm_handlers =QLIST_HEAD_INITIALIZER (loadvm_handlers);
savevm.c int qemu_loadvm_state(QEMUFile *f) · line 1976
Quoted 7 lines
savevm.c +2
-2@@ -1971,21 +1972,25 @@ int qemu_loadvm_state(QEMUFile *f)unsigned int v;int ret; -if (qemu_savevm_state_blocked (NULL )) { -return -EINVAL ; +if (qemu_savevm_state_blocked (errp)) { +return -ENOTSUP ;
Why?
savevm.c int qemu_loadvm_state(QEMUFile *f) · line 1987
Quoted 13 lines
savevm.c +4
-2} v =qemu_get_be32 (f); -if (v !=QEMU_VM_FILE_MAGIC ) +if (v !=QEMU_VM_FILE_MAGIC ) { +error_setg (errp,"Unknown vmstate file magic" );return -EINVAL ; + } v =qemu_get_be32 (f);if (v ==QEMU_VM_FILE_VERSION_COMPAT ) { -fprintf (stderr,"SaveVM v2 format is obsolete and don't work anymore\n" ); +error_setg (errp,"Not supported." );
Will this new error message reach the terminal as fprintf() does? Also, please keep the good error message.
savevm.c int qemu_loadvm_state(QEMUFile *f) · line 1991
Quoted 5 lines
savevm.c +2
-1return -ENOTSUP ; } -if (v !=QEMU_VM_FILE_VERSION ) +if (v !=QEMU_VM_FILE_VERSION ) { +error_setg (errp,"Not supported." );
Please, improve the error message.
Quoted 60 lines
savevm.c +9
-9return -ENOTSUP ; + }while ((section_type =qemu_get_byte (f)) !=QEMU_VM_EOF ) {uint32_t instance_id, version_id, section_id;@@ -2007,15 +2012,16 @@ int qemu_loadvm_state(QEMUFile *f)/* Find savevm section */ se =find_se (idstr, instance_id);if (se ==NULL ) { -fprintf (stderr,"Unknown savevm section or instance '%s' %d\n" , idstr, instance_id); +error_setg (errp,"Unknown savevm section or instance '%s' %d" , + idstr, instance_id); ret = -EINVAL ; goto out; }/* Validate version */ if (version_id > se->version_id) { -fprintf (stderr,"savevm: unsupported version %d for '%s' v%d\n" , - version_id, idstr, se->version_id); +error_setg (errp,"savevm: unsupported version %d for '%s' v%d" , + version_id, idstr, se->version_id); ret = -EINVAL ; goto out; }@@ -2030,8 +2036,7 @@ int qemu_loadvm_state(QEMUFile *f) ret =vmstate_load (f, le->se, le->version_id);if (ret <0 ) { -fprintf (stderr,"qemu: warning: error while loading state for instance 0x%x of device '%s'\n" , - instance_id, idstr); +error_setg (errp,"Failed to load vmstate." ); goto out; }break ;@@ -2045,20 +2050,19 @@ int qemu_loadvm_state(QEMUFile *f) } }if (le ==NULL ) { -fprintf (stderr,"Unknown savevm section %d\n" , section_id); +error_setg (errp,"Unknown savevm section %d" , section_id); ret = -EINVAL ; goto out; } ret =vmstate_load (f, le->se, le->version_id);if (ret <0 ) { -fprintf (stderr,"qemu: warning: error while loading state section id %d\n" , - section_id); +error_setg (errp,"Failed to load vmstate." ); goto out; }break ;default : -fprintf (stderr,"Unknown savevm section type %d\n" , section_type); +error_setg (errp,"Unknown savevm section type %d" , section_type); ret = -EINVAL ; goto out; }
savevm.c out: · line 2084
Quoted 5 lines
savevm.c +2
-0@@ -2076,6 +2080,9 @@ out:if (ret ==0 ) { ret =qemu_file_get_error (f); +if (ret <0 ) { +error_set (errp,QERR_GENERIC_ERROR , ret);
No reason to sue QERR_ macros here.
Quoted 27 lines
savevm.c +2
-1+ } }return ret;@@ -2342,7 +2349,7 @@ int load_vmstate(const char *name) }qemu_system_reset (VMRESET_SILENT ); - ret =qemu_loadvm_state (f); + ret =qemu_loadvm_state (f,NULL );qemu_fclose (f);if (ret <0 ) {sysemu.h +2
-1@@ -81,7 +81,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,int qemu_savevm_state_complete (QEMUFile *f,Error **errp);void qemu_savevm_state_cancel (QEMUFile *f); -int qemu_loadvm_state (QEMUFile *f); +int qemu_loadvm_state (QEMUFile *f, +Error **errp);/* SLIRP */ void do_info_slirp (Monitor *mon);
Re: [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions
In reply to Pavel Hrdina[Forgot adding block layer guys, re-sending]
Quoted 39 lines · On Thu, 13 Dec 2012 16:40:36 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- block.c | 26 ++++++++++++++++++-------- block.h | 3 ++- block/qcow2-snapshot.c | 11 ++++++++++- block/qcow2.h | 4 +++- block/rbd.c | 17 +++++++++++------ block/sheepdog.c | 18 ++++++++++-------- block_int.h | 3 ++- qemu-img.c | 8 +++----- savevm.c | 2 +- 9 files changed, 60 insertions(+), 32 deletions(-)block.c +8
-8@@ -3075,16 +3075,26 @@ BlockDriverState *bdrv_snapshots(void) }int bdrv_snapshot_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info) +QEMUSnapshotInfo *sn_info, +Error **errp) {BlockDriver *drv = bs->drv; -if (!drv) -return -ENOMEDIUM ; -if (drv->bdrv_snapshot_create) -return drv->bdrv_snapshot_create (bs, sn_info); -if (bs->file) -return bdrv_snapshot_create (bs->file, sn_info); -return -ENOTSUP ; +int ret; + +if (!drv) { +error_setg (errp,"Device '%s' has no medium." , +bdrv_get_device_name (bs)); + ret = -ENOMEDIUM ;
block.c BlockDriverState *bdrv_snapshots(void) · line 3097
Quoted 10 lines
block.c +10
-0+ }else if (drv->bdrv_snapshot_create) { + ret = drv->bdrv_snapshot_create (bs, sn_info, errp); + }else if (bs->file) { + ret =bdrv_snapshot_create (bs->file, sn_info, errp); + }else { +error_setg (errp,"Not supported." ); + ret = -ENOTSUP ; + } + +return ret;
This function returns an error code _and_ an error object. This is fine if you drop the error code in a later patch, but you don't do it in this series.
IMO, we shouldn't return both unless there's a strong reason to do so (ie. you need the error code and a very specific error message, or you plan to drop the error code in the future). If this is the case here, please tell us.
When converting functions that return an error code, I check if any caller of the function check for a specific error code. If no caller does this, then I just drop the error code (but in a different patch).
If a caller actually checks the error code, you generally have two options:
1. See if checking the error code is actually required, if it's not then just drop it
2. Don't add the Error object to the function returning the error code. Add it to the callers instead
We (block layer guys and me) had a discussion about this some weeks ago, I don't exactly remember if what I described above matches our conclusions, so I'm CC'ing them.
Quoted 22 lines
block.c +0
-0} int bdrv_snapshot_goto(BlockDriverState *bs,block.h +2
-1@@ -321,7 +321,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);int bdrv_is_snapshot (BlockDriverState *bs);BlockDriverState *bdrv_snapshots (void );int bdrv_snapshot_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info); +QEMUSnapshotInfo *sn_info, +Error **errp);int bdrv_snapshot_goto (BlockDriverState *bs,const char *snapshot_id);int bdrv_snapshot_delete (BlockDriverState *bs,const char *snapshot_id);block/qcow2-snapshot.c +0
-0
block/qcow2-snapshot.c line 28
Quoted 4 lines
block/qcow2-snapshot.c +1
-0@@ -25,6 +25,7 @@#include "qemu-common.h" #include "block_int.h" #include "block/qcow2.h" +#include "qerror.h"
I think you want to include error.h. qerror.h is legacy.
Quoted 14 lines
block/qcow2-snapshot.c +3
-1typedef 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 ;
block/qcow2-snapshot.c int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) · line 337
Quoted 4 lines
block/qcow2-snapshot.c +1
-0@@ -331,6 +334,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)/* Check that the ID is unique */ if (find_snapshot_by_id (bs, sn_info->id_str) >=0 ) { +error_setg (errp,"Parameter 'name' has to be unique ID." );
Here you're converting qcow2_snapshot_create() to propagate the error, and I see that you do it for more functions below.
Basically, this kind of conversion should be split into one per-patch or even one per-series depending on the complexity.
A good series converting do_foo() to take an Error object would do:
1. Add an Error object to do_foo() and fill it properly. Callers of do_foo() should pass NULL for the new argument
2. For each caller of do_foo() create a new patch, which changes the caller to pass an Error object to do_foo() and use it internally
3. If do_foo() returns an error code, drop it
Quoted 238 lines
block/qcow2-snapshot.c +5
-0return -EEXIST ; }@@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) l1_table_offset =qcow2_alloc_clusters (bs, s->l1_size *sizeof (uint64_t ));if (l1_table_offset <0 ) { ret = l1_table_offset; +error_setg (errp,"Failed to allocate L1 table." ); goto fail; }@@ -362,6 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) ret =bdrv_pwrite (bs->file, sn->l1_table_offset, l1_table, s->l1_size *sizeof (uint64_t ));if (ret <0 ) { +error_setg (errp,"Failed to save L1 table." ); goto fail; }@@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)*/ ret =qcow2_update_snapshot_refcount (bs, s->l1_table_offset, s->l1_size,1 );if (ret <0 ) { +error_setg (errp,"Failed to update snapshot refcount." ); goto fail; } ret =bdrv_flush (bs);if (ret <0 ) { +error_setg (errp,"Failed to flush data." ); goto fail; }@@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)if (ret <0 ) {g_free (s->snapshots); s->snapshots = old_snapshot_list; +error_setg (errp,"Failed to write new snapshots." ); goto fail; }block/qcow2.h +3
-1@@ -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);block/rbd.c +11
-6@@ -16,6 +16,7 @@#include "qemu-common.h" #include "qemu-error.h" #include "block_int.h" +#include "qerror.h" #include <rbd/librbd.h> @@ -811,12 +812,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset) }static int qemu_rbd_snap_create (BlockDriverState *bs, -QEMUSnapshotInfo *sn_info) +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVRBDState *s = bs->opaque; -int r; +int ret;if (sn_info->name[0 ] =='\0' ) { +error_setg (errp,"Parameter 'name' cannot be empty." );return -EINVAL ;/* we need a name for rbd snapshots */ }@@ -826,17 +829,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,*/ if (sn_info->id_str[0 ] !='\0' &&strcmp (sn_info->id_str, sn_info->name) !=0 ) { +error_setg (errp,"ID and name have to be equal." );return -EINVAL ; }if (strlen (sn_info->name) >=sizeof (sn_info->id_str)) { +error_setg (errp,"Parameter 'name' has to be shorter that 127 chars." );return -ERANGE ; } - r =rbd_snap_create (s->image, sn_info->name); -if (r <0 ) { -error_report ("failed to create snap: %s" ,strerror (-r)); -return r; + ret =rbd_snap_create (s->image, sn_info->name); +if (ret <0 ) { +error_setg (errp,"Failed to create snapshot." ); +return ret; }return 0 ;block/sheepdog.c +10
-8@@ -17,6 +17,7 @@#include "qemu_socket.h" #include "block_int.h" #include "bitops.h" +#include "qerror.h" #define SD_PROTO_VER 0x01@@ -1735,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)return 0 ; } -static int sd_snapshot_create (BlockDriverState *bs,QEMUSnapshotInfo *sn_info) +static int sd_snapshot_create (BlockDriverState *bs, +QEMUSnapshotInfo *sn_info, +Error **errp) {BDRVSheepdogState *s = bs->opaque;int ret, fd;@@ -1748,9 +1751,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) s->name, sn_info->vm_state_size, s->is_snapshot);if (s->is_snapshot) { -error_report ("You can't create a snapshot of a snapshot VDI, " -"%s (%" PRIu32")." , s->name, s->inode.vdi_id); - +error_setg (errp,"You can't create a snapshot '%s' of a VDI snapshot." , + sn_info->name);return -EINVAL ; }@@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) fd =connect_to_sdog (s->addr, s->port);if (fd <0 ) { ret = fd; +error_setg (errp,"Failed to connect to sdog." ); goto cleanup; } ret =write_object (fd, (char *)&s->inode,vid_to_vdi_oid (s->inode.vdi_id), s->inode.nr_copies, datalen,0 , false, s->cache_enabled);if (ret <0 ) { -error_report ("failed to write snapshot's inode." ); +error_setg (errp,"Failed to write snapshot's inode." ); goto cleanup; } ret =do_sd_create (s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,1 , s->addr, s->port);if (ret <0 ) { -error_report ("failed to create inode for snapshot. %s" , -strerror (errno)); +error_setg (errp,"Failed to create inode for snapshot." ); goto cleanup; }@@ -1793,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) s->inode.nr_copies, datalen, 0, s->cache_enabled);if (ret <0 ) { -error_report ("failed to read new inode info. %s" ,strerror (errno)); +error_setg (errp,"Failed to read new inode info." ); goto cleanup; }block_int.h +2
-1@@ -147,7 +147,8 @@ struct BlockDriver { const uint8_t *buf, int nb_sectors);int (*bdrv_snapshot_create)(BlockDriverState *bs, -QEMUSnapshotInfo *sn_info); +QEMUSnapshotInfo *sn_info, +Error **errp);int (*bdrv_snapshot_goto)(BlockDriverState *bs,const char *snapshot_id);int (*bdrv_snapshot_delete)(BlockDriverState *bs,const char *snapshot_id);qemu-img.c +3
-5@@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)int c, ret =0 , bdrv_oflags;int action =0 ;qemu_timeval tv; +Error *err =NULL ; bdrv_oflags =BDRV_O_FLAGS |BDRV_O_RDWR ;/* Parse commandline parameters */ @@ -1559,11 +1560,7 @@ static int img_snapshot(int argc, char **argv) sn.date_sec = tv.tv_sec; sn.date_nsec = tv.tv_usec *1000 ; - ret =bdrv_snapshot_create (bs, &sn); -if (ret) { -error_report ("Could not create snapshot '%s': %d (%s)" , - snapshot_name, ret,strerror (-ret)); - } + ret =bdrv_snapshot_create (bs, &sn, &err);break ;case SNAPSHOT_APPLY :@@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)/* Cleanup */ bdrv_delete (bs); +handle_error (&err);if (ret) {return 1 ; }savevm.c +1
-1@@ -2211,7 +2211,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)if (bdrv_can_snapshot (bs1)) {/* Write VM state size only to the image that contains the state */ sn->vm_state_size = (bs == bs1 ? vm_state_size :0 ); - ret =bdrv_snapshot_create (bs1, sn); + ret =bdrv_snapshot_create (bs1, sn,NULL );if (ret <0 ) {monitor_printf (mon,"Error while creating snapshot on '%s'\n" ,bdrv_get_device_name (bs1));
Re: [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm
In reply to Pavel HrdinaQuoted 71 lines · On Thu, 13 Dec 2012 16:40:47 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- hmp-commands.hx | 4 ++-- hmp.c | 9 +++++++++ hmp.h | 1 + qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 29 +++++++++++++++++++++++++++++ savevm.c | 26 ++++++++++---------------- sysemu.h | 1 - 7 files changed, 70 insertions(+), 19 deletions(-)hmp-commands.hx +2
-2@@ -310,7 +310,7 @@ ETEXI .args_type = "name:s?", .params = "[tag|id]", .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", - .mhandler.cmd = do_savevm, + .mhandler.cmd = hmp_vm_snapshot_save, }, STEXI@@ -318,7 +318,7 @@ STEXI @findex savevm Create a snapshot of the whole virtual machine. If @var{tag} is provided, it is used as human readable identifier. If there is already -a snapshot with the same tag or ID, it is replaced. More info at +a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at @ref{vm_snapshots}. ETEXIhmp.c +9
-0@@ -1335,3 +1335,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)qmp_nbd_server_stop (&errp);hmp_handle_error (mon, &errp); } + +void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict) +{ +const char *name =qdict_get_try_str (qdict,"name" ); +Error *err =NULL ; + +qmp_vm_snapshot_save (!!name, name, &err); +hmp_handle_error (mon, &err); +}hmp.h +1
-0@@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict);void hmp_nbd_server_start (Monitor *mon,const QDict *qdict);void hmp_nbd_server_add (Monitor *mon,const QDict *qdict);void hmp_nbd_server_stop (Monitor *mon,const QDict *qdict); +void hmp_vm_snapshot_save (Monitor *mon,const QDict *qdict);#endif qapi-schema.json +2
-0@@ -3017,3 +3017,22 @@ # Since: 1.3.0 ## { 'command': 'nbd-server-stop' } + +##
qapi-schema.json line 3031
Quoted 10 lines
qapi-schema.json +10
-0+# @vm-snapshot-save: +# +# Create a snapshot of the whole virtual machine. If tag is provided as @name, +# it is used as human readable identifier. If there is already a snapshot +# with the same tag or ID, it is replaced. +# +# The VM is automatically stopped and resumed and saving a snapshot can take +# a long time. +# +# @name: #optional tag of new snapshot or tag|id of existing snapshot
I wonder if we should allow 'name' to be optional in QMP.
qapi-schema.json line 3034
Quoted 3 lines
qapi-schema.json +3
-0+# +# Returns: Nothing on success +# If an error occurs, GenericError with error message
You can drop this last line.
Quoted 94 lines
qapi-schema.json +4
-0+# +# Since: 1.4.0 +## +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }qmp-commands.hx +29
-0@@ -1348,6 +1348,35 @@ Example: EQMP { + .name = "vm-snapshot-save", + .args_type = "name:s?", + .params = "name", + .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save + }, + +SQMP +vm-snapshot-save +------ + +Create a snapshot of the whole virtual machine. If tag is provided as name, +it is used as human readable identifier. If there is already a snapshot +with the same tag or id, it is replaced. + +The VM is automatically stopped and resumed and saving a snapshot can take +a long time. + +Arguments: + +- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional) + +Example: + +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } +<- { "return": {} } + +EQMP + { .name = "qmp_capabilities", .args_type = "", .params = "",savevm.c +5
-6@@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,return 0 ; } -void do_savevm (Monitor *mon,const QDict *qdict) +void qmp_vm_snapshot_save (bool has_name,const char *name,Error **errp) {BlockDriverState *bs, *bs1;QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;@@ -2156,7 +2156,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)struct timeval tv;struct tm tm;#endif -const char *name =qdict_get_try_str (qdict,"name" );/* Verify if there is a device that doesn't support snapshots and is writable */ bs =NULL ;@@ -2167,15 +2166,15 @@ void do_savevm(Monitor *mon, const QDict *qdict) }if (!bdrv_can_snapshot (bs)) { -monitor_printf (mon,"Device '%s' is writable but does not support snapshots.\n" , -bdrv_get_device_name (bs)); +error_setg (errp,"Device '%s' is writable but does not support " +"snapshots." ,bdrv_get_device_name (bs));return ; } } bs =bdrv_snapshots ();if (!bs) { -monitor_printf (mon,"No block device can accept snapshots\n" ); +error_setg (errp,"No block device can accept snapshots." );return ; }@@ -2196,7 +2195,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)#endif sn->vm_clock_nsec =qemu_get_clock_ns (vm_clock); -if (name) { +if (has_name) { ret =bdrv_snapshot_find (bs, old_sn, name,NULL );if (ret >=0 ) {pstrcpy (sn->name,sizeof (sn->name), old_sn->name);
savevm.c void do_savevm(Monitor *mon, const QDict *qdict) · line 2219
Quoted 5 lines
savevm.c +1
-1@@ -2217,21 +2216,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) }/* Delete old snapshots of the same name */ -if (name &&del_existing_snapshots (name,NULL ) <0 ) { +if (has_name &&del_existing_snapshots (name, errp) <0 ) {
You shouldn't use errp (the argument passed to qmp_vm_snapshot_save) to detect internal error. You should have Error *local_err, pass it instead and propagate errors with error_propagate().
Quoted 45 lines
savevm.c +4
-9goto the_end; }/* save the VM state */ f =qemu_fopen_bdrv (bs,1 );if (!f) { -monitor_printf (mon,"Could not open VM state file\n" ); +error_setg (errp,"Failed to open '%s' file." ,bdrv_get_device_name (bs)); goto the_end; } - ret =qemu_savevm_state (f,NULL ); + ret =qemu_savevm_state (f, errp); vm_state_size =qemu_ftell (f);qemu_fclose (f); -if (ret <0 ) { -monitor_printf (mon,"Error %d while writing VM\n" , ret); +if (error_is_set (errp)) { goto the_end; }@@ -2242,11 +2240,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)if (bdrv_can_snapshot (bs1)) {/* Write VM state size only to the image that contains the state */ sn->vm_state_size = (bs == bs1 ? vm_state_size :0 ); - ret =bdrv_snapshot_create (bs1, sn,NULL ); -if (ret <0 ) { -monitor_printf (mon,"Error while creating snapshot on '%s'\n" , -bdrv_get_device_name (bs1)); - } +bdrv_snapshot_create (bs1, sn, errp); } }sysemu.h +0
-1@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);void qemu_add_machine_init_done_notifier (Notifier *notify); -void do_savevm (Monitor *mon,const QDict *qdict);int load_vmstate (const char *name);void do_delvm (Monitor *mon,const QDict *qdict);void do_info_snapshots (Monitor *mon);
Re: [Qemu-devel] [PATCH v2 13/17] qapi: Convert savevm
Quoted 5 lines · Luiz Capitulino
Quoted 5 lines
qapi-schema.json +5
-0+## +# @vm-snapshot-save: +# +# Create a snapshot of the whole virtual machine. If tag is provided as @name, +# it is used as human readable identifier. If there is already a snapshot
Quoted 8 lines
qapi-schema.json line 3031
Quoted 6 lines
qapi-schema.json +6
-0+# with the same tag or ID, it is replaced. +# +# The VM is automatically stopped and resumed and saving a snapshot can take +# a long time. +# +# @name: #optional tag of new snapshot or tag|id of existing snapshotI wonder if we should allow 'name' to be optional in QMP.
Libvirt will always pass a 'name'. Where it gets confusing is that 'name' can be all digits; so if I say a name of '3', but it gets id '2', then future operations get way confusing (especially if later we also create a snapshot whose id becomes 3).
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+# +# Returns: Nothing on success
One thing is for certain - if 'name' remains optional, then you MUST return the 'id' that was auto-allocated. And even if name is not optional, returning the 'id' that was either auto-allocated or successfully looked up as an existing snapshot would be more useful than returning nothing.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 3 lines · On Thu, 13 Dec 2012 16:40:34 +0100 Pavel Hrdina <[email protected]> wrote:
This patch series convert these commands into qapi and introduce QMP commands vm-snapshot-save, vm-snapshot-load, vm-snapshot-delete and query-vm-snapshots. It also rewrite error report for functions used by these commands.
I've reviewed most of this series and identified several problems, but I think that the major problem here is that this series is doing too much at once.
A properly done and split series would easily reach twice as many patches this series currently has.
So, my main advice here is to start simple. You could take one of savevm, delvm or loadvm and convert it to propagate errors internally.
Also note that this series doesn't build:
CC x86_64-softmmu/savevm.o/home/lcapitulino/work/src/qmp-unstable/savevm.c: In function ‘qmp_vm_snapshot_load’:/home/lcapitulino/work/src/qmp-unstable/savevm.c:2274:9: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
cc1: all warnings being treated as errors
make[1]: *** [savevm.o] Error 1
make: *** [subdir-x86_64-softmmu] Error 2
Re: [Qemu-devel] [PATCH v2 16/17] qapi: Convert info snapshots
In reply to Pavel HrdinaQuoted 40 lines · On Thu, 13 Dec 2012 16:40:50 +0100 Pavel Hrdina <[email protected]> wrote:
Signed-off-by Pavel Hrdina <[email protected]>--- hmp.c | 33 +++++++++++++++++++++++++++++++++ hmp.h | 1 + monitor.c | 2 +- qapi-schema.json | 13 +++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ savevm.c | 52 +++++++++++++++++++++++++--------------------------- sysemu.h | 2 -- 7 files changed, 107 insertions(+), 30 deletions(-)hmp.c +21
-0@@ -628,6 +628,39 @@ void hmp_info_block_jobs(Monitor *mon) } } +void hmp_info_snapshots (Monitor *mon) +{ +SnapshotInfoList *list; +Error *err =NULL ; +char buf[256 ]; +QEMUSnapshotInfo sn; + + list =qmp_query_vm_snapshots (&err); + +if (error_is_set (&err)) { +hmp_handle_error (mon, &err); +return ; + } + +if (!list) { +monitor_printf (mon,"There is no snapshot available.\n" ); +return ; + } +monitor_printf (mon,"%s\n" ,bdrv_snapshot_dump (buf,sizeof (buf),NULL )); +while (list) { +memcpy (&(sn.id_str), list->value->id,sizeof (sn.id_str));
hmp.c void hmp_info_block_jobs(Monitor *mon) · line 661
Quoted 10 lines
hmp.c +10
-0+memcpy (&(sn.name), list->value->name,sizeof (sn.name)); + sn.date_sec = list->value->date_sec; + sn.date_nsec = list->value->date_nsec; + sn.vm_clock_nsec = list->value->vm_clock_nsec; + sn.vm_state_size = list->value->vm_state_size; +monitor_printf (mon,"%s\n" ,bdrv_snapshot_dump (buf,sizeof (buf), &sn)); + list = list->next; + } + +qapi_free_SnapshotInfoList (list);
list should be NULL here, so you're actually leaking the list returned by qmp_query_vm_snapshots().
Quoted 35 lines
hmp.c +2
-0+} +void hmp_quit (Monitor *mon,const QDict *qdict) {monitor_suspend (mon);hmp.h +1
-0@@ -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);monitor.c +1
-1@@ -2596,7 +2596,7 @@ static mon_cmd_t info_cmds[] = { .args_type = "", .params = "", .help = "show the currently saved VM snapshots", - .mhandler.info = do_info_snapshots, + .mhandler.info = hmp_info_snapshots, }, { .name = "status",qapi-schema.json +0
-0
qapi-schema.json line 1233
Quoted 10 lines
qapi-schema.json +7
-0@@ -1224,6 +1224,19 @@ { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] } ## +# @query-vm-snapshots: +# +# List available snapshots for VM. +# +# Returns: an array of @SnapshotInfo describing each snapshot or an empty array +# on success +# If an error occurs, GenericError with error message
We don't need the line above.
Quoted 73 lines
qapi-schema.json +6
-0+# +# Since: 1.3 +## +{ 'command': 'query-vm-snapshots', 'returns': ['SnapshotInfo'] } + +## # @quit: # # This command will cause the QEMU process to exit gracefully. While everyqmp-commands.hx +34
-0@@ -2732,3 +2732,37 @@ EQMP .args_type = "", .mhandler.cmd_new = qmp_marshal_input_query_target, }, + +SQMP +query-vm-snapshots +---------- + +List available snapshots for VM. + +Return an array of json-object, each with the following information: + +- "id": unique snapshot id + +- "name": user choosen name + +- "vm-state-size": size of the VM state + +- "date-sec": UTC date of the snapshot + +- "date-nsec": date in nano seconds + +- "vm-clock-nsec": VM clock relative to boot in nano seconds + +Example: + +-> { "execute": "query-vm-snapshots" } +<- {"return": [{"vm-clock-nsec": 36420253254, "name": "my_snapshot", + "date-sec": 1345120008, "date-nsec": 151984000, "id": "6", + "vm-state-size": 166515500}]} + +EQMP + { + .name = "query-vm-snapshots", + .args_type = "", + .mhandler.cmd_new = qmp_marshal_input_query_vm_snapshots, + },savevm.c +2
-5@@ -2373,34 +2373,28 @@ void qmp_vm_snapshot_delete(const char *name, Error **errp) } } -void do_info_snapshots (Monitor *mon) +SnapshotInfoList *qmp_query_vm_snapshots (Error **errp) { +SnapshotInfoList *snapshot_list =NULL , *last =NULL ;BlockDriverState *bs, *bs1;QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;int nb_sns, i, ret, available; -int total; -int *available_snapshots; -char buf[256 ]; bs =bdrv_snapshots ();if (!bs) { -monitor_printf (mon,"No available block device supports snapshots\n" );
savevm.c void qmp_vm_snapshot_delete(const char *name, Error **errp) · line 2389
Quoted 10 lines
savevm.c +3
-5-return ; +error_setg (errp,"No block device supports snapshots." ); +return NULL ; } - nb_sns =bdrv_snapshot_list (bs, &sn_tab,NULL ); -if (nb_sns <0 ) { -monitor_printf (mon,"bdrv_snapshot_list: error %d\n" , nb_sns); -return ; + nb_sns =bdrv_snapshot_list (bs, &sn_tab, errp);
You should not use errp to detect internal errors.
Quoted 70 lines
savevm.c +20
-17+if (error_is_set (errp)) { +return NULL ; }if (nb_sns ==0 ) { -monitor_printf (mon,"There is no snapshot available.\n" ); -return ; +return NULL ; } - available_snapshots =g_malloc0 (sizeof (int ) * nb_sns); - total =0 ;for (i =0 ; i < nb_sns; i++) { sn = &sn_tab[i]; available =1 ;@@ -2417,24 +2411,28 @@ void do_info_snapshots(Monitor *mon) }if (available) { - available_snapshots[total] = i; - total++; - } - } - -if (total >0 ) { -monitor_printf (mon,"%s\n" ,bdrv_snapshot_dump (buf,sizeof (buf),NULL )); -for (i =0 ; i < total; i++) { - sn = &sn_tab[available_snapshots[i]]; -monitor_printf (mon,"%s\n" ,bdrv_snapshot_dump (buf,sizeof (buf), sn)); +SnapshotInfoList *info =g_malloc0 (sizeof (*info)); + info->value =g_malloc0 (sizeof (*info->value)); + info->value->id =g_strdup (sn->id_str); + info->value->name =g_strdup (sn->name); + info->value->vm_state_size = sn->vm_state_size; + info->value->date_sec = sn->date_sec; + info->value->date_nsec = sn->date_nsec; + info->value->vm_clock_nsec = sn->vm_clock_nsec; + +if (!snapshot_list) { + snapshot_list = info; + last = info; + }else { + last->next = info; + last = info; + } } - }else { -monitor_printf (mon,"There is no suitable snapshot available\n" ); }g_free (sn_tab); -g_free (available_snapshots); +return snapshot_list; }void vmstate_register_ram (MemoryRegion *mr,DeviceState *dev)sysemu.h +0
-2@@ -65,8 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);void qemu_add_machine_init_done_notifier (Notifier *notify); -void do_info_snapshots (Monitor *mon); -void qemu_announce_self (void );bool qemu_savevm_state_blocked (Error **errp);
Re: [Qemu-devel] [PATCH v2 14/17] qapi: Convert loadvm
In reply to Pavel HrdinaQuoted 17 lines · Pavel Hrdina
Signed-off-by Pavel Hrdina <[email protected]>--- hmp-commands.hx | 4 ++-- hmp.c | 9 ++++++++ hmp.h | 1 + monitor.c | 12 ----------- qapi-schema.json | 15 +++++++++++++ qmp-commands.hx | 25 ++++++++++++++++++++++ savevm.c | 65 ++++++++++++++++++++++++++++---------------------------- sysemu.h | 1 - vl.c | 6 +++++- 9 files changed, 90 insertions(+), 48 deletions(-)qapi-schema.json +3
-0+ +## +# @vm-snapshot-load:
qapi-schema.json line 3051
Quoted 10 lines
qapi-schema.json +10
-0+# +# Set the whole virtual machine to the snapshot identified by the tag +# or the unique snapshot id. It must be snapshot of whole VM. +# +# @name: tag|id of existing snapshot +# +# Returns: Nothing on success +# If an error occurs, GenericError with error message +# +# Since: 1.3
1.4, now
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 15/17] qapi: Convert delvm
In reply to Pavel HrdinaQuoted 13 lines · Pavel Hrdina
Signed-off-by Pavel Hrdina <[email protected]>--- 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(-)qapi-schema.json +1
-0+##
qapi-schema.json line 3065
Quoted 10 lines
qapi-schema.json +10
-0+# @vm-snapshot-delete: +# +# Delete the snapshot identified by tag or id. +# +# @name: tag|id of existing snapshot +# +# Returns: Nothing on success +# If an error occurs, GenericError with error message +# +# Since: 1.3
Another case of 1.4.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 6 lines
We should also merge functionality of migrations and vm-snapshots and make some clean-ups of this code. I could do it as another patch. With this rewrite we could make vm-snapshots asynchronous.
Hi, Pavel
For vm-snapshots asynchronous, I have sent a RFC patch for it at:
http://lists.nongnu.org/archive/html/qemu-devel/2012-12/msg00800.html
and seems [email protected] also have his own solution at:
https://git.proxmox.com/?p=pve-qemu-kvm.git;a=blob;f=debian/patches/internal-snapshot-async.patch;
So I am reworking on it to include Dietmar's solution, My plan
will include three steps:
1) abstract a function for internal/external snapshots(block only).
2) abstract a function for internal/external live vmstate save.
3) assemble above functions at will at above layer.
patch for 1) is sent at
http://lists.nongnu.org/archive/html/qemu-devel/2012-12/msg02393.html
I agree that code should be merged as much as possible, just hope
my patch wouldn't conflict with yours patch about vmstate save. For vmstate it have a issue about how to limit the file size to be written, that is predict the size to write. We may need a discuss about that.