From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54082) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StPQh-0001dr-Mz for qemu-devel@nongnu.org; Mon, 23 Jul 2012 16:40:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1StPQf-0006nH-O0 for qemu-devel@nongnu.org; Mon, 23 Jul 2012 16:40:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46348) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StPQf-0006nC-Ca for qemu-devel@nongnu.org; Mon, 23 Jul 2012 16:40:33 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6NKeW1p008172 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 23 Jul 2012 16:40:32 -0400 Date: Mon, 23 Jul 2012 17:41:08 -0300 From: Luiz Capitulino Message-ID: <20120723174108.744f835e@doriath.home> In-Reply-To: <03cf631acdeac4654b1d7d8505d9d5f133ea6e6c.1342092497.git.phrdina@redhat.com> References: <03cf631acdeac4654b1d7d8505d9d5f133ea6e6c.1342092497.git.phrdina@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: qemu-devel@nongnu.org On Thu, 12 Jul 2012 18:55:15 +0200 Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > hmp-commands.hx | 2 +- > hmp.c | 10 ++++++++++ > hmp.h | 1 + > qapi-schema.json | 22 ++++++++++++++++++++++ > qerror.c | 24 ++++++++++++++++++++++++ > qerror.h | 18 ++++++++++++++++++ > qmp-commands.hx | 26 ++++++++++++++++++++++++++ > savevm.c | 29 ++++++++++++++--------------- > sysemu.h | 1 - > 9 files changed, 116 insertions(+), 17 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index f5d9d91..e8c5325 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -267,7 +267,7 @@ ETEXI > .args_type = "name:s?", > .params = "[tag|id]", > .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", > - .mhandler.cmd = do_savevm, > + .mhandler.cmd = hmp_savevm, > }, > > STEXI > diff --git a/hmp.c b/hmp.c > index 4c6d4ae..491599d 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1002,3 +1002,13 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) > qmp_netdev_del(id, &err); > hmp_handle_error(mon, &err); > } > + > +void hmp_savevm(Monitor *mon, const QDict *qdict) > +{ > + const char *name = qdict_get_try_str(qdict, "name"); > + Error *err = NULL; > + > + qmp_savevm(!!name, name, &err); > + > + hmp_handle_error(mon, &err); > +} > diff --git a/hmp.h b/hmp.h > index 79d138d..dc6410b 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); > void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); > void hmp_netdev_add(Monitor *mon, const QDict *qdict); > void hmp_netdev_del(Monitor *mon, const QDict *qdict); > +void hmp_savevm(Monitor *mon, const QDict *qdict); > > #endif > diff --git a/qapi-schema.json b/qapi-schema.json > index 1ab5dbd..4db1447 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1868,3 +1868,25 @@ > # Since: 0.14.0 > ## > { 'command': 'netdev_del', 'data': {'id': 'str'} } > + > +## > +# @savevm: Let's call it save-vm. > +# > +# Create a snapshot of the whole virtual machine. If 'tag' is provided, > +# it is used as human readable identifier. If there is already a snapshot > +# with the same tag or ID, it is replaced. Please, document that the vm is automatically stopped and resumed, and that this can take a long time. > +# > +# @name: tag or id of new or existing snapshot I don't like the idea of making 'name' optional in QMP. We could (and should) have it as optional in HMP though. This means that we should move the code that auto-generates it to HMP. Also, I remember an old discussion about distinguishing between 'tag' and 'id'. I remember it was difficult to do, so we could choose not to do it, but let's re-evaluate this again. > +# > +# Returns: Nothing on success > +# If there is a writable device not supporting snapshots, > +# SnapshotNotSupported > +# If no block device can accept snapshots, SnapshotNotAccepted > +# If an error occures while creating a snapshot, SnapshotCreateFailed > +# If open a block device for vm state fail, SnapshotOpenFailed > +# If an error uccures while writing vm state, SnapshotWriteFailed > +# If delete snapshot with same 'name' fail, SnapshotDeleteFailed I don't think we should re-create all these errors, more comments on that below. > +# > +# Since: 1.2 > +## > +{ 'command': 'savevm', 'data': {'*name': 'str'} } > \ No newline at end of file > diff --git a/qerror.c b/qerror.c > index 92c4eff..4e6efa1 100644 > --- a/qerror.c > +++ b/qerror.c > @@ -309,6 +309,30 @@ static const QErrorStringTable qerror_table[] = { > .desc = "Could not start VNC server on %(target)", > }, > { > + .error_fmt = QERR_SNAPSHOT_CREATE_FAILED, > + .desc = "Error '%(errno)' while creating snapshot on '%(device)'", > + }, > + { > + .error_fmt = QERR_SNAPSHOT_DELETE_FAILED, > + .desc = "Error '%(errno)' while deleting snapshot on '%(device)'", > + }, > + { > + .error_fmt = QERR_SNAPSHOT_NOT_ACCEPTED, > + .desc = "No block device can accept snapshots", > + }, > + { > + .error_fmt = QERR_SNAPSHOT_NOT_SUPPORTED, > + .desc = "Device '%(device)' is writable but does not support snapshots", > + }, > + { > + .error_fmt = QERR_SNAPSHOT_OPEN_FAILED, > + .desc = "Error while opening snapshot on '%(device)'", > + }, > + { > + .error_fmt = QERR_SNAPSHOT_WRITE_FAILED, > + .desc = "Error '%(errno)' while writing snapshot on '%(device)'", > + }, > + { > .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS, > .desc = "Connection can not be completed immediately", > }, > diff --git a/qerror.h b/qerror.h > index b4c8758..bc47f4a 100644 > --- a/qerror.h > +++ b/qerror.h > @@ -251,6 +251,24 @@ QError *qobject_to_qerror(const QObject *obj); > #define QERR_VNC_SERVER_FAILED \ > "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }" > > +#define QERR_SNAPSHOT_CREATE_FAILED \ > + "{ 'class': 'SnapshotCreateFailed', 'data': { 'device': %s, 'errno': %d } }" > + > +#define QERR_SNAPSHOT_DELETE_FAILED \ > + "{ 'class': 'SnapshotDeleteFailed', 'data': { 'device': %s, 'errno': %d } }" > + > +#define QERR_SNAPSHOT_NOT_ACCEPTED \ > + "{ 'class': 'SnapshotNotAccepted', 'data': {} }" > + > +#define QERR_SNAPSHOT_NOT_SUPPORTED \ > + "{ 'class': 'SnapshotNotSupported', 'data': { 'device': %s } }" > + > +#define QERR_SNAPSHOT_OPEN_FAILED \ > + "{ 'class': 'SnapshotOpenFailed', 'data': { 'device': %s } }" > + > +#define QERR_SNAPSHOT_WRITE_FAILED \ > + "{ 'class': 'SnapshotWriteFailed', 'data': { 'device': %s, 'errno': %d } }" > + > #define QERR_SOCKET_CONNECT_IN_PROGRESS \ > "{ 'class': 'SockConnectInprogress', 'data': {} }" > > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 2e1a38e..a1c82f7 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1061,6 +1061,32 @@ Example: > > EQMP > { > + .name = "savevm", > + .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_savevm > + }, > + > +SQMP > +savevm > +------ > + > +Create a snapshot of the whole virtual machine. If 'tag' is provided, > +it is used as human readable identifier. If there is already a snapshot > +with the same tag or ID, it is replaced. > + > +Arguments: > + > +- "name": tag or id of new or existing snapshot > + > +Example: > + > +-> { "execute": "savevm", "arguments": { "name": "my_snapshot" } } > +<- { "return": {} } > + > +EQMP > + { > .name = "qmp_capabilities", > .args_type = "", > .params = "", > diff --git a/savevm.c b/savevm.c > index a15c163..9fc1b53 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2033,7 +2033,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > /* > * 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(Error **errp, const char *name) > { > BlockDriverState *bs; > QEMUSnapshotInfo sn1, *snapshot = &sn1; > @@ -2046,9 +2046,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > { > ret = bdrv_snapshot_delete(bs, name); > if (ret < 0) { > - monitor_printf(mon, > - "Error while deleting snapshot on '%s'\n", > - bdrv_get_device_name(bs)); > + error_set(errp, QERR_SNAPSHOT_DELETE_FAILED, > + bdrv_get_device_name(bs), ret); The Right Thing to do here is to change bdrv_snapshot_delete() to take an Error argument and let it set the real error cause. Three considerations: 1. The QMP command shouldn't delete existing snapshots by default. Either, we add a 'force' argument to it or don't delete snapshots in save-vm at all (in which case a mngt app would have to delete the snapshots with the same name manually, I prefer this approach btw) 2. This has to be done in a separate series 3. It's a good idea to wait for my series improving error_set() to be merged before doing this > return -1; > } > } > @@ -2057,7 +2056,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > return 0; > } > > -void do_savevm(Monitor *mon, const QDict *qdict) > +void qmp_savevm(bool has_name, const char *name, Error **errp) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; > @@ -2072,7 +2071,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; > @@ -2083,15 +2081,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_set(errp, QERR_SNAPSHOT_NOT_SUPPORTED, > + bdrv_get_device_name(bs)); Please, use QERR_NOT_SUPPORTED instead. I know it doesn't allow any arguments today, but I'm working on a series which will allow you to pass any arguments you wish. > return; > } > } > > bs = bdrv_snapshots(); > if (!bs) { > - monitor_printf(mon, "No block device can accept snapshots\n"); > + error_set(errp, QERR_SNAPSHOT_NOT_ACCEPTED); I think we should use QERR_NOT_SUPPORTED here too. > return; > } > > @@ -2112,7 +2110,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); > if (ret >= 0) { > pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > @@ -2133,21 +2131,22 @@ void do_savevm(Monitor *mon, const QDict *qdict) > } > > /* Delete old snapshots of the same name */ > - if (name && del_existing_snapshots(mon, name) < 0) { > + if (has_name && del_existing_snapshots(errp, name) < 0) { > goto the_end; > } The QMP command should not delete existing snapshots, as said above. > > /* save the VM state */ > f = qemu_fopen_bdrv(bs, 1); > if (!f) { > - monitor_printf(mon, "Could not open VM state file\n"); > + error_set(errp, QERR_SNAPSHOT_OPEN_FAILED, bdrv_get_device_name(bs)); Please, use QERR_OPEN_FILE_FAILED instead. The filename should be the backing file name. > goto the_end; > } > ret = qemu_savevm_state(f); > vm_state_size = qemu_ftell(f); > qemu_fclose(f); > if (ret < 0) { > - monitor_printf(mon, "Error %d while writing VM\n", ret); > + error_set(errp, QERR_SNAPSHOT_WRITE_FAILED, > + bdrv_get_device_name(bs), ret); The Right Thing to do here it to convert qemu_savevm_sstate() to take an Error argument. The same considerations I wrote above about del_existing_snapshots() apply here, except that you can report IOError for most qemu_savevm_state() errors. > goto the_end; > } > > @@ -2160,8 +2159,8 @@ void do_savevm(Monitor *mon, const QDict *qdict) > sn->vm_state_size = (bs == bs1 ? vm_state_size : 0); > ret = bdrv_snapshot_create(bs1, sn); > if (ret < 0) { > - monitor_printf(mon, "Error while creating snapshot on '%s'\n", > - bdrv_get_device_name(bs1)); > + error_set(errp, QERR_SNAPSHOT_CREATE_FAILED, > + bdrv_get_device_name(bs1), ret); Here too, bdrv_snapshot_create() should be changed to take an Error argument. > } > } > } > diff --git a/sysemu.h b/sysemu.h > index 6540c79..95d1207 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -69,7 +69,6 @@ void qemu_remove_exit_notifier(Notifier *notify); > > void qemu_add_machine_init_done_notifier(Notifier *notify); > > -void do_savevm(Monitor *mon, const QDict *qdict); > int load_vmstate(const char *name); > void do_delvm(Monitor *mon, const QDict *qdict); > void do_info_snapshots(Monitor *mon);