From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58589) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StQO0-00044o-V3 for qemu-devel@nongnu.org; Mon, 23 Jul 2012 17:41:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1StQNu-00070L-9p for qemu-devel@nongnu.org; Mon, 23 Jul 2012 17:41:52 -0400 Received: from mail-lb0-f173.google.com ([209.85.217.173]:50994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StQNt-000703-JA for qemu-devel@nongnu.org; Mon, 23 Jul 2012 17:41:46 -0400 Received: by lbok6 with SMTP id k6so9059517lbo.4 for ; Mon, 23 Jul 2012 14:41:44 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: Date: Tue, 24 Jul 2012 05:41:42 +0800 Message-ID: From: Paulo Arcinas Content-Type: multipart/alternative; boundary=bcaec554d63ca6929d04c5861c82 Subject: Re: [Qemu-devel] Qemu-devel Digest, Vol 112, Issue 561 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org --bcaec554d63ca6929d04c5861c82 Content-Type: text/plain; charset=ISO-8859-1 > > Message: 1 > Date: Mon, 23 Jul 2012 19:44:44 +0000 > From: Blue Swirl > To: Eduardo Habkost > Cc: Anthony Liguori , Igor Mammedov > , seabios@seabios.org, qemu-devel@nongnu.org, > Gleb Natapov > Subject: Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC > ID utility functions > Message-ID: > < CAAu8pHuOBPLz7kkq0o7vUxn6dVd2_dd8bvsGHMW8qDBKsaPXPA@mail.gmail.com> > Content-Type: text/plain; charset=UTF-8 > > On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost wrote: > > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote: > >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost wrote: > >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: > >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost < ehabkost@redhat.com> wrote: > >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: > >> >> > [...] > >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile > >> >> >> >> > index b605e14..89bd890 100644 > >> >> >> >> > --- a/tests/Makefile > >> >> >> >> > +++ b/tests/Makefile > >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) > >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) > >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF) > >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > >> >> >> >> > >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets > >> >> >> >> and break them all. > >> >> >> > > >> >> >> > I don't think there's any concept of "targets" for the check-unit tests. > >> >> >> > >> >> >> How about: > >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > >> >> > > >> >> > test-x86-cpuid is not a qtest test case. > >> >> > >> >> Why not? I don't think it is a unit test either, judging from what the > >> >> other unit tests do. > >> > > >> > It is absolutely a unit test. I don't know why you don't think so. It > >> > simply checks the results of the APIC ID calculation functions. > >> > >> Yes, but the other 'unit tests' (the names used here are very > >> confusing, btw) check supporting functions like coroutines, not > >> hardware. Hardware tests (qtest) need to bind to an architecture, in > >> this case x86. > > > > test-x86-cpuid doesn't check hardware, either. It just checks the simple > > functions that calculate APIC IDs (to make sure the math is correct). > > Those functions aren't even used by any hardware emulation code until > > later in the patch series (when the CPU initialization code gets changed > > to use the new function). > > By that time the function is clearly x86 HW and the check is an x86 > hardware check. QEMU as whole consists of simple functions that > calculate something. > > > > > > -- > > Eduardo > > > > ------------------------------ > > Message: 2 > Date: Mon, 23 Jul 2012 16:46:37 -0300 > From: Luiz Capitulino > To: Markus Armbruster > Cc: jan.kiszka@siemens.com, aliguori@us.ibm.com, > qemu-devel@nongnu.org, afaerber@suse.de > Subject: Re: [Qemu-devel] [PATCH 1/8] qemu-option: > qemu_opt_set_bool(): fix code duplication > Message-ID: <20120723164637.01863346@doriath.home> > Content-Type: text/plain; charset=US-ASCII > > On Mon, 23 Jul 2012 20:14:31 +0200 > Markus Armbruster wrote: > > > Luiz Capitulino writes: > > > > > On Sat, 21 Jul 2012 10:09:09 +0200 > > > Markus Armbruster wrote: > > > > > >> Luiz Capitulino writes: > > >> > > >> > Call qemu_opt_set() instead of duplicating opt_set(). > > >> > > > >> > Signed-off-by: Luiz Capitulino > > >> > --- > > >> > qemu-option.c | 28 +--------------------------- > > >> > 1 file changed, 1 insertion(+), 27 deletions(-) > > >> > > > >> > diff --git a/qemu-option.c b/qemu-option.c > > >> > index bb3886c..2cb2835 100644 > > >> > --- a/qemu-option.c > > >> > +++ b/qemu-option.c > > >> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char > > >> > *name, const char *value, > > >> > > > >> > int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > > >> > { > > >> > - QemuOpt *opt; > > >> > - const QemuOptDesc *desc = opts->list->desc; > > >> > - int i; > > >> > - > > >> > - for (i = 0; desc[i].name != NULL; i++) { > > >> > - if (strcmp(desc[i].name, name) == 0) { > > >> > - break; > > >> > - } > > >> > - } > > >> > - if (desc[i].name == NULL) { > > >> > - if (i == 0) { > > >> > - /* empty list -> allow any */; > > >> > - } else { > > >> > - qerror_report(QERR_INVALID_PARAMETER, name); > > >> > - return -1; > > >> > - } > > >> > - } > > >> > - > > >> > - opt = g_malloc0(sizeof(*opt)); > > >> > - opt->name = g_strdup(name); > > >> > - opt->opts = opts; > > >> > - QTAILQ_INSERT_TAIL(&opts->head, opt, next); > > >> > - if (desc[i].name != NULL) { > > >> > - opt->desc = desc+i; > > >> > - } > > >> > - opt->value.boolean = !!val; > > >> > - return 0; > > >> > + return qemu_opt_set(opts, name, val ? "on" : "off"); > > >> > } > > >> > > > >> > int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, > > >> > > >> Does a bit more than just obvious code de-duplication. Two things in > > >> particular: > > >> > > >> * Error reporting > > >> > > >> Old: qerror_report(); return -1 > > >> > > >> New: opt_set() and qemu_opt_set() cooperate, like this: > > >> opt_set(): error_set(); return; > > >> qemu_opt_set(): > > >> if (error_is_set(&local_err)) { > > >> qerror_report_err(local_err); > > >> error_free(local_err); > > >> return -1; > > >> } > > >> > > >> I guess the net effect is the same. Not sure it's worth mentioning in > > >> the commit message. > > > > > > The end result of calling qemu_opt_set_bool() is the same. The difference > > > between qerror_report() and qerror_report_err() is that the former gets error > > > information from the error call, while the latter gets error information > > > from the Error object. But they do the same thing. > > > > > >> * New sets opt->str to either "on" or "off" depending on val, then lets > > >> reconstructs the value with qemu_opt_parse(). Which can't fail then. > > >> I figure the latter part has no further impact. But what about > > >> setting opt->str? Is this a bug fix? > > > > > > I don't remember if opt->str is read after the QemuOpt object is built. If > > > it's, then yes, this is a bug fix. Otherwise it just make the final > > > QemuOpt object more 'conforming'. > > > > Uses of opt->str, and what happens when it isn't set: > > > > * qemu_opt_get(): returns NULL, which means "not set". Bug can bite > > when value isn't the default value. > > > > * qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it > > like "on". Wrong if the value is actually false. Bug can bite when > > qemu_opts_validate() runs after qemu_opt_set_bool(). > > > > * qemu_opt_del(): passes NULL to g_free(), which is just fine. > > > > * qemu_opt_foreach(): passes NULL to the callback, which is unlikely to > > be prepared for it. > > > > * qemu_opts_print(): prints NULL, which crashes on some systems. > > > > * qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which > > crashes. > > Thanks for the clarification. > > > > > Right now, the only use of qemu_opt_set_bool() is the one in vl.c. > > Can't see how to break it, but I didn't look hard. > > > > I recommend to document the bug fix in the commit message. > > Ok, will do. > > > > ------------------------------ > > Message: 3 > Date: Mon, 23 Jul 2012 17:00:02 -0300 > From: Luiz Capitulino > To: Markus Armbruster > Cc: jan.kiszka@siemens.com, aliguori@us.ibm.com, > qemu-devel@nongnu.org, afaerber@suse.de > Subject: Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support > Message-ID: <20120723170002.510fd7f0@doriath.home> > Content-Type: text/plain; charset=US-ASCII > > On Mon, 23 Jul 2012 20:33:52 +0200 > Markus Armbruster wrote: > > > Luiz Capitulino writes: > > > > > On Sat, 21 Jul 2012 10:11:39 +0200 > > > Markus Armbruster wrote: > > > > > >> Luiz Capitulino writes: > > >> > > >> > Allow for specifying an alias for each option name, see next commits > > >> > for examples. > > >> > > > >> > Signed-off-by: Luiz Capitulino > > >> > --- > > >> > qemu-option.c | 5 +++-- > > >> > qemu-option.h | 1 + > > >> > 2 files changed, 4 insertions(+), 2 deletions(-) > > >> > > > >> > diff --git a/qemu-option.c b/qemu-option.c > > >> > index 65ba1cf..b2f9e21 100644 > > >> > --- a/qemu-option.c > > >> > +++ b/qemu-option.c > > >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc, > > >> > int i; > > >> > > > >> > for (i = 0; desc[i].name != NULL; i++) { > > >> > - if (strcmp(desc[i].name, name) == 0) { > > >> > + if (strcmp(desc[i].name, name) == 0 || > > >> > + (desc[i].alias && strcmp(desc[i].alias, name) == 0)) { > > >> > return &desc[i]; > > >> > } > > >> > } > > >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, > > > > static void opt_set(QemuOpts *opts, const char *name, const > > bool prepend, Error **errp) > > { > > QemuOpt *opt; > > const QemuOptDesc *desc; > > Error *local_err = NULL; > > > > desc = find_desc_by_name(opts->list->desc, name); > > if (!desc && !opts_accepts_any(opts)) { > > error_set(errp, QERR_INVALID_PARAMETER, name); > > return; > > >> > } > > >> > > > >> > opt = g_malloc0(sizeof(*opt)); > > >> > - opt->name = g_strdup(name); > > >> > + opt->name = g_strdup(desc ? desc->name : name); > > >> > opt->opts = opts; > > >> > if (prepend) { > > >> > QTAILQ_INSERT_HEAD(&opts->head, opt, next); > > >> > > >> Are you sure this hunk belongs to this patch? If yes, please explain > > >> why :) > > > > > > Yes, I think it's fine because the change that makes it necessary to choose > > > between desc->name and name is introduced by the hunk above. > > > > > > > I think I now get why you have this hunk: > > > > We reach it only if the parameter with this name exists (desc non-null), > > or opts accepts anthing (opts_accepts_any(opts). > > > > If it exists, name equals desc->name before your patch. But afterwards, > > it could be either desc->name or desc->alias. You normalize to > > desc->name. > > > > Else, all we can do is stick to name. > > > > Correct? > > Yes. > > > If yes, then "normal" options and "accept any" options behave > > differently: the former set opts->name to the canonical name, even when > > the user uses an alias. The latter set opts->name to whatever the user > > uses. I got a bad feeling about that. > > Why? Or, more importantly, how do you think we should do it? > > For 'normal' options, the alias is just a different name to refer to the > option name. At least in theory, it shouldn't matter that the option was > set through the alias. > > For 'accept any' options, it's up to the code handling the options know > what to do with it. It can also support aliases if it wants to, or just > refuse it. > > > > ------------------------------ > > Message: 4 > Date: Mon, 23 Jul 2012 22:07:27 +0200 > From: Laszlo Ersek > To: Stefan Hajnoczi > Cc: Paolo Bonzini , Zhi Yong Wu > , qemu-devel@nongnu.org, Zhi Yong Wu > > Subject: Re: [Qemu-devel] [PATCH 06/16] net: Convert qdev_prop_vlan to > peer with hub > Message-ID: <500DAEFF.7060708@redhat.com> > Content-Type: text/plain; charset=ISO-8859-1 > > On 07/20/12 14:01, Stefan Hajnoczi wrote: > > > @@ -638,11 +642,17 @@ static void get_vlan(Object *obj, Visitor *v, void *opaque, > > { > > DeviceState *dev = DEVICE(obj); > > Property *prop = opaque; > > - VLANState **ptr = qdev_get_prop_ptr(dev, prop); > > - int64_t id; > > + VLANClientState **ptr = qdev_get_prop_ptr(dev, prop); > > + int64_t id = -1; > > > > - id = *ptr ? (*ptr)->id : -1; > > - visit_type_int64(v, &id, name, errp); > > + if (*ptr) { > > + unsigned int hub_id; > > + if (!net_hub_id_for_client(*ptr, &hub_id)) { > > + id = (int64_t)hub_id; > > + } > > + } > > + > > + visit_type_int(v, &id, name, errp); > > } > > Should we use uint32 here? (No particular reason, just for "cleanliness" > or whatever.) > > Thanks, > Laszlo > > > > ------------------------------ > > Message: 5 > Date: Mon, 23 Jul 2012 17:14:46 -0300 > From: Eduardo Habkost > To: Blue Swirl > Cc: Anthony Liguori , Igor Mammedov > , seabios@seabios.org, qemu-devel@nongnu.org, > Gleb Natapov > Subject: Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC > ID utility functions > Message-ID: <20120723201446.GX13029@otherpad.lan.raisama.net> > Content-Type: text/plain; charset=us-ascii > > On Mon, Jul 23, 2012 at 07:44:44PM +0000, Blue Swirl wrote: > > On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost wrote: > > > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote: > > >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost wrote: > > >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: > > >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost < ehabkost@redhat.com> wrote: > > >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: > > >> >> > [...] > > >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile > > >> >> >> >> > index b605e14..89bd890 100644 > > >> >> >> >> > --- a/tests/Makefile > > >> >> >> >> > +++ b/tests/Makefile > > >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > > >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) > > >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) > > >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF) > > >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > > >> >> >> >> > > >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets > > >> >> >> >> and break them all. > > >> >> >> > > > >> >> >> > I don't think there's any concept of "targets" for the check-unit tests. > > >> >> >> > > >> >> >> How about: > > >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > > >> >> > > > >> >> > test-x86-cpuid is not a qtest test case. > > >> >> > > >> >> Why not? I don't think it is a unit test either, judging from what the > > >> >> other unit tests do. > > >> > > > >> > It is absolutely a unit test. I don't know why you don't think so. It > > >> > simply checks the results of the APIC ID calculation functions. > > >> > > >> Yes, but the other 'unit tests' (the names used here are very > > >> confusing, btw) check supporting functions like coroutines, not > > >> hardware. Hardware tests (qtest) need to bind to an architecture, in > > >> this case x86. > > > > > > test-x86-cpuid doesn't check hardware, either. It just checks the simple > > > functions that calculate APIC IDs (to make sure the math is correct). > > > Those functions aren't even used by any hardware emulation code until > > > later in the patch series (when the CPU initialization code gets changed > > > to use the new function). > > > > By that time the function is clearly x86 HW and the check is an x86 > > hardware check. QEMU as whole consists of simple functions that > > calculate something. > > It's useful onily for a specific architecture, yes, but it surely > doesn't make libqtest necessary. > > That's the difference between the unit tests and qtest test cases: unit > tests simply test small parts of the code (that may or may not be > hardware-specific), and qtest tests hardware emulation after starting up > a whole qemu process. It's a different kind of testing, with different > sets of goals. > > I suppose you are not arguing that no function inside target-* would be > ever allowed to have a unit test written for it. That would be a very > silly constraint for people writing tests. > > -- > Eduardo > > > > ------------------------------ > > Message: 6 > Date: Mon, 23 Jul 2012 17:41:08 -0300 > From: Luiz Capitulino > To: Pavel Hrdina > Cc: qemu-devel@nongnu.org > Subject: Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm > Message-ID: <20120723174108.744f835e@doriath.home> > Content-Type: text/plain; charset=US-ASCII > > 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); > > > > > ------------------------------ > > _______________________________________________ > Qemu-devel mailing list > Qemu-devel@nongnu.org > https://lists.nongnu.org/mailman/listinfo/qemu-devel > > > End of Qemu-devel Digest, Vol 112, Issue 561 > ******************************************** --bcaec554d63ca6929d04c5861c82 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

