Re: [Qemu-devel] Qemu-devel Digest, Vol 112, Issue 561

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Qemu-devel Digest, Vol 112, Issue 561

From:	Paulo Arcinas
Subject:	Re: [Qemu-devel] Qemu-devel Digest, Vol 112, Issue 561
Date:	Tue, 24 Jul 2012 05:41:42 +0800

>
> Message: 1
> Date: Mon, 23 Jul 2012 19:44:44 +0000
> From: Blue Swirl <address@hidden>
> To: Eduardo Habkost <address@hidden>
> Cc: Anthony Liguori <address@hidden>, Igor Mammedov
> <address@hidden>, address@hidden, address@hidden,
> Gleb Natapov <address@hidden>
> Subject: Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC
> ID utility functions
> Message-ID:
> <address@hidden>
> Content-Type: text/plain; charset=UTF-8
>
> On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
> To: Markus Armbruster <address@hidden>
> Cc: address@hidden, address@hidden,
> address@hidden, address@hidden
> Subject: Re: [Qemu-devel] [PATCH 1/8] qemu-option:
> qemu_opt_set_bool(): fix code duplication
> Message-ID: <address@hidden>
> Content-Type: text/plain; charset=US-ASCII
>
> On Mon, 23 Jul 2012 20:14:31 +0200
> Markus Armbruster <address@hidden> wrote:
>
> > Luiz Capitulino <address@hidden> writes:
> >
> > > On Sat, 21 Jul 2012 10:09:09 +0200
> > > Markus Armbruster <address@hidden> wrote:
> > >
> > >> Luiz Capitulino <address@hidden> writes:
> > >>
> > >> > Call qemu_opt_set() instead of duplicating opt_set().
> > >> >
> > >> > Signed-off-by: Luiz Capitulino <address@hidden>
> > >> > ---
> > >> > 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 <address@hidden>
> To: Markus Armbruster <address@hidden>
> Cc: address@hidden, address@hidden,
> address@hidden, address@hidden
> Subject: Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support
> Message-ID: <address@hidden>
> Content-Type: text/plain; charset=US-ASCII
>
> On Mon, 23 Jul 2012 20:33:52 +0200
> Markus Armbruster <address@hidden> wrote:
>
> > Luiz Capitulino <address@hidden> writes:
> >
> > > On Sat, 21 Jul 2012 10:11:39 +0200
> > > Markus Armbruster <address@hidden> wrote:
> > >
> > >> Luiz Capitulino <address@hidden> writes:
> > >>
> > >> > Allow for specifying an alias for each option name, see next commits
> > >> > for examples.
> > >> >
> > >> > Signed-off-by: Luiz Capitulino <address@hidden>
> > >> > ---
> > >> > 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 <address@hidden>
> To: Stefan Hajnoczi <address@hidden>
> Cc: Paolo Bonzini <address@hidden>, Zhi Yong Wu
> <address@hidden>, address@hidden, Zhi Yong Wu
> <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH 06/16] net: Convert qdev_prop_vlan to
> peer with hub
> Message-ID: <address@hidden>
> 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 <address@hidden>
> To: Blue Swirl <address@hidden>
> Cc: Anthony Liguori <address@hidden>, Igor Mammedov
> <address@hidden>, address@hidden, address@hidden,
> Gleb Natapov <address@hidden>
> Subject: Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC
> ID utility functions
> Message-ID: <address@hidden>
> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
> To: Pavel Hrdina <address@hidden>
> Cc: address@hidden
> Subject: Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm
> Message-ID: <address@hidden>
> Content-Type: text/plain; charset=US-ASCII
>
> On Thu, 12 Jul 2012 18:55:15 +0200
> Pavel Hrdina <address@hidden> wrote:
>
> > Signed-off-by: Pavel Hrdina <address@hidden>
> > ---
> > 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
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/qemu-devel
>
>
> End of Qemu-devel Digest, Vol 112, Issue 561
> ********************************************

[Prev in Thread]

Current Thread

[Next in Thread]

Re: [Qemu-devel] Qemu-devel Digest, Vol 112, Issue 561, Paulo Arcinas <=