>
> Message: 1
> Date: Mon, 23 Jul 2012 19:44:44 +0000
> From: Blue Swirl <blauwirbe= l@gmail.com>
> To: Eduardo Habkost <ehabkos= t@redhat.com>
> Cc: Anthony Liguori <antho= ny@codemonkey.ws>, =A0 =A0Igor Mammedov
> =A0 =A0 =A0 =A0 <imammedo@re= dhat.com>, seabios@seabios.or= g, =A0 =A0 qemu-devel@nongnu.o= rg,
> =A0 =A0 =A0 =A0 Gleb Natapov <gl= eb@redhat.com>
> Subject: Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & AP= IC
> =A0 =A0 =A0 =A0 ID utility functions
> Message-ID:
> =A0 =A0 =A0 =A0 <CAAu8pHuOBPLz7kkq0o7vUxn6dVd2_dd8bvsGHM= W8qDBKsaPXPA@mail.gmail.com>
> Content-Type: text/plain; charset=3DUTF-8
>
> On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote:
> >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wro= te:
> >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <= ;ehabkost@redhat.com> wrote:<= br> > >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue = Swirl wrote:
> >> >> > [...]
> >> >> >> >> > diff --git a/tests/Makefile b= /tests/Makefile
> >> >> >> >> > index b605e14..89bd890 100644=
> >> >> >> >> > --- a/tests/Makefile
> >> >> >> >> > +++ b/tests/Makefile
> >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-= y +=3D tests/test-string-output-visitor$(EXESUF)
> >> >> >> >> > =A0check-unit-y +=3D tests/te= st-coroutine$(EXESUF)
> >> >> >> >> > =A0check-unit-y +=3D tests/te= st-visitor-serialization$(EXESUF)
> >> >> >> >> > =A0check-unit-y +=3D tests/te= st-iov$(EXESUF)
> >> >> >> >> > +check-unit-y +=3D tests/test= -x86-cpuid$(EXESUF)
> >> >> >> >>
> >> >> >> >> This probably tries to build the c= puid test also for non-x86 targets
> >> >> >> >> and break them all.
> >> >> >> >
> >> >> >> > I don't think there's any conc= ept of "targets" for the check-unit tests.
> >> >> >>
> >> >> >> How about:
> >> >> >> check-qtest-i386-y =3D tests/test-x86-cpuid= $(EXESUF)
> >> >> >
> >> >> > test-x86-cpuid is not a qtest test case.
> >> >>
> >> >> Why not? I don't think it is a unit test either,= judging from what the
> >> >> other unit tests do.
> >> >
> >> > It is absolutely a unit test. I don't know why you d= on't think so. It
> >> > simply checks the results of the APIC ID calculation fun= ctions.
> >>
> >> Yes, but the other 'unit tests' (the names used here = are very
> >> confusing, btw) check supporting functions like coroutines, n= ot
> >> hardware. Hardware tests (qtest) need to bind to an architect= ure, in
> >> this case x86.
> >
> > test-x86-cpuid doesn't check hardware, either. It just checks= the simple
> > functions that calculate APIC IDs (to make sure the math is corre= ct).
> > Those functions aren't even used by any hardware emulation co= de until
> > later in the patch series (when the CPU initialization code gets = changed
> > to use the new function).
>
> By that time the function is clearly x86 HW and the check is an x86 > hardware check. QEMU as whole consists of simple functions that
> calculate something.
>
>
> >
> > --
> > Eduardo
>
>
>
> ------------------------------
>
> Message: 2
> Date: Mon, 23 Jul 2012 16:46:37 -0300
> From: Luiz Capitulino <lc= apitulino@redhat.com>
> To: Markus Armbruster <armbru@= redhat.com>
> Cc: jan.kiszka@siemens.com, aliguori@us.ibm.com,
> =A0 =A0 =A0 =A0 qemu-devel@no= ngnu.org, =A0afaerber@suse.de > Subject: Re: [Qemu-devel] [PATCH 1/8] qemu-option:
> =A0 =A0 =A0 =A0 qemu_opt_set_bool(): fix code duplication
> Message-ID: <20120723164637.01863346@doriath.home>
> Content-Type: text/plain; charset=3DUS-ASCII
>
> On Mon, 23 Jul 2012 20:14:31 +0200
> Markus Armbruster <armbru@redh= at.com> wrote:
>
> > Luiz Capitulino <lca= pitulino@redhat.com> writes:
> >
> > > On Sat, 21 Jul 2012 10:09:09 +0200
> > > Markus Armbruster <a= rmbru@redhat.com> wrote:
> > >
> > >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> > >>
> > >> > Call qemu_opt_set() instead of duplicating opt_set(= ).
> > >> >
> > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > >> > ---
> > >> > =A0qemu-option.c | 28 +---------------------------<= br> > > >> > =A01 file changed, 1 insertion(+), 27 deletions(-)<= br> > > >> >
> > >> > diff --git a/qemu-option.c b/qemu-option.c
> > >> > index bb3886c..2cb2835 100644
> > >> > --- a/qemu-option.c
> > >> > +++ b/qemu-option.c
> > >> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts= *opts, const char
> > >> > *name, const char *value,
> > >> >
> > >> > =A0int qemu_opt_set_bool(QemuOpts *opts, const char= *name, bool val)
> > >> > =A0{
> > >> > - =A0 =A0QemuOpt *opt;
> > >> > - =A0 =A0const QemuOptDesc *desc =3D opts->list-= >desc;
> > >> > - =A0 =A0int i;
> > >> > -
> > >> > - =A0 =A0for (i =3D 0; desc[i].name !=3D NULL; i++)= {
> > >> > - =A0 =A0 =A0 =A0if (strcmp(desc[i].name, name) =3D= =3D 0) {
> > >> > - =A0 =A0 =A0 =A0 =A0 =A0break;
> > >> > - =A0 =A0 =A0 =A0}
> > >> > - =A0 =A0}
> > >> > - =A0 =A0if (desc[i].name =3D=3D NULL) {
> > >> > - =A0 =A0 =A0 =A0if (i =3D=3D 0) {
> > >> > - =A0 =A0 =A0 =A0 =A0 =A0/* empty list -> allow = any */;
> > >> > - =A0 =A0 =A0 =A0} else {
> > >> > - =A0 =A0 =A0 =A0 =A0 =A0qerror_report(QERR_INVALID= _PARAMETER, name);
> > >> > - =A0 =A0 =A0 =A0 =A0 =A0return -1;
> > >> > - =A0 =A0 =A0 =A0}
> > >> > - =A0 =A0}
> > >> > -
> > >> > - =A0 =A0opt =3D g_malloc0(sizeof(*opt));
> > >> > - =A0 =A0opt->name =3D g_strdup(name);
> > >> > - =A0 =A0opt->opts =3D opts;
> > >> > - =A0 =A0QTAILQ_INSERT_TAIL(&opts->head, opt= , next);
> > >> > - =A0 =A0if (desc[i].name !=3D NULL) {
> > >> > - =A0 =A0 =A0 =A0opt->desc =3D desc+i;
> > >> > - =A0 =A0}
> > >> > - =A0 =A0opt->value.boolean =3D !!val;
> > >> > - =A0 =A0return 0;
> > >> > + =A0 =A0return qemu_opt_set(opts, name, val ? &quo= t;on" : "off");
> > >> > =A0}
> > >> >
> > >> > =A0int qemu_opt_foreach(QemuOpts *opts, qemu_opt_lo= opfunc func, void *opaque,
> > >>
> > >> Does a bit more than just obvious code de-duplication. = =A0Two things in
> > >> particular:
> > >>
> > >> * Error reporting
> > >>
> > >> =A0 Old: qerror_report(); return -1
> > >>
> > >> =A0 New: opt_set() and qemu_opt_set() cooperate, like th= is:
> > >> =A0 =A0 =A0 =A0opt_set(): error_set(); return;
> > >> =A0 =A0 =A0 =A0qemu_opt_set():
> > >> =A0 =A0 =A0 =A0 =A0 =A0 if (error_is_set(&local_err)= ) {
> > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qerror_report_err(local_= err);
> > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error_free(local_err); > > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -1;
> > >> =A0 =A0 =A0 =A0 =A0 =A0 }
> > >>
> > >> =A0 I guess the net effect is the same. =A0Not sure it&#= 39;s worth mentioning in
> > >> =A0 the commit message.
> > >
> > > The end result of calling qemu_opt_set_bool() is the same. T= he difference
> > > between qerror_report() and qerror_report_err() is that the = former gets error
> > > information from the error call, while the latter gets error= information
> > > from the Error object. But they do the same thing.
> > >
> > >> * New sets opt->str to either "on" or "= ;off" depending on val, then lets
> > >> =A0 reconstructs the value with qemu_opt_parse(). =A0Whi= ch can't fail then.
> > >> =A0 I figure the latter part has no further impact. =A0B= ut what about
> > >> =A0 setting opt->str? =A0Is this a bug fix?
> > >
> > > I don't remember if opt->str is read after the QemuOp= t object is built. If
> > > it's, then yes, this is a bug fix. Otherwise it just mak= e the final
> > > QemuOpt object more 'conforming'.
> >
> > Uses of opt->str, and what happens when it isn't set:
> >
> > * qemu_opt_get(): returns NULL, which means "not set". = =A0Bug can bite
> > =A0 when value isn't the default value.
> >
> > * qemu_opt_parse(): passes NULL to parse_option_bool(), which tre= ats it
> > =A0 like "on". =A0Wrong if the value is actually false.= =A0Bug can bite when
> > =A0 qemu_opts_validate() runs after qemu_opt_set_bool().
> >
> > * qemu_opt_del(): passes NULL to g_free(), which is just fine. > >
> > * qemu_opt_foreach(): passes NULL to the callback, which is unlik= ely to
> > =A0 be prepared for it.
> >
> > * qemu_opts_print(): prints NULL, which crashes on some systems.<= br> > >
> > * qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which<= br> > > =A0 crashes.
>
> Thanks for the clarification.
>
> >
> > Right now, the only use of qemu_opt_set_bool() is the one in vl.c= .
> > Can't see how to break it, but I didn't look hard.
> >
> > I recommend to document the bug fix in the commit message.
>
> Ok, will do.
>
>
>
> ------------------------------
>
> Message: 3
> Date: Mon, 23 Jul 2012 17:00:02 -0300
> From: Luiz Capitulino <lc= apitulino@redhat.com>
> To: Markus Armbruster <armbru@= redhat.com>
> Cc: jan.kiszka@siemens.com, aliguori@us.ibm.com,
> =A0 =A0 =A0 =A0 qemu-devel@no= ngnu.org, =A0afaerber@suse.de > Subject: Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support > Message-ID: <20120723170002.510fd7f0@doriath.home>
> Content-Type: text/plain; charset=3DUS-ASCII
>
> On Mon, 23 Jul 2012 20:33:52 +0200
> Markus Armbruster <armbru@redh= at.com> wrote:
>
> > Luiz Capitulino <lca= pitulino@redhat.com> writes:
> >
> > > On Sat, 21 Jul 2012 10:11:39 +0200
> > > Markus Armbruster <a= rmbru@redhat.com> wrote:
> > >
> > >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> > >>
> > >> > Allow for specifying an alias for each option name,= see next commits
> > >> > for examples.
> > >> >
> > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > >> > ---
> > >> > =A0qemu-option.c | 5 +++--
> > >> > =A0qemu-option.h | 1 +
> > >> > =A02 files changed, 4 insertions(+), 2 deletions(-)=
> > >> >
> > >> > diff --git a/qemu-option.c b/qemu-option.c
> > >> > index 65ba1cf..b2f9e21 100644
> > >> > --- a/qemu-option.c
> > >> > +++ b/qemu-option.c
> > >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_= desc_by_name(const QemuOptDesc *desc,
> > >> > =A0 =A0 =A0int i;
> > >> >
> > >> > =A0 =A0 =A0for (i =3D 0; desc[i].name !=3D NULL; i+= +) {
> > >> > - =A0 =A0 =A0 =A0if (strcmp(desc[i].name, name) =3D= =3D 0) {
> > >> > + =A0 =A0 =A0 =A0if (strcmp(desc[i].name, name) =3D= =3D 0 ||
> > >> > + =A0 =A0 =A0 =A0 =A0 =A0(desc[i].alias && = strcmp(desc[i].alias, name) =3D=3D 0)) {
> > >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0return &desc[i];
> > >> > =A0 =A0 =A0 =A0 =A0}
> > >> > =A0 =A0 =A0}
> > >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *o= pts, const char *name, const char *value,
> >
> > =A0 =A0 =A0 static void opt_set(QemuOpts *opts, const char *name,= const
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bool prepend,= Error **errp)
> > =A0 =A0 =A0 {
> > =A0 =A0 =A0 =A0 =A0 QemuOpt *opt;
> > =A0 =A0 =A0 =A0 =A0 const QemuOptDesc *desc;
> > =A0 =A0 =A0 =A0 =A0 Error *local_err =3D NULL;
> >
> > =A0 =A0 =A0 =A0 =A0 desc =3D find_desc_by_name(opts->list->= desc, name);
> > =A0 =A0 =A0 =A0 =A0 if (!desc && !opts_accepts_any(opts))= {
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 error_set(errp, QERR_INVALID_PARAMETE= R, name);
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> > >> > =A0 =A0 =A0}
> > >> >
> > >> > =A0 =A0 =A0opt =3D g_malloc0(sizeof(*opt));
> > >> > - =A0 =A0opt->name =3D g_strdup(name);
> > >> > + =A0 =A0opt->name =3D g_strdup(desc ? desc->= name : name);
> > >> > =A0 =A0 =A0opt->opts =3D opts;
> > >> > =A0 =A0 =A0if (prepend) {
> > >> > =A0 =A0 =A0 =A0 =A0QTAILQ_INSERT_HEAD(&opts->= ;head, opt, next);
> > >>
> > >> Are you sure this hunk belongs to this patch? =A0If yes,= please explain
> > >> why :)
> > >
> > > Yes, I think it's fine because the change that makes it = necessary to choose
> > > between desc->name and name is introduced by the hunk abo= ve.
> > >
> >
> > I think I now get why you have this hunk:
> >
> > We reach it only if the parameter with this name exists (desc non= -null),
> > or opts accepts anthing (opts_accepts_any(opts).
> >
> > If it exists, name equals desc->name before your patch. =A0But= afterwards,
> > it could be either desc->name or desc->alias. =A0You normal= ize to
> > desc->name.
> >
> > Else, all we can do is stick to name.
> >
> > Correct?
>
> Yes.
>
> > If yes, then "normal" options and "accept any"= ; options behave
> > differently: the former set opts->name to the canonical name, = even when
> > the user uses an alias. =A0The latter set opts->name to whatev= er the user
> > uses. =A0I got a bad feeling about that.
>
> Why? Or, more importantly, how do you think we should do it?
>
> For 'normal' options, the alias is just a different name to re= fer to the
> option name. At least in theory, it shouldn't matter that the opti= on was
> set through the alias.
>
> For 'accept any' options, it's up to the code handling the= options know
> what to do with it. It can also support aliases if it wants to, or jus= t
> refuse it.
>
>
>
> ------------------------------
>
> Message: 4
> Date: Mon, 23 Jul 2012 22:07:27 +0200
> From: Laszlo Ersek <lersek@red= hat.com>
> To: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Paolo Bonzini <pbonzini@= redhat.com>, Zhi Yong Wu
> =A0 =A0 =A0 =A0 <wuzhy@= linux.vnet.ibm.com>, =A0 =A0 qemu-devel@nongnu.org, Zhi Yong Wu
> =A0 =A0 =A0 =A0 <wuzhy@cn.ibm.c= om>
> Subject: Re: [Qemu-devel] [PATCH 06/16] net: Convert qdev_prop_vlan to=
> =A0 =A0 =A0 =A0 peer =A0 =A0with hub
> Message-ID: <500DAEF= F.7060708@redhat.com>
> Content-Type: text/plain; charset=3DISO-8859-1
>
> On 07/20/12 14:01, Stefan Hajnoczi wrote:
>
> > @@ -638,11 +642,17 @@ static void get_vlan(Object *obj, Visitor *= v, void *opaque,
> > =A0{
> > =A0 =A0 =A0DeviceState *dev =3D DEVICE(obj);
> > =A0 =A0 =A0Property *prop =3D opaque;
> > - =A0 =A0VLANState **ptr =3D qdev_get_prop_ptr(dev, prop);
> > - =A0 =A0int64_t id;
> > + =A0 =A0VLANClientState **ptr =3D qdev_get_prop_ptr(dev, prop);<= br> > > + =A0 =A0int64_t id =3D -1;
> >
> > - =A0 =A0id =3D *ptr ? (*ptr)->id : -1;
> > - =A0 =A0visit_type_int64(v, &id, name, errp);
> > + =A0 =A0if (*ptr) {
> > + =A0 =A0 =A0 =A0unsigned int hub_id;
> > + =A0 =A0 =A0 =A0if (!net_hub_id_for_client(*ptr, &hub_id)) {=
> > + =A0 =A0 =A0 =A0 =A0 =A0id =3D (int64_t)hub_id;
> > + =A0 =A0 =A0 =A0}
> > + =A0 =A0}
> > +
> > + =A0 =A0visit_type_int(v, &id, name, errp);
> > =A0}
>
> Should we use uint32 here? (No particular reason, just for "clean= liness"
> or whatever.)
>
> Thanks,
> Laszlo
>
>
>
> ------------------------------
>
> Message: 5
> Date: Mon, 23 Jul 2012 17:14:46 -0300
> From: Eduardo Habkost <ehabk= ost@redhat.com>
> To: Blue Swirl <blauwirbel@= gmail.com>
> Cc: Anthony Liguori <antho= ny@codemonkey.ws>, =A0 =A0Igor Mammedov
> =A0 =A0 =A0 =A0 <imammedo@re= dhat.com>, seabios@seabios.or= g, =A0 =A0 qemu-devel@nongnu.o= rg,
> =A0 =A0 =A0 =A0 Gleb Natapov <gl= eb@redhat.com>
> Subject: Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & AP= IC
> =A0 =A0 =A0 =A0 ID utility functions
> Message-ID: <20120723201446.GX13029@otherpad.lan.raisama.net>
> Content-Type: text/plain; charset=3Dus-ascii
>
> On Mon, Jul 23, 2012 at 07:44:44PM +0000, Blue Swirl wrote:
> > On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote:<= br> > > >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swir= l wrote:
> > >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkos= t <ehabkost@redhat.com> wr= ote:
> > >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, = Blue Swirl wrote:
> > >> >> > [...]
> > >> >> >> >> > diff --git a/tests/Makef= ile b/tests/Makefile
> > >> >> >> >> > index b605e14..89bd890 1= 00644
> > >> >> >> >> > --- a/tests/Makefile
> > >> >> >> >> > +++ b/tests/Makefile
> > >> >> >> >> > @@ -15,6 +15,7 @@ check-= unit-y +=3D tests/test-string-output-visitor$(EXESUF)
> > >> >> >> >> > =A0check-unit-y +=3D tes= ts/test-coroutine$(EXESUF)
> > >> >> >> >> > =A0check-unit-y +=3D tes= ts/test-visitor-serialization$(EXESUF)
> > >> >> >> >> > =A0check-unit-y +=3D tes= ts/test-iov$(EXESUF)
> > >> >> >> >> > +check-unit-y +=3D tests= /test-x86-cpuid$(EXESUF)
> > >> >> >> >>
> > >> >> >> >> This probably tries to build = the cpuid test also for non-x86 targets
> > >> >> >> >> and break them all.
> > >> >> >> >
> > >> >> >> > I don't think there's any= concept of "targets" for the check-unit tests.
> > >> >> >>
> > >> >> >> How about:
> > >> >> >> check-qtest-i386-y =3D tests/test-x86-= cpuid$(EXESUF)
> > >> >> >
> > >> >> > test-x86-cpuid is not a qtest test case. > > >> >>
> > >> >> Why not? I don't think it is a unit test ei= ther, judging from what the
> > >> >> other unit tests do.
> > >> >
> > >> > It is absolutely a unit test. I don't know why = you don't think so. It
> > >> > simply checks the results of the APIC ID calculatio= n functions.
> > >>
> > >> Yes, but the other 'unit tests' (the names used = here are very
> > >> confusing, btw) check supporting functions like coroutin= es, not
> > >> hardware. Hardware tests (qtest) need to bind to an arch= itecture, in
> > >> this case x86.
> > >
> > > test-x86-cpuid doesn't check hardware, either. It just c= hecks the simple
> > > functions that calculate APIC IDs (to make sure the math is = correct).
> > > Those functions aren't even used by any hardware emulati= on code until
> > > later in the patch series (when the CPU initialization code = gets changed
> > > to use the new function).
> >
> > By that time the function is clearly x86 HW and the check is an x= 86
> > hardware check. QEMU as whole consists of simple functions that > > calculate something.
>
> It's useful onily for a specific architecture, yes, but it surely<= br> > doesn't make libqtest necessary.
>
> That's the difference between the unit tests and qtest test cases:= unit
> tests simply test small parts of the code (that may or may not be
> hardware-specific), and qtest tests hardware emulation after starting = up
> a whole qemu process. It's a different kind of testing, with diffe= rent
> sets of goals.
>
> I suppose you are not arguing that no function inside target-* would b= e
> ever allowed to have a unit test written for it. That would be a very<= br> > silly constraint for people writing tests.
>
> --
> Eduardo
>
>
>
> ------------------------------
>
> Message: 6
> Date: Mon, 23 Jul 2012 17:41:08 -0300
> From: Luiz Capitulino <lc= apitulino@redhat.com>
> To: Pavel Hrdina <phrdina@red= hat.com>
> Cc: qemu-devel@nongnu.org=
> Subject: Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm
> Message-ID: <20120723174108.744f835e@doriath.home>
> Content-Type: text/plain; charset=3DUS-ASCII
>
> On Thu, 12 Jul 2012 18:55:15 +0200
> Pavel Hrdina <phrdina@redhat.= com> wrote:
>
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> > =A0hmp-commands.hx =A0| =A0 =A02 +-
> > =A0hmp.c =A0 =A0 =A0 =A0 =A0 =A0| =A0 10 ++++++++++
> > =A0hmp.h =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A01 +
> > =A0qapi-schema.json | =A0 22 ++++++++++++++++++++++
> > =A0qerror.c =A0 =A0 =A0 =A0 | =A0 24 ++++++++++++++++++++++++
> > =A0qerror.h =A0 =A0 =A0 =A0 | =A0 18 ++++++++++++++++++
> > =A0qmp-commands.hx =A0| =A0 26 ++++++++++++++++++++++++++
> > =A0savevm.c =A0 =A0 =A0 =A0 | =A0 29 ++++++++++++++--------------= -
> > =A0sysemu.h =A0 =A0 =A0 =A0 | =A0 =A01 -
> > =A09 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
> > =A0 =A0 =A0 =A0 =A0.args_type =A0=3D "name:s?",
> > =A0 =A0 =A0 =A0 =A0.params =A0 =A0 =3D "[tag|id]",
> > =A0 =A0 =A0 =A0 =A0.help =A0 =A0 =A0 =3D "save a VM snapshot= . If no tag or id are provided, a new snapshot is created",
> > - =A0 =A0 =A0 =A0.mhandler.cmd =3D do_savevm,
> > + =A0 =A0 =A0 =A0.mhandler.cmd =3D hmp_savevm,
> > =A0 =A0 =A0},
> >
> > =A0STEXI
> > 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 QD= ict *qdict)
> > =A0 =A0 =A0qmp_netdev_del(id, &err);
> > =A0 =A0 =A0hmp_handle_error(mon, &err);
> > =A0}
> > +
> > +void hmp_savevm(Monitor *mon, const QDict *qdict)
> > +{
> > + =A0 =A0const char *name =3D qdict_get_try_str(qdict, "name= ");
> > + =A0 =A0Error *err =3D NULL;
> > +
> > + =A0 =A0qmp_savevm(!!name, name, &err);
> > +
> > + =A0 =A0hmp_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);
> > =A0void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);<= br> > > =A0void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> > =A0void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> > +void hmp_savevm(Monitor *mon, const QDict *qdict);
> >
> > =A0#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 @@
> > =A0# Since: 0.14.0
> > =A0##
> > =A0{ 'command': 'netdev_del', 'data': {&#= 39;id': 'str'} }
> > +
> > +##
> > +# @savevm:
>
> Let's call it save-vm.
>
> > +#
> > +# Create a snapshot of the whole virtual machine. If 'tag= 9; 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 't= ag' and 'id'.
> I remember it was difficult to do, so we could choose not to do it, bu= t let's
> re-evaluate this again.
>
> > +#
> > +# Returns: Nothing on success
> > +# =A0 =A0 =A0 =A0 =A0If there is a writable device not supportin= g snapshots,
> > +# =A0 =A0 =A0 =A0 =A0 =A0SnapshotNotSupported
> > +# =A0 =A0 =A0 =A0 =A0If no block device can accept snapshots, Sn= apshotNotAccepted
> > +# =A0 =A0 =A0 =A0 =A0If an error occures while creating a snapsh= ot, SnapshotCreateFailed
> > +# =A0 =A0 =A0 =A0 =A0If open a block device for vm state fail, S= napshotOpenFailed
> > +# =A0 =A0 =A0 =A0 =A0If an error uccures while writing vm state,= SnapshotWriteFailed
> > +# =A0 =A0 =A0 =A0 =A0If 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': {'*na= me': '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[= ] =3D {
> > =A0 =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Could not start VNC= server on %(target)",
> > =A0 =A0 =A0},
> > =A0 =A0 =A0{
> > + =A0 =A0 =A0 =A0.error_fmt =3D QERR_SNAPSHOT_CREATE_FAILED,
> > + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Error '%(errno)&#= 39; while creating snapshot on '%(device)'",
> > + =A0 =A0},
> > + =A0 =A0{
> > + =A0 =A0 =A0 =A0.error_fmt =3D QERR_SNAPSHOT_DELETE_FAILED,
> > + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Error '%(errno)&#= 39; while deleting snapshot on '%(device)'",
> > + =A0 =A0},
> > + =A0 =A0{
> > + =A0 =A0 =A0 =A0.error_fmt =3D QERR_SNAPSHOT_NOT_ACCEPTED,
> > + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "No block device can a= ccept snapshots",
> > + =A0 =A0},
> > + =A0 =A0{
> > + =A0 =A0 =A0 =A0.error_fmt =3D QERR_SNAPSHOT_NOT_SUPPORTED,
> > + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Device '%(device)= ' is writable but does not support snapshots",
> > + =A0 =A0},
> > + =A0 =A0{
> > + =A0 =A0 =A0 =A0.error_fmt =3D QERR_SNAPSHOT_OPEN_FAILED,
> > + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Error while opening s= napshot on '%(device)'",
> > + =A0 =A0},
> > + =A0 =A0{
> > + =A0 =A0 =A0 =A0.error_fmt =3D QERR_SNAPSHOT_WRITE_FAILED,
> > + =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Error '%(errno)&#= 39; while writing snapshot on '%(device)'",
> > + =A0 =A0},
> > + =A0 =A0{
> > =A0 =A0 =A0 =A0 =A0.error_fmt =3D QERR_SOCKET_CONNECT_IN_PROGRESS= ,
> > =A0 =A0 =A0 =A0 =A0.desc =A0 =A0 =A0=3D "Connection can not = be completed immediately",
> > =A0 =A0 =A0},
> > 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= );
> > =A0#define QERR_VNC_SERVER_FAILED \
> > =A0 =A0 =A0"{ 'class': 'VNCServerFailed', &#= 39;data': { 'target': %s } }"
> >
> > +#define QERR_SNAPSHOT_CREATE_FAILED \
> > + =A0 =A0"{ 'class': 'SnapshotCreateFailed',= 'data': { 'device': %s, 'errno': %d } }"
> > +
> > +#define QERR_SNAPSHOT_DELETE_FAILED \
> > + =A0 =A0"{ 'class': 'SnapshotDeleteFailed',= 'data': { 'device': %s, 'errno': %d } }"
> > +
> > +#define QERR_SNAPSHOT_NOT_ACCEPTED \
> > + =A0 =A0"{ 'class': 'SnapshotNotAccepted', = 'data': {} }"
> > +
> > +#define QERR_SNAPSHOT_NOT_SUPPORTED \
> > + =A0 =A0"{ 'class': 'SnapshotNotSupported',= 'data': { 'device': %s } }"
> > +
> > +#define QERR_SNAPSHOT_OPEN_FAILED \
> > + =A0 =A0"{ 'class': 'SnapshotOpenFailed', &= #39;data': { 'device': %s } }"
> > +
> > +#define QERR_SNAPSHOT_WRITE_FAILED \
> > + =A0 =A0"{ 'class': 'SnapshotWriteFailed', = 'data': { 'device': %s, 'errno': %d } }"
> > +
> > =A0#define QERR_SOCKET_CONNECT_IN_PROGRESS \
> > =A0 =A0 =A0"{ 'class': 'SockConnectInprogress= 9;, '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:
> >
> > =A0EQMP
> > =A0 =A0 =A0{
> > + =A0 =A0 =A0 =A0.name =A0 =A0 =A0 =3D "savevm",
> > + =A0 =A0 =A0 =A0.args_type =A0=3D "name:s?",
> > + =A0 =A0 =A0 =A0.params =A0 =A0 =3D "name",
> > + =A0 =A0 =A0 =A0.help =A0 =A0 =A0 =3D "save a VM snapshot. = If no tag or id are provided, a new snapshot is created",
> > + =A0 =A0 =A0 =A0.mhandler.cmd_new =3D qmp_marshal_input_savevm > > + =A0 =A0},
> > +
> > +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 s= napshot
> > +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
> > + =A0 =A0{
> > =A0 =A0 =A0 =A0 =A0.name =A0 =A0 =A0 =3D "qmp_capabilities&q= uot;,
> > =A0 =A0 =A0 =A0 =A0.args_type =A0=3D "",
> > =A0 =A0 =A0 =A0 =A0.params =A0 =A0 =3D "",
> > 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(BlockDriverSt= ate *bs, QEMUSnapshotInfo *sn_info,
> > =A0/*
> > =A0 * Deletes snapshots of a given name in all opened images.
> > =A0 */
> > -static int del_existing_snapshots(Monitor *mon, const char *name= )
> > +static int del_existing_snapshots(Error **errp, const char *name= )
> > =A0{
> > =A0 =A0 =A0BlockDriverState *bs;
> > =A0 =A0 =A0QEMUSnapshotInfo sn1, *snapshot =3D &sn1;
> > @@ -2046,9 +2046,8 @@ static int del_existing_snapshots(Monitor *= mon, const char *name)
> > =A0 =A0 =A0 =A0 =A0{
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D bdrv_snapshot_delete(bs, name)= ;
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret < 0) {
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0monitor_printf(mon,
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &qu= ot;Error while deleting snapshot on '%s'\n",
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdr= v_get_device_name(bs));
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error_set(errp, QERR_SNAPSHOT_DE= LETE_FAILED,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bdrv_get_dev= ice_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: >
> =A01. The QMP command shouldn't delete existing snapshots by defau= lt. Either,
> =A0 =A0 we add a 'force' argument to it or don't delete sn= apshots in save-vm
> =A0 =A0 at all (in which case a mngt app would have to delete the snap= shots with the
> =A0 =A0 same name manually, I prefer this approach btw)
>
> =A02. This has to be done in a separate series
>
> =A03. It's a good idea to wait for my series improving error_set()= to be merged
> =A0 =A0 before doing this
>
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -1;
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0 =A0 =A0}
> > @@ -2057,7 +2056,7 @@ static int del_existing_snapshots(Monitor *= mon, const char *name)
> > =A0 =A0 =A0return 0;
> > =A0}
> >
> > -void do_savevm(Monitor *mon, const QDict *qdict)
> > +void qmp_savevm(bool has_name, const char *name, Error **errp) > > =A0{
> > =A0 =A0 =A0BlockDriverState *bs, *bs1;
> > =A0 =A0 =A0QEMUSnapshotInfo sn1, *sn =3D &sn1, old_sn1, *old_= sn =3D &old_sn1;
> > @@ -2072,7 +2071,6 @@ void do_savevm(Monitor *mon, const QDict *q= dict)
> > =A0 =A0 =A0struct timeval tv;
> > =A0 =A0 =A0struct tm tm;
> > =A0#endif
> > - =A0 =A0const char *name =3D qdict_get_try_str(qdict, "name= ");
> >
> > =A0 =A0 =A0/* Verify if there is a device that doesn't suppor= t snapshots and is writable */
> > =A0 =A0 =A0bs =3D NULL;
> > @@ -2083,15 +2081,15 @@ void do_savevm(Monitor *mon, const QDict = *qdict)
> > =A0 =A0 =A0 =A0 =A0}
> >
> > =A0 =A0 =A0 =A0 =A0if (!bdrv_can_snapshot(bs)) {
> > - =A0 =A0 =A0 =A0 =A0 =A0monitor_printf(mon, "Device '%s= ' is writable but does not support snapshots.\n",
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdr= v_get_device_name(bs));
> > + =A0 =A0 =A0 =A0 =A0 =A0error_set(errp, QERR_SNAPSHOT_NOT_SUPPOR= TED,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bdrv_get_device_name= (bs));
>
> Please, use QERR_NOT_SUPPORTED instead. I know it doesn't allow an= y arguments
> today, but I'm working on a series which will allow you to pass an= y arguments
> you wish.
>
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
> > =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0}
> >
> > =A0 =A0 =A0bs =3D bdrv_snapshots();
> > =A0 =A0 =A0if (!bs) {
> > - =A0 =A0 =A0 =A0monitor_printf(mon, "No block device can ac= cept snapshots\n");
> > + =A0 =A0 =A0 =A0error_set(errp, QERR_SNAPSHOT_NOT_ACCEPTED);
>
> I think we should use QERR_NOT_SUPPORTED here too.
>
> > =A0 =A0 =A0 =A0 =A0return;
> > =A0 =A0 =A0}
> >
> > @@ -2112,7 +2110,7 @@ void do_savevm(Monitor *mon, const QDict *q= dict)
> > =A0#endif
> > =A0 =A0 =A0sn->vm_clock_nsec =3D qemu_get_clock_ns(vm_clock);<= br> > >
> > - =A0 =A0if (name) {
> > + =A0 =A0if (has_name) {
> > =A0 =A0 =A0 =A0 =A0ret =3D bdrv_snapshot_find(bs, old_sn, name);<= br> > > =A0 =A0 =A0 =A0 =A0if (ret >=3D 0) {
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0pstrcpy(sn->name, sizeof(sn->nam= e), old_sn->name);
> > @@ -2133,21 +2131,22 @@ void do_savevm(Monitor *mon, const QDict = *qdict)
> > =A0 =A0 =A0}
> >
> > =A0 =A0 =A0/* Delete old snapshots of the same name */
> > - =A0 =A0if (name && del_existing_snapshots(mon, name) &l= t; 0) {
> > + =A0 =A0if (has_name && del_existing_snapshots(errp, nam= e) < 0) {
> > =A0 =A0 =A0 =A0 =A0goto the_end;
> > =A0 =A0 =A0}
>
> The QMP command should not delete existing snapshots, as said above. >
> >
> > =A0 =A0 =A0/* save the VM state */
> > =A0 =A0 =A0f =3D qemu_fopen_bdrv(bs, 1);
> > =A0 =A0 =A0if (!f) {
> > - =A0 =A0 =A0 =A0monitor_printf(mon, "Could not open VM stat= e file\n");
> > + =A0 =A0 =A0 =A0error_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.
>
> > =A0 =A0 =A0 =A0 =A0goto the_end;
> > =A0 =A0 =A0}
> > =A0 =A0 =A0ret =3D qemu_savevm_state(f);
> > =A0 =A0 =A0vm_state_size =3D qemu_ftell(f);
> > =A0 =A0 =A0qemu_fclose(f);
> > =A0 =A0 =A0if (ret < 0) {
> > - =A0 =A0 =A0 =A0monitor_printf(mon, "Error %d while writing= VM\n", ret);
> > + =A0 =A0 =A0 =A0error_set(errp, QERR_SNAPSHOT_WRITE_FAILED,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bdrv_get_device_name(bs), re= t);
>
> The Right Thing to do here it to convert qemu_savevm_sstate() to take<= br> > an Error argument. The same considerations I wrote above about
> del_existing_snapshots() apply here, except that you can report IOErro= r
> for most qemu_savevm_state() errors.
>
> > =A0 =A0 =A0 =A0 =A0goto the_end;
> > =A0 =A0 =A0}
> >
> > @@ -2160,8 +2159,8 @@ void do_savevm(Monitor *mon, const QDict *q= dict)
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0sn->vm_state_size =3D (bs =3D=3D bs= 1 ? vm_state_size : 0);
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D bdrv_snapshot_create(bs1, sn);=
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret < 0) {
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0monitor_printf(mon, "Error = while creating snapshot on '%s'\n",
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdr= v_get_device_name(bs1));
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error_set(errp, QERR_SNAPSHOT_CR= EATE_FAILED,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bdrv_get_dev= ice_name(bs1), ret);
>
> Here too, bdrv_snapshot_create() should be changed to take an Error > argument.
>
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0}
> > 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= );
> >
> > =A0void qemu_add_machine_init_done_notifier(Notifier *notify); > >
> > -void do_savevm(Monitor *mon, const QDict *qdict);
> > =A0int load_vmstate(const char *name);
> > =A0void do_delvm(Monitor *mon, const QDict *qdict);
> > =A0void do_info_snapshots(Monitor *mon);
>
>
>
>
> ------------------------------
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> https= ://lists.nongnu.org/mailman/listinfo/qemu-devel
>
>
> End of Qemu-devel Digest, Vol 112, Issue 561
> ********************************************

--bcaec554d63ca6929d04c5861c82--