Re: [PATCH v3 3/4] migration: Deprecate block migration
Am 12.10.2023 um 12:01 hat Markus Armbruster geschrieben: > Juan Quintela writes: > > > It is obsolete. It is better to use driver-mirror with NBD instead. > > drive-mirror > > Several more below. Actually, blockdev-mirror, please. We don't want to move people from the oldest way to the next oldest one. Pointing them to the modern way makes more sense. > > > > CC: Kevin Wolf > > CC: Eric Blake > > CC: Stefan Hajnoczi > > CC: Hanna Czenczek > > > > Signed-off-by: Juan Quintela Kevin
[PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments
While introducing a non-QemuOpts code path for device creation for JSON -device, we noticed that QMP device_add doesn't check its input correctly (accepting arguments that should have been rejected), and that users may be relying on this behaviour (libvirt did until it was fixed recently). Let's use a deprecation period before we fix this bug in QEMU to avoid nasty surprises for users. Signed-off-by: Kevin Wolf --- docs/about/deprecated.rst | 14 ++ 1 file changed, 14 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 600031210d..c03fcf951f 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -250,6 +250,20 @@ options are removed in favor of using explicit ``blockdev-create`` and ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for details. +Incorrectly typed ``device_add`` arguments (since 6.2) +'' + +Due to shortcomings in the internal implementation of ``device_add``, QEMU +incorrectly accepts certain invalid arguments: Any object or list arguments are +silently ignored. Other argument types are not checked, but an implicit +conversion happens, so that e.g. string values can be assigned to integer +device properties or vice versa. + +This is a bug in QEMU that will be fixed in the future so that previously +accepted incorrect commands will return an error. Users should make sure that +all arguments passed to ``device_add`` are consistent with the documented +property types. + System accelerators --- -- 2.31.1
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
Am 29.10.2021 um 15:07 hat Eric Blake geschrieben:
> On Mon, Oct 25, 2021 at 07:25:25AM +0200, Markus Armbruster wrote:
> > Add special feature 'unstable' everywhere the name starts with 'x-',
> > except for InputBarrierProperties member x-origin and
> > MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
> > because these two are actually stable.
> >
> > Signed-off-by: Markus Armbruster
> > ---
> > @@ -2495,27 +2508,57 @@
> > #
> > # Properties for throttle-group objects.
> > #
> > -# The options starting with x- are aliases for the same key without x- in
> > -# the @limits object. As indicated by the x- prefix, this is not a stable
> > -# interface and may be removed or changed incompatibly in the future. Use
> > -# @limits for a supported stable interface.
> > -#
> > # @limits: limits to apply for this throttle group
> > #
> > +# Features:
> > +# @unstable: All members starting with x- are aliases for the same key
> > +#without x- in the @limits object. This is not a stable
> > +#interface and may be removed or changed incompatibly in
> > +#the future. Use @limits for a supported stable
> > +#interface.
> > +#
> > # Since: 2.11
> > ##
> > { 'struct': 'ThrottleGroupProperties',
> >'data': { '*limits': 'ThrottleLimits',
> > -'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
>
> > +'*x-iops-total': { 'type': 'int',
> > + 'features': [ 'unstable' ] },
>
> This struct has been around since 381bd74 (v6.0); but was not listed
> as deprecated at the time. Do we still need it in 6.2, or have we
> gone enough release cycles with the saner naming without x- that we
> could drop this? But that is a question independent of this patch.
There is no reason any more to use the x- options, and I think libvirt
never used them anyway.
I actually have a commit in my QAPI object branch that removes these
properties, but I think it still broke some tests.
Anyway, something for a separate patch.
Kevin
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Am 26.10.2021 um 11:37 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > >> By convention, names starting with "x-" are experimental. The parts > >> of external interfaces so named may be withdrawn or changed > >> incompatibly in future releases. > >> > >> Drawback: promoting something from experimental to stable involves a > >> name change. Client code needs to be updated. > >> > >> Moreover, the convention is not universally observed: > >> > >> * QOM type "input-barrier" has properties "x-origin", "y-origin". > >> Looks accidental, but it's ABI since 4.2. > >> > >> * QOM types "memory-backend-file", "memory-backend-memfd", > >> "memory-backend-ram", and "memory-backend-epc" have a property > >> "x-use-canonical-path-for-ramblock-id" that is documented to be > >> stable despite its name. > >> > >> We could document these exceptions, but documentation helps only > >> humans. We want to recognize "unstable" in code, like "deprecated". > >> > >> Replace the convention by a new special feature flag "unstable". It > >> will be recognized by the QAPI generator, like the existing feature > >> flag "deprecated", and unlike regular feature flags. > >> > >> This commit updates documentation and prepares tests. The next commit > >> updates the QAPI schema. The remaining patches update the QAPI > >> generator and wire up -compat policy checking. > >> > >> Signed-off-by: Markus Armbruster > > > > Obviously, replacing the old convention gets rid of the old drawbacks, > > but adds a new one: While using x- makes it very obvious for a human > > user that this is an unstable feature, a feature flag in the schema will > > almost certainly go unnoticed in manual use. > > I thought about this, but neglected to put it in writing. My bad. > > Manual use of unstable interfaces is mostly fine. Human users can adapt > to changing interfaces. HMP works that way. > > Management applications are better off with a feature flag than with a > naming convention we sometimes ignore. > > The most potential for trouble is in between: programs that aren't > full-fledged management applications. > > If we want to keep "unstable" obvious to the humans who write such > programs, we can continue to require "x-", in addition to the feature > flag. We pay for it with renames, and the risk of forgetting to rename > in time (which is what got us the awkward stable > "x-use-canonical-path-for-ramblock-id"). Tradeoff. I chose not to, but > if y'all think we should... Just to clarify, I'm not implying that we should keep it. I'm merely pointing out that there is a tradeoff that requires us to make a choice. The decision for one of the options should be explicit rather than just happening as a side effect. Documenting that it was a conscious decision is probably best done by adding the reasoning for it to the commit message. > What we can't do, at least not easily, is to use *only* the "x-" > convention: it is not reliable. We'd have to add a way to say 'this is > stable even though the name starts with "x-"'. No question. Kevin
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > By convention, names starting with "x-" are experimental. The parts > of external interfaces so named may be withdrawn or changed > incompatibly in future releases. > > Drawback: promoting something from experimental to stable involves a > name change. Client code needs to be updated. > > Moreover, the convention is not universally observed: > > * QOM type "input-barrier" has properties "x-origin", "y-origin". > Looks accidental, but it's ABI since 4.2. > > * QOM types "memory-backend-file", "memory-backend-memfd", > "memory-backend-ram", and "memory-backend-epc" have a property > "x-use-canonical-path-for-ramblock-id" that is documented to be > stable despite its name. > > We could document these exceptions, but documentation helps only > humans. We want to recognize "unstable" in code, like "deprecated". > > Replace the convention by a new special feature flag "unstable". It > will be recognized by the QAPI generator, like the existing feature > flag "deprecated", and unlike regular feature flags. > > This commit updates documentation and prepares tests. The next commit > updates the QAPI schema. The remaining patches update the QAPI > generator and wire up -compat policy checking. > > Signed-off-by: Markus Armbruster Obviously, replacing the old convention gets rid of the old drawbacks, but adds a new one: While using x- makes it very obvious for a human user that this is an unstable feature, a feature flag in the schema will almost certainly go unnoticed in manual use. Kevin
Re: [PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally
Am 19.10.2021 um 10:06 hat Laurent Vivier geschrieben:
> On 08/10/2021 15:34, Kevin Wolf wrote:
> > Instead of accessing the global QemuOptsList, which really belong to the
> > command line parser and shouldn't be accessed from devices, store a
> > pointer to the QemuOpts in a new VirtIONet field.
> >
> > This is not the final state, but just an intermediate step to get rid of
> > QemuOpts in devices. It will later be replaced with an options QDict.
> >
> > Before this patch, two "primary" devices could be hidden for the same
> > standby device, but only one of them would actually be enabled and the
> > other one would be kept hidden forever, so this doesn't make sense.
> > After this patch, configuring a second primary device is an error.
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > include/hw/virtio/virtio-net.h | 1 +
> > hw/net/virtio-net.c| 26 ++
> > 2 files changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index 824a69c23f..d118c95f69 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -209,6 +209,7 @@ struct VirtIONet {
> > bool failover_primary_hidden;
> > bool failover;
> > DeviceListener primary_listener;
> > +QemuOpts *primary_opts;
> > Notifier migration_state;
> > VirtioNetRssData rss_data;
> > struct NetRxPkt *rx_pkt;
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a17d5739fc..ed9a9012e9 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -858,27 +858,24 @@ static DeviceState
> > *failover_find_primary_device(VirtIONet *n)
> > static void failover_add_primary(VirtIONet *n, Error **errp)
> > {
> > Error *err = NULL;
> > -QemuOpts *opts;
> > -char *id;
> > DeviceState *dev = failover_find_primary_device(n);
> > if (dev) {
> > return;
> > }
> > -id = failover_find_primary_device_id(n);
> > -if (!id) {
> > +if (!n->primary_opts) {
> > error_setg(errp, "Primary device not found");
> > error_append_hint(errp, "Virtio-net failover will not work. Make "
> > "sure primary device has parameter"
> > " failover_pair_id=%s\n", n->netclient_name);
> > return;
> > }
> > -opts = qemu_opts_find(qemu_find_opts("device"), id);
> > -g_assert(opts); /* cannot be NULL because id was found using opts list
> > */
> > -dev = qdev_device_add(opts, );
> > +
> > +dev = qdev_device_add(n->primary_opts, );
> > if (err) {
> > -qemu_opts_del(opts);
> > +qemu_opts_del(n->primary_opts);
> > +n->primary_opts = NULL;
> > } else {
> > object_unref(OBJECT(dev));
> > }
> > @@ -3317,6 +3314,19 @@ static bool
> > failover_hide_primary_device(DeviceListener *listener,
> > return false;
> > }
> > +if (n->primary_opts) {
> > +error_setg(errp, "Cannot attach more than one primary device to
> > '%s'",
> > + n->netclient_name);
> > +return false;
> > +}
> > +
>
> This part has introduced a regression, I've sent a patch to fix that.
>
> https://patchew.org/QEMU/20211019071532.682717-1-lviv...@redhat.com/
Thanks for catching this! The fix looks good to me.
Kevin
Re: [PATCH v2 00/15] qdev: Add JSON -device
Am 08.10.2021 um 15:34 hat Kevin Wolf geschrieben: > It's still a long way until we'll have QAPIfied devices, but there are > some improvements that we can already make now to make the future switch > easier. > > One important part of this is having code paths without QemuOpts, which > we want to get rid of and replace with the keyval parser in the long > run. This series adds support for JSON syntax to -device, which bypasses > QemuOpts. > > While we're not using QAPI yet, devices are based on QOM, so we already > do have type checks and an implied schema. JSON syntax supported now can > be supported by QAPI later and regarding command line compatibility, > actually switching to it becomes an implementation detail this way (of > course, it will still add valuable user-visible features like > introspection and documentation). > > Apart from making things more future proof, this also immediately adds > a way to do non-scalar properties on the command line. nvme could have > used list support recently, and the lack of it in -device led to some > rather unnatural solution in the first version (doing the relationship > between a device and objects backwards) and loss of features in the > following. With this series, using a list as a device property should be > possible without any weird tricks. > > Unfortunately, even QMP device_add goes through QemuOpts before this > series, which destroys any type safety QOM provides and also can't > support non-scalar properties. This is a bug, but it turns out that > libvirt actually relies on it and passes only strings for everything. > So this series still leaves device_add alone until libvirt is fixed. Thanks for the review and testing, applied to my tree. Kevin
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
Am 13.10.2021 um 23:37 hat Eric Blake geschrieben:
> On Wed, Oct 13, 2021 at 03:10:38PM +0200, Damien Hedde wrote:
> > > > @@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
> > > > **errp)
> > > > }
> > > > }
> > > > -qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
> > > > +/*
> > > > + * set dev's parent and register its id.
> > > > + * If it fails it means the id is already taken.
> > > > + */
> > > > +if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
> > > > +goto err_del_dev;
> > >
> > > ...nor on this, which means the g_strdup() leaks on failure.
> > >
> >
> > Since we strdup the id just before calling qdev_set_id.
> > Maybe we should do the the strdup in qdev_set_id (and free things on error
> > there too). It seems simplier than freeing things on the callee side just in
> > case of an error.
>
> Indeed. If we expected qdev_set_id() to be passed something that it
> can later free, we would have used 'char *'; but because we used
> 'const char *' for that parameter, it really does make more sense for
> the callers to pass in any string and for qdev_set_id() to do the
> necessary strdup()ing, as well as clean up on error.
Since this seems to be the only thing in the series that needs to be
addressed, I'll do the minimal fix while applying (adding g_free() to
the error path in qemu_opts_id()) and then we can clean up on top.
Kevin
Re: [PATCH v2 00/15] qdev: Add JSON -device
Am 13.10.2021 um 17:30 hat Michael S. Tsirkin geschrieben: > On Fri, Oct 08, 2021 at 03:34:27PM +0200, Kevin Wolf wrote: > > It's still a long way until we'll have QAPIfied devices, but there are > > some improvements that we can already make now to make the future switch > > easier. > > > > One important part of this is having code paths without QemuOpts, which > > we want to get rid of and replace with the keyval parser in the long > > run. This series adds support for JSON syntax to -device, which bypasses > > QemuOpts. > > > > While we're not using QAPI yet, devices are based on QOM, so we already > > do have type checks and an implied schema. JSON syntax supported now can > > be supported by QAPI later and regarding command line compatibility, > > actually switching to it becomes an implementation detail this way (of > > course, it will still add valuable user-visible features like > > introspection and documentation). > > > > Apart from making things more future proof, this also immediately adds > > a way to do non-scalar properties on the command line. nvme could have > > used list support recently, and the lack of it in -device led to some > > rather unnatural solution in the first version (doing the relationship > > between a device and objects backwards) and loss of features in the > > following. With this series, using a list as a device property should be > > possible without any weird tricks. > > > > Unfortunately, even QMP device_add goes through QemuOpts before this > > series, which destroys any type safety QOM provides and also can't > > support non-scalar properties. This is a bug, but it turns out that > > libvirt actually relies on it and passes only strings for everything. > > So this series still leaves device_add alone until libvirt is fixed. > > > Reviewed-by: Michael S. Tsirkin > > I assume you are merging this? Yes, I can merge it through my tree. Thanks for the review! Kevin
Re: [PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup
Am 11.10.2021 um 20:58 hat Eric Blake geschrieben:
> On Sat, Oct 09, 2021 at 02:09:44PM +0200, Markus Armbruster wrote:
> > Several moons ago, Vladimir posted
> >
> > Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
> > Date: Wed, 5 May 2021 16:58:03 +0300
> > Message-Id: <20210505135803.67896-4-vsement...@virtuozzo.com>
> > https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
> >
> > with this
> >
> > TODO: We also need to deprecate drive-backup transaction action..
> > But union members in QAPI doesn't support 'deprecated' feature. I tried
> > to dig a bit, but failed :/ Markus, could you please help with it? At
> > least by advice?
> >
> > This is one way to resolve it. Sorry it took so long.
> >
> > John explored another way, namely adding feature flags to union
> > branches. Could also be useful, say to add different features to
> > branches in multiple unions sharing the same tag enum.
> >
> > Signed-off-by: Markus Armbruster
> > ---
> > qapi/transaction.json | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/transaction.json b/qapi/transaction.json
> > index d175b5f863..0564a893b3 100644
> > --- a/qapi/transaction.json
> > +++ b/qapi/transaction.json
> > @@ -54,6 +54,9 @@
> > # @blockdev-snapshot-sync: since 1.1
> > # @drive-backup: Since 1.6
> > #
> > +# Features:
> > +# @deprecated: Member @drive-backup is deprecated. Use FIXME instead.
>
> Obviously, we'd need to flesh this out ("'blockdev-backup' with proper
> node names"? something else?) before dropping RFC on this patch.
What does 'blockdev-backup' with improper node names look like?
I think it's sufficient to say "Use @blockdev-backup instead", which is
already documented to take a node/device name instead of a file name.
> And we'd want to edit docs/about/deprecated.rst to match.
Yes.
Kevin
Re: [PATCH v2 2/5] qapi: Add feature flags to enum members
Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben: > This is quite similar to commit 84ab008687 "qapi: Add feature flags to > struct members", only for enums instead of structs. > > Special feature flag 'deprecated' is silently ignored there. This is > okay only because it will be implemented shortly. > > Signed-off-by: Markus Armbruster > Reviewed-by: Eric Blake Should we have a test case for an invalid value for 'features'? Kevin
Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name
Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
> The next commit will add feature flags to enum members. There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values). If it showed
> an array of objects with a name member, we could simply add more
> members to these objects. Since it's just strings, we can't.
>
> I can see three ways to correct this design mistake:
>
> 1. Do it the way we should have done it, plus compatibility goo.
>
>We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum. Since
>changing @values would be a compatibility break, add a new member
>@members instead.
>
>@values is now redundant. In my testing, output of
>qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
>
>We can deprecate @values now and drop it later. This will break
>outmoded clients. Well-behaved clients such as libvirt are
>expected to break cleanly.
>
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>
>@values does not become redundant. @members augments it. Somewhat
>cumbersome, but output of query-qmp-schema grows only as we make
>enum members non-boring.
>
>There is nothing to deprecate here.
>
> 3. Versioned query-qmp-schema.
>
>query-qmp-schema provides either @values or @members. The QMP
>client can select which version it wants. There is no redundant
>output.
>
>We can deprecate old versions and eventually drop them. This will
>break outmoded clients. Breaking cleanly is easier than for 1.
>
>While 1 and 2 operate within the common rules for compatible
>evolution apply (section "Compatibility considerations" in
>docs/devel/qapi-code-gen.rst), 3 bypasses them. Attractive when
>operating within the rules is just too awkward. Not the case here.
>
> This commit implements 1. Libvirt developers prefer it.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/introspect.json | 21 +++--
> scripts/qapi/introspect.py | 18 ++
> 2 files changed, 33 insertions(+), 6 deletions(-)
docs/devel/qapi-code-gen.rst still says:
The SchemaInfo for an enumeration type has meta-type "enum" and
variant member "values". The values are listed in no particular
order; clients must search the entire enum when learning whether a
particular value is supported.
Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::
{ "name": "MyEnum", "meta-type": "enum",
"values": [ "value1", "value2", "value3" ] }
It probably needs an update.
Kevin
[PATCH v2 07/15] qdev: Avoid using string visitor for properties
The only thing the string visitor adds compared to a keyval visitor is
list support. git grep for 'visit_start_list' and 'visit.*List' shows
that devices don't make use of this.
In a world with a QAPIfied command line interface, the keyval visitor is
used to parse the command line. In order to make sure that no devices
start using this feature that would make backwards compatibility harder,
just switch away from object_property_parse(), which internally uses the
string visitor, to a keyval visitor and object_property_set().
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
softmmu/qdev-monitor.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f00846..034b999401 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -28,6 +28,8 @@
#include "qapi/qmp/dispatch.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
#include "qemu/help_option.h"
@@ -198,16 +200,28 @@ static int set_property(void *opaque, const char *name,
const char *value,
Error **errp)
{
Object *obj = opaque;
+QString *val;
+Visitor *v;
+int ret;
if (strcmp(name, "driver") == 0)
return 0;
if (strcmp(name, "bus") == 0)
return 0;
-if (!object_property_parse(obj, name, value, errp)) {
-return -1;
+val = qstring_from_str(value);
+v = qobject_input_visitor_new_keyval(QOBJECT(val));
+
+if (!object_property_set(obj, name, v, errp)) {
+ret = -1;
+goto out;
}
-return 0;
+
+ret = 0;
+out:
+visit_free(v);
+qobject_unref(val);
+return ret;
}
static const char *find_typename_by_alias(const char *alias)
--
2.31.1
[PATCH v2 10/15] qemu-option: Allow deleting opts during qemu_opts_foreach()
Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted
while iterating through the whole list.
Signed-off-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
util/qemu-option.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 61cb4a97bd..eedd08929b 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1126,11 +1126,11 @@ int qemu_opts_foreach(QemuOptsList *list,
qemu_opts_loopfunc func,
void *opaque, Error **errp)
{
Location loc;
-QemuOpts *opts;
+QemuOpts *opts, *next;
int rc = 0;
loc_push_none();
-QTAILQ_FOREACH(opts, >head, next) {
+QTAILQ_FOREACH_SAFE(opts, >head, next, next) {
loc_restore(>loc);
rc = func(opaque, opts, errp);
if (rc) {
--
2.31.1
[PATCH v2 15/15] vl: Enable JSON syntax for -device
Like we already do for -object, introduce support for JSON syntax in
-device, which can be kept stable in the long term and guarantees that a
single code path with identical behaviour is used for both QMP and the
command line. Compared to the QemuOpts based code, the parser contains
less surprises and has support for non-scalar options (lists and
structs). Switching management tools to JSON means that we can more
easily change the "human" CLI syntax from QemuOpts to the keyval parser
later.
In the QAPI schema, a feature flag is added to the device-add command to
allow management tools to detect support for this.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
qapi/qdev.json | 15
softmmu/vl.c | 63 --
2 files changed, 67 insertions(+), 11 deletions(-)
diff --git a/qapi/qdev.json b/qapi/qdev.json
index d75e68908b..69656b14df 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -32,17 +32,23 @@
##
# @device_add:
#
+# Add a device.
+#
# @driver: the name of the new device's driver
#
# @bus: the device's parent bus (device tree path)
#
# @id: the device's ID, must be unique
#
-# Additional arguments depend on the type.
-#
-# Add a device.
+# Features:
+# @json-cli: If present, the "-device" command line option supports JSON
+#syntax with a structure identical to the arguments of this
+#command.
#
# Notes:
+#
+# Additional arguments depend on the type.
+#
# 1. For detailed information about this command, please refer to the
#'docs/qdev-device-use.txt' file.
#
@@ -67,7 +73,8 @@
##
{ 'command': 'device_add',
'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
- 'gen': false } # so we can get the additional arguments
+ 'gen': false, # so we can get the additional arguments
+ 'features': ['json-cli'] }
##
# @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97..af0c4cbd99 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -144,6 +144,12 @@ typedef struct ObjectOption {
QTAILQ_ENTRY(ObjectOption) next;
} ObjectOption;
+typedef struct DeviceOption {
+QDict *opts;
+Location loc;
+QTAILQ_ENTRY(DeviceOption) next;
+} DeviceOption;
+
static const char *cpu_option;
static const char *mem_path;
static const char *incoming;
@@ -151,6 +157,7 @@ static const char *loadvm;
static const char *accelerators;
static QDict *machine_opts_dict;
static QTAILQ_HEAD(, ObjectOption) object_opts =
QTAILQ_HEAD_INITIALIZER(object_opts);
+static QTAILQ_HEAD(, DeviceOption) device_opts =
QTAILQ_HEAD_INITIALIZER(device_opts);
static ram_addr_t maxram_size;
static uint64_t ram_slots;
static int display_remote;
@@ -494,21 +501,39 @@ const char *qemu_get_vm_name(void)
return qemu_name;
}
-static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+static void default_driver_disable(const char *driver)
{
-const char *driver = qemu_opt_get(opts, "driver");
int i;
-if (!driver)
-return 0;
+if (!driver) {
+return;
+}
+
for (i = 0; i < ARRAY_SIZE(default_list); i++) {
if (strcmp(default_list[i].driver, driver) != 0)
continue;
*(default_list[i].flag) = 0;
}
+}
+
+static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *driver = qemu_opt_get(opts, "driver");
+
+default_driver_disable(driver);
return 0;
}
+static void default_driver_check_json(void)
+{
+DeviceOption *opt;
+
+QTAILQ_FOREACH(opt, _opts, next) {
+const char *driver = qdict_get_try_str(opt->opts, "driver");
+default_driver_disable(driver);
+}
+}
+
static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
{
const char *proc_name;
@@ -1311,6 +1336,7 @@ static void qemu_disable_default_devices(void)
{
MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
+default_driver_check_json();
qemu_opts_foreach(qemu_find_opts("device"),
default_driver_check, NULL, NULL);
qemu_opts_foreach(qemu_find_opts("global"),
@@ -2637,6 +2663,8 @@ static void qemu_init_board(void)
static void qemu_create_cli_devices(void)
{
+DeviceOption *opt;
+
soundhw_init();
qemu_opts_foreach(qemu_find_opts("fw_cfg"),
@@ -2652,6 +2680,18 @@ static void qemu_create_cli_devices(void)
rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
qemu_opts_foreach(qemu_find_opts("device"),
device_init_func, NULL, _fatal);
+QTAILQ_FOREACH(opt, _opts, next) {
+loc_push_restore(>loc);
+/*
+ * TODO Eventually we should call qmp_device_add() here to make sure it
+ * behaves the same, but QMP still has to accept incorrectly typed
+ * options until libvirt is fixed and we want to be strict on th
[PATCH v2 14/15] qdev: Base object creation on QDict rather than QemuOpts
QDicts are both what QMP natively uses and what the keyval parser
produces. Going through QemuOpts isn't useful for either one, so switch
the main device creation function to QDicts. By sharing more code with
the -object/object-add code path, we can even reduce the code size a
bit.
This commit doesn't remove the detour through QemuOpts from any code
path yet, but it allows the following commits to do so.
Signed-off-by: Kevin Wolf
---
include/hw/qdev-core.h | 11 +++---
include/hw/virtio/virtio-net.h | 3 +-
include/monitor/qdev.h | 2 +
hw/core/qdev.c | 7 ++--
hw/net/virtio-net.c| 23 +++-
hw/vfio/pci.c | 4 +-
softmmu/qdev-monitor.c | 69 +++---
7 files changed, 60 insertions(+), 59 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 74d8b614a7..910042c650 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,7 +180,7 @@ struct DeviceState {
char *canonical_path;
bool realized;
bool pending_deleted_event;
-QemuOpts *opts;
+QDict *opts;
int hotplugged;
bool allow_unplug_during_migration;
BusState *parent_bus;
@@ -205,8 +205,8 @@ struct DeviceListener {
* On errors, it returns false and errp is set. Device creation
* should fail in this case.
*/
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
-Error **errp);
+bool (*hide_device)(DeviceListener *listener, const QDict *device_opts,
+bool from_json, Error **errp);
QTAILQ_ENTRY(DeviceListener) link;
};
@@ -835,13 +835,14 @@ void device_listener_unregister(DeviceListener *listener);
/**
* @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict
+ * @from_json: true if @opts entries are typed, false for all strings
*
* Check if a device should be added.
* When a device is added via qdev_device_add() this will be called,
* and return if the device should be added now or not.
*/
-bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
typedef enum MachineInitPhase {
/* current_machine is NULL. */
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index d118c95f69..74a10ebe85 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,7 +209,8 @@ struct VirtIONet {
bool failover_primary_hidden;
bool failover;
DeviceListener primary_listener;
-QemuOpts *primary_opts;
+QDict *primary_opts;
+bool primary_opts_from_json;
Notifier migration_state;
VirtioNetRssData rss_data;
struct NetRxPkt *rx_pkt;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 74e6c55a2b..1d57bf6577 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
+DeviceState *qdev_device_add_from_qdict(const QDict *opts,
+bool from_json, Error **errp);
/**
* qdev_set_id: parent the device and set its id if provided.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c3a021c444..7f06403752 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -28,6 +28,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qapi/qapi-events-qdev.h"
+#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
@@ -211,14 +212,14 @@ void device_listener_unregister(DeviceListener *listener)
QTAILQ_REMOVE(_listeners, listener, link);
}
-bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp)
{
ERRP_GUARD();
DeviceListener *listener;
QTAILQ_FOREACH(listener, _listeners, link) {
if (listener->hide_device) {
-if (listener->hide_device(listener, opts, errp)) {
+if (listener->hide_device(listener, opts, from_json, errp)) {
return true;
} else if (*errp) {
return false;
@@ -958,7 +959,7 @@ static void device_finalize(Object *obj)
dev->canonical_path = NULL;
}
-qemu_opts_del(dev->opts);
+qobject_unref(dev->opts);
g_free(dev->id);
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f503f28c00..09e173a558 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,9 +858,11 @@ static void failover_add_primary(VirtIONet *n, Error
**errp)
return;
}
-dev = qdev_device_add(n->primary_opts, );
+dev = qdev_device_a
[PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally
Instead of accessing the global QemuOptsList, which really belong to the
command line parser and shouldn't be accessed from devices, store a
pointer to the QemuOpts in a new VirtIONet field.
This is not the final state, but just an intermediate step to get rid of
QemuOpts in devices. It will later be replaced with an options QDict.
Before this patch, two "primary" devices could be hidden for the same
standby device, but only one of them would actually be enabled and the
other one would be kept hidden forever, so this doesn't make sense.
After this patch, configuring a second primary device is an error.
Signed-off-by: Kevin Wolf
---
include/hw/virtio/virtio-net.h | 1 +
hw/net/virtio-net.c| 26 ++
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f..d118c95f69 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,6 +209,7 @@ struct VirtIONet {
bool failover_primary_hidden;
bool failover;
DeviceListener primary_listener;
+QemuOpts *primary_opts;
Notifier migration_state;
VirtioNetRssData rss_data;
struct NetRxPkt *rx_pkt;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a17d5739fc..ed9a9012e9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,27 +858,24 @@ static DeviceState
*failover_find_primary_device(VirtIONet *n)
static void failover_add_primary(VirtIONet *n, Error **errp)
{
Error *err = NULL;
-QemuOpts *opts;
-char *id;
DeviceState *dev = failover_find_primary_device(n);
if (dev) {
return;
}
-id = failover_find_primary_device_id(n);
-if (!id) {
+if (!n->primary_opts) {
error_setg(errp, "Primary device not found");
error_append_hint(errp, "Virtio-net failover will not work. Make "
"sure primary device has parameter"
" failover_pair_id=%s\n", n->netclient_name);
return;
}
-opts = qemu_opts_find(qemu_find_opts("device"), id);
-g_assert(opts); /* cannot be NULL because id was found using opts list */
-dev = qdev_device_add(opts, );
+
+dev = qdev_device_add(n->primary_opts, );
if (err) {
-qemu_opts_del(opts);
+qemu_opts_del(n->primary_opts);
+n->primary_opts = NULL;
} else {
object_unref(OBJECT(dev));
}
@@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener
*listener,
return false;
}
+if (n->primary_opts) {
+error_setg(errp, "Cannot attach more than one primary device to '%s'",
+ n->netclient_name);
+return false;
+}
+
+/*
+ * Having a weak reference here should be okay because a device can't be
+ * deleted while it's hidden. This will be replaced soon with a QDict that
+ * has a clearer ownership model.
+ */
+n->primary_opts = device_opts;
+
/* failover_primary_hidden is set during feature negotiation */
return qatomic_read(>failover_primary_hidden);
}
--
2.31.1
[PATCH v2 13/15] virtio-net: Avoid QemuOpts in failover_find_primary_device()
Don't go through the global QemuOptsList, it is state of the legacy
command line parser and we will create devices that are not contained
in it. It is also just the command line configuration and not
necessarily the current runtime state.
Instead, look at the qdev device tree which has the current state of all
existing devices.
Signed-off-by: Kevin Wolf
---
hw/net/virtio-net.c | 52 +
1 file changed, 19 insertions(+), 33 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index ed9a9012e9..f503f28c00 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -796,48 +796,34 @@ static inline uint64_t
virtio_net_supported_guest_offloads(VirtIONet *n)
typedef struct {
VirtIONet *n;
-char *id;
-} FailoverId;
+DeviceState *dev;
+} FailoverDevice;
/**
- * Set the id of the failover primary device
+ * Set the failover primary device
*
* @opaque: FailoverId to setup
* @opts: opts for device we are handling
* @errp: returns an error if this function fails
*/
-static int failover_set_primary(void *opaque, QemuOpts *opts, Error **errp)
+static int failover_set_primary(DeviceState *dev, void *opaque)
{
-FailoverId *fid = opaque;
-const char *standby_id = qemu_opt_get(opts, "failover_pair_id");
+FailoverDevice *fdev = opaque;
+PCIDevice *pci_dev = (PCIDevice *)
+object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE);
-if (g_strcmp0(standby_id, fid->n->netclient_name) == 0) {
-fid->id = g_strdup(opts->id);
+if (!pci_dev) {
+return 0;
+}
+
+if (!g_strcmp0(pci_dev->failover_pair_id, fdev->n->netclient_name)) {
+fdev->dev = dev;
return 1;
}
return 0;
}
-/**
- * Find the primary device id for this failover virtio-net
- *
- * @n: VirtIONet device
- * @errp: returns an error if this function fails
- */
-static char *failover_find_primary_device_id(VirtIONet *n)
-{
-Error *err = NULL;
-FailoverId fid;
-
-fid.n = n;
-if (!qemu_opts_foreach(qemu_find_opts("device"),
- failover_set_primary, , )) {
-return NULL;
-}
-return fid.id;
-}
-
/**
* Find the primary device for this failover virtio-net
*
@@ -846,13 +832,13 @@ static char *failover_find_primary_device_id(VirtIONet *n)
*/
static DeviceState *failover_find_primary_device(VirtIONet *n)
{
-char *id = failover_find_primary_device_id(n);
-
-if (!id) {
-return NULL;
-}
+FailoverDevice fdev = {
+.n = n,
+};
-return qdev_find_recursive(sysbus_get_default(), id);
+qbus_walk_children(sysbus_get_default(), failover_set_primary, NULL,
+ NULL, NULL, );
+return fdev.dev;
}
static void failover_add_primary(VirtIONet *n, Error **errp)
--
2.31.1
[PATCH v2 11/15] qdev: Add Error parameter to hide_device() callbacks
hide_device() is used for virtio-net failover, where the standby virtio
device delays creation of the primary device. It only makes sense to
have a single primary device for each standby device. Adding a second
one should result in an error instead of hiding it and never using it
afterwards.
Prepare for this by adding an Error parameter to the hide_device()
callback where virtio-net is informed about adding a primary device.
Signed-off-by: Kevin Wolf
---
include/hw/qdev-core.h | 8 ++--
hw/core/qdev.c | 7 +--
hw/net/virtio-net.c| 2 +-
softmmu/qdev-monitor.c | 5 -
4 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5a073fc368..74d8b614a7 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -201,8 +201,12 @@ struct DeviceListener {
* informs qdev if a device should be visible or hidden. We can
* hide a failover device depending for example on the device
* opts.
+ *
+ * On errors, it returns false and errp is set. Device creation
+ * should fail in this case.
*/
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts);
+bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
+Error **errp);
QTAILQ_ENTRY(DeviceListener) link;
};
@@ -837,7 +841,7 @@ void device_listener_unregister(DeviceListener *listener);
* When a device is added via qdev_device_add() this will be called,
* and return if the device should be added now or not.
*/
-bool qdev_should_hide_device(QemuOpts *opts);
+bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
typedef enum MachineInitPhase {
/* current_machine is NULL. */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d918b50a1d..c3a021c444 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -211,14 +211,17 @@ void device_listener_unregister(DeviceListener *listener)
QTAILQ_REMOVE(_listeners, listener, link);
}
-bool qdev_should_hide_device(QemuOpts *opts)
+bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
{
+ERRP_GUARD();
DeviceListener *listener;
QTAILQ_FOREACH(listener, _listeners, link) {
if (listener->hide_device) {
-if (listener->hide_device(listener, opts)) {
+if (listener->hide_device(listener, opts, errp)) {
return true;
+} else if (*errp) {
+return false;
}
}
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..a17d5739fc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3304,7 +3304,7 @@ static void virtio_net_migration_state_notifier(Notifier
*notifier, void *data)
}
static bool failover_hide_primary_device(DeviceListener *listener,
- QemuOpts *device_opts)
+ QemuOpts *device_opts, Error **errp)
{
VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
const char *standby_id;
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index feb15818e6..ccc3c11563 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -625,6 +625,7 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error
**errp)
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
{
+ERRP_GUARD();
DeviceClass *dc;
const char *driver, *path;
DeviceState *dev = NULL;
@@ -668,11 +669,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
error_setg(errp, "Device with failover_pair_id don't have id");
return NULL;
}
-if (qdev_should_hide_device(opts)) {
+if (qdev_should_hide_device(opts, errp)) {
if (bus && !qbus_is_hotpluggable(bus)) {
error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
}
return NULL;
+} else if (*errp) {
+return NULL;
}
}
--
2.31.1
[PATCH v2 04/15] qom: Reduce use of error_propagate()
ERRP_GUARD() makes debugging easier by making sure that _abort
still fails at the real origin of the error instead of
error_propagate().
Signed-off-by: Kevin Wolf
---
qom/object.c| 7 +++
qom/object_interfaces.c | 19 ++-
2 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name,
Visitor *v,
bool object_property_set(Object *obj, const char *name, Visitor *v,
Error **errp)
{
-Error *err = NULL;
+ERRP_GUARD();
ObjectProperty *prop = object_property_find_err(obj, name, errp);
if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name,
Visitor *v,
error_setg(errp, QERR_PERMISSION_DENIED);
return false;
}
-prop->set(obj, v, name, prop->opaque, );
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
}
bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..3b61c195c5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -46,25 +46,18 @@ static void object_set_properties_from_qdict(Object *obj,
const QDict *qdict,
Visitor *v, Error **errp)
{
const QDictEntry *e;
-Error *local_err = NULL;
-if (!visit_start_struct(v, NULL, NULL, 0, _err)) {
-goto out;
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+return;
}
for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, _err)) {
-break;
+if (!object_property_set(obj, e->key, v, errp)) {
+goto out;
}
}
-if (!local_err) {
-visit_check_struct(v, _err);
-}
-visit_end_struct(v, NULL);
-
+visit_check_struct(v, errp);
out:
-if (local_err) {
-error_propagate(errp, local_err);
-}
+visit_end_struct(v, NULL);
}
void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
--
2.31.1
[PATCH v2 05/15] iotests/245: Fix type for iothread property
iothread is a string property, so None (= JSON null) is not a valid
value for it. Pass the empty string instead to get the default iothread.
Signed-off-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/245 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bf8261eec0..9b12b42eed 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1189,10 +1189,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.run_test_iothreads('iothread0', 'iothread0')
def test_iothreads_switch_backing(self):
-self.run_test_iothreads('iothread0', None)
+self.run_test_iothreads('iothread0', '')
def test_iothreads_switch_overlay(self):
-self.run_test_iothreads(None, 'iothread0')
+self.run_test_iothreads('', 'iothread0')
if __name__ == '__main__':
iotests.activate_logging()
--
2.31.1
[PATCH v2 08/15] qdev: Make DeviceState.id independent of QemuOpts
DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.
Signed-off-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/hw/qdev-core.h | 2 +-
include/monitor/qdev.h | 2 +-
hw/arm/virt.c | 2 +-
hw/core/qdev.c | 1 +
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/ppc/e500.c | 2 +-
softmmu/qdev-monitor.c | 5 +++--
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4ff19c714b..5a073fc368 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
Object parent_obj;
/*< public >*/
-const char *id;
+char *id;
char *canonical_path;
bool realized;
bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
#endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7170aaacd5..4160d49688 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
MemoryRegion *sysmem = get_system_memory();
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
qdev_prop_set_uint32(dev, "mmio_size",
vms->memmap[VIRT_PLATFORM_BUS].size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
}
qemu_opts_del(dev->opts);
+g_free(dev->id);
}
static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
pcie, Error **errp)
} else {
bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
TYPE_PXB_BUS);
bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
/* Platform Bus Device */
if (pmc->has_platform_bus) {
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
return bus;
}
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
{
if (id) {
dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
2.31.1
[PATCH v2 06/15] iotests/051: Fix typo
The iothread isn't called 'iothread0', but 'thread0'. Depending on the order that properties are parsed, the error message may change from the expected one to another one saying that the iothread doesn't exist. Signed-off-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/051| 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 7bf29343d7..1d2fa93a11 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -199,7 +199,7 @@ case "$QEMU_DEFAULT_MACHINE" in # virtio-blk enables the iothread only when the driver initialises the # device, so a second virtio-blk device can't be added even with the # same iothread. virtio-scsi allows this. -run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on run_qemu $iothread -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on ;; *) diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index afe7632964..063e4fc584 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -183,9 +183,9 @@ Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id QEMU X.Y.Z monitor - type 'help' for more information (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: Cannot change iothread of active block backend -Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change iothread of active block backend +(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -- 2.31.1
[PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
From: Damien Hedde
qdev_set_id() is mostly used when the user adds a device (using
-device cli option or device_add qmp command). This commit adds
an error parameter to handle the case where the given id is
already taken.
Also document the function and add a return value in order to
be able to capture success/failure: the function now returns the
id in case of success, or NULL in case of failure.
The commit modifies the 2 calling places (qdev-monitor and
xen-legacy-backend) to add the error object parameter.
Note that the id is, right now, guaranteed to be unique because
all ids came from the "device" QemuOptsList where the id is used
as key. This addition is a preparation for a future commit which
will relax the uniqueness.
Signed-off-by: Damien Hedde
Signed-off-by: Kevin Wolf
---
include/monitor/qdev.h | 25 +++-
hw/xen/xen-legacy-backend.c | 3 ++-
softmmu/qdev-monitor.c | 38 +++--
3 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..74e6c55a2b 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,29 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+
+/**
+ * qdev_set_id: parent the device and set its id if provided.
+ * @dev: device to handle
+ * @id: id to be given to the device, or NULL.
+ *
+ * Returns: the id of the device in case of success; otherwise NULL.
+ *
+ * @dev must be unrealized, unparented and must not have an id.
+ *
+ * If @id is non-NULL, this function tries to setup @dev qom path as
+ * "/peripheral/id". If @id is already taken, it fails. If it succeeds,
+ * the id field of @dev is set to @id (@dev now owns the given @id
+ * parameter).
+ *
+ * If @id is NULL, this function generates a unique name and setups @dev
+ * qom path as "/peripheral-anon/name". This name is not set as the id
+ * of @dev.
+ *
+ * Upon success, it returns the id/name (generated or provided). The
+ * returned string is owned by the corresponding child property and must
+ * not be freed by the caller.
+ */
+const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index be3cf4a195..085fd31ef7 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char
*type, int dom,
xendev = g_malloc0(ops->size);
object_initialize(>qdev, ops->size, TYPE_XENBACKEND);
OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+_fatal);
qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..feb15818e6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,22 +593,34 @@ static BusState *qbus_find(const char *path, Error **errp)
}
/* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
{
-if (id) {
-dev->id = id;
-}
+ObjectProperty *prop;
-if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
- OBJECT(dev));
+assert(!dev->id && !dev->realized);
+
+/*
+ * object_property_[try_]add_child() below will assert the device
+ * has no parent
+ */
+if (id) {
+prop = object_property_try_add_child(qdev_get_peripheral(), id,
+ OBJECT(dev), NULL);
+if (prop) {
+dev->id = id;
+} else {
+error_setg(errp, "Duplicate device ID '%s'", id);
+return NULL;
+}
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
- OBJECT(dev));
+prop = object_property_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev));
g_free(name);
}
+
+return prop->name;
}
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
@@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+/*
+ * set dev's parent and register its id.
+ * If it fails it means the id is alre
[PATCH v2 01/15] net: Introduce NetClientInfo.check_peer_type()
Some network backends (vhost-user and vhost-vdpa) work only with
specific devices. At startup, they second guess what the command line
option handling will do and error out if they think a non-virtio device
will attach to them.
This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.
Add a callback where backends can check compatibility with a device when
it actually tries to attach, even on hotplug.
Signed-off-by: Kevin Wolf
---
include/net/net.h| 2 ++
hw/core/qdev-properties-system.c | 6 ++
2 files changed, 8 insertions(+)
diff --git a/include/net/net.h b/include/net/net.h
index 5d1508081f..986288eb07 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -62,6 +62,7 @@ typedef struct SocketReadState SocketReadState;
typedef void (SocketReadStateFinalize)(SocketReadState *rs);
typedef void (NetAnnounce)(NetClientState *);
typedef bool (SetSteeringEBPF)(NetClientState *, int);
+typedef bool (NetCheckPeerType)(NetClientState *, ObjectClass *, Error **);
typedef struct NetClientInfo {
NetClientDriver type;
@@ -84,6 +85,7 @@ typedef struct NetClientInfo {
SetVnetBE *set_vnet_be;
NetAnnounce *announce;
SetSteeringEBPF *set_steering_ebpf;
+NetCheckPeerType *check_peer_type;
} NetClientInfo;
struct NetClientState {
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index e71f5d64d1..a91f60567a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -431,6 +431,12 @@ static void set_netdev(Object *obj, Visitor *v, const char
*name,
goto out;
}
+if (peers[i]->info->check_peer_type) {
+if (!peers[i]->info->check_peer_type(peers[i], obj->class, errp)) {
+goto out;
+}
+}
+
ncs[i] = peers[i];
ncs[i]->queue_index = i;
}
--
2.31.1
[PATCH v2 02/15] net/vhost-user: Fix device compatibility check
vhost-user works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.
This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.
Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.
Signed-off-by: Kevin Wolf
---
net/vhost-user.c | 41 ++---
1 file changed, 14 insertions(+), 27 deletions(-)
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 4a939124d2..b1a0247b59 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -198,6 +198,19 @@ static bool vhost_user_has_ufo(NetClientState *nc)
return true;
}
+static bool vhost_user_check_peer_type(NetClientState *nc, ObjectClass *oc,
+ Error **errp)
+{
+const char *driver = object_class_get_name(oc);
+
+if (!g_str_has_prefix(driver, "virtio-net-")) {
+error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
+return false;
+}
+
+return true;
+}
+
static NetClientInfo net_vhost_user_info = {
.type = NET_CLIENT_DRIVER_VHOST_USER,
.size = sizeof(NetVhostUserState),
@@ -207,6 +220,7 @@ static NetClientInfo net_vhost_user_info = {
.has_ufo = vhost_user_has_ufo,
.set_vnet_be = vhost_user_set_vnet_endianness,
.set_vnet_le = vhost_user_set_vnet_endianness,
+.check_peer_type = vhost_user_check_peer_type,
};
static gboolean net_vhost_user_watch(void *do_not_use, GIOCondition cond,
@@ -397,27 +411,6 @@ static Chardev *net_vhost_claim_chardev(
return chr;
}
-static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
-{
-const char *name = opaque;
-const char *driver, *netdev;
-
-driver = qemu_opt_get(opts, "driver");
-netdev = qemu_opt_get(opts, "netdev");
-
-if (!driver || !netdev) {
-return 0;
-}
-
-if (strcmp(netdev, name) == 0 &&
-!g_str_has_prefix(driver, "virtio-net-")) {
-error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
-return -1;
-}
-
-return 0;
-}
-
int net_init_vhost_user(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
@@ -433,12 +426,6 @@ int net_init_vhost_user(const Netdev *netdev, const char
*name,
return -1;
}
-/* verify net frontend */
-if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
- (char *)name, errp)) {
-return -1;
-}
-
queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
if (queues < 1 || queues > MAX_QUEUE_NUM) {
error_setg(errp,
--
2.31.1
[PATCH v2 03/15] net/vhost-vdpa: Fix device compatibility check
vhost-vdpa works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.
This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.
Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.
Signed-off-by: Kevin Wolf
---
net/vhost-vdpa.c | 37 ++---
1 file changed, 14 insertions(+), 23 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 912686457c..6dc68d8677 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -147,12 +147,26 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
}
+static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
+ Error **errp)
+{
+const char *driver = object_class_get_name(oc);
+
+if (!g_str_has_prefix(driver, "virtio-net-")) {
+error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
+return false;
+}
+
+return true;
+}
+
static NetClientInfo net_vhost_vdpa_info = {
.type = NET_CLIENT_DRIVER_VHOST_VDPA,
.size = sizeof(VhostVDPAState),
.cleanup = vhost_vdpa_cleanup,
.has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
.has_ufo = vhost_vdpa_has_ufo,
+.check_peer_type = vhost_vdpa_check_peer_type,
};
static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
@@ -179,24 +193,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const
char *device,
return ret;
}
-static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
-{
-const char *name = opaque;
-const char *driver, *netdev;
-
-driver = qemu_opt_get(opts, "driver");
-netdev = qemu_opt_get(opts, "netdev");
-if (!driver || !netdev) {
-return 0;
-}
-if (strcmp(netdev, name) == 0 &&
-!g_str_has_prefix(driver, "virtio-net-")) {
-error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
-return -1;
-}
-return 0;
-}
-
int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
@@ -204,10 +200,5 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char
*name,
assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
opts = >u.vhost_vdpa;
-/* verify net frontend */
-if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
- (char *)name, errp)) {
-return -1;
-}
return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
}
--
2.31.1
[PATCH v2 00/15] qdev: Add JSON -device
It's still a long way until we'll have QAPIfied devices, but there are some improvements that we can already make now to make the future switch easier. One important part of this is having code paths without QemuOpts, which we want to get rid of and replace with the keyval parser in the long run. This series adds support for JSON syntax to -device, which bypasses QemuOpts. While we're not using QAPI yet, devices are based on QOM, so we already do have type checks and an implied schema. JSON syntax supported now can be supported by QAPI later and regarding command line compatibility, actually switching to it becomes an implementation detail this way (of course, it will still add valuable user-visible features like introspection and documentation). Apart from making things more future proof, this also immediately adds a way to do non-scalar properties on the command line. nvme could have used list support recently, and the lack of it in -device led to some rather unnatural solution in the first version (doing the relationship between a device and objects backwards) and loss of features in the following. With this series, using a list as a device property should be possible without any weird tricks. Unfortunately, even QMP device_add goes through QemuOpts before this series, which destroys any type safety QOM provides and also can't support non-scalar properties. This is a bug, but it turns out that libvirt actually relies on it and passes only strings for everything. So this series still leaves device_add alone until libvirt is fixed. v2: - Drop type safe QMP device_add because libvirt gets it wrong, too - More network patches to eliminate dependencies on the legacy QemuOpts data structures which would not contain all devices any more after this series. Fix some bugs there as a side effect. - Remove an unnecessary use of ERRP_GUARD() - Replaced error handling patch for qdev_set_id() with Damien's - Drop the deprecation patch until libvirt uses the new JSON syntax Damien Hedde (1): softmmu/qdev-monitor: add error handling in qdev_set_id Kevin Wolf (14): net: Introduce NetClientInfo.check_peer_type() net/vhost-user: Fix device compatibility check net/vhost-vdpa: Fix device compatibility check qom: Reduce use of error_propagate() iotests/245: Fix type for iothread property iotests/051: Fix typo qdev: Avoid using string visitor for properties qdev: Make DeviceState.id independent of QemuOpts qemu-option: Allow deleting opts during qemu_opts_foreach() qdev: Add Error parameter to hide_device() callbacks virtio-net: Store failover primary opts pointer locally virtio-net: Avoid QemuOpts in failover_find_primary_device() qdev: Base object creation on QDict rather than QemuOpts vl: Enable JSON syntax for -device qapi/qdev.json | 15 +++-- include/hw/qdev-core.h | 15 +++-- include/hw/virtio/virtio-net.h | 2 + include/monitor/qdev.h | 27 +++- include/net/net.h | 2 + hw/arm/virt.c | 2 +- hw/core/qdev-properties-system.c| 6 ++ hw/core/qdev.c | 11 +++- hw/net/virtio-net.c | 85 - hw/pci-bridge/pci_expander_bridge.c | 2 +- hw/ppc/e500.c | 2 +- hw/vfio/pci.c | 4 +- hw/xen/xen-legacy-backend.c | 3 +- net/vhost-user.c| 41 net/vhost-vdpa.c| 37 --- qom/object.c| 7 +- qom/object_interfaces.c | 19 ++ softmmu/qdev-monitor.c | 99 +++-- softmmu/vl.c| 63 -- util/qemu-option.c | 4 +- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.pc.out | 4 +- tests/qemu-iotests/245 | 4 +- 23 files changed, 278 insertions(+), 178 deletions(-) -- 2.31.1
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Am 06.10.2021 um 11:20 hat Laurent Vivier geschrieben: > On 06/10/2021 10:21, Juan Quintela wrote: > > Kevin Wolf wrote: > > > Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben: > > > > Hi > > > > > > > Usage > > > > > - > > > > > > > > > > The primary device can be hotplugged or be part of the startup > > > > > configuration > > > > > > > > > >-device virtio-net-pci,netdev=hostnet1,id=net1, > > > > >mac=52:54:00:6f:55:cc,bus=root2,failover=on > > > > > > > > > > With the parameter failover=on the VIRTIO_NET_F_STANDBY feature > > > > > will be enabled. > > > > > > > > > > -device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1, > > > > > failover_pair_id=net1 > > > > > > > > > > failover_pair_id references the id of the virtio-net standby device. > > > > > This is only for pairing the devices within QEMU. The guest kernel > > > > > module net_failover will match devices with identical MAC addresses. > > > > > > > > > > Hotplug > > > > > --- > > > > > > > > > > Both primary and standby device can be hotplugged via the QEMU > > > > > monitor. Note that if the virtio-net device is plugged first a > > > > > warning will be issued that it couldn't find the primary device. > > > > > > > > So maybe this whole primary device lookup can happen during the -device > > > > CLI > > > > option creation loop. And we can indeed have un-created devices still > > > > in the > > > > list ? > > > > > > Yes, that's the only case for which I could imagine for an inconsistency > > > between the qdev tree and QemuOpts, but failover_add_primary() is only > > > called after feature negotiation with the guest driver, so we can be > > > sure that the -device loop has completed long ago. > > > > > > And even if it hadn't completed yet, the paragraph also says that even > > > hotplugging the device later is supported, so creating devices in the > > > wrong order should still succeed. > > > > > > I hope that some of the people I added to CC have some more hints. > > > > Failover is ... interesting. > > > > You have two devices: primary and seconday. > > seconday is virtio-net, primary can be vfio and some other emulated > > devices. > > > > In the command line, devices can appear on any order, primary then > > secondary, secondary then primary, or only one of them. > > You can add (any of them) later in the toplevel. > > > > And now, what all this mess is about. We only enable the primary if the > > guest knows about failover. Otherwise we use only the virtio device > > (*). The important bit here is that we need to wait until the guest is > > booted, and the virtio-net driver is loaded, and then it tells us if it > > understands failover (or not). At that point we decide if we want to > > "really" create the primary. > > > > I know that it abuses device_add() as much as it can be, but I can't see > > any better way to handle it. We need to be able to "create" a device > > without showing it to the guest. And later, when we create a different > > device, and depending of driver support on the guest, we "finish" the > > creation of the primary device. > > > > Any good idea? Hm, the naive idea would be creating the device without attaching it to any bus. But I suppose qdev doesn't let you do that. Anyway, the part that I missed yesterday is that qdev_device_add() already skips creating the device if qdev_should_hide_device(), which explains how the inconsistency is created. (As an aside, it then returns NULL without setting an error to indicate success, which is an awkward interface, and sure enough, qmp_device_add() gets it wrong and deletes the QemuOpts again. So hotplugging the virtio-net standby device doesn't even seem to work?) Could we just save the configuration in the .hide_device callback (i.e. failover_hide_primary_device() in virtio-net) to a new field in VirtIONet and then use that when actually creating the device instead of accessing the command line state in the QemuOptsList? It seems that we can currently add two primary devices that are then both hidden. failover_add_primary() adds only one of them, leaving the other one hidden. Is this a bug and we should reject such a configuration or do we need to supp
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben:
>
>
> On 10/5/21 16:37, Kevin Wolf wrote:
> > Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben:
> > > Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
> > > > On 9/24/21 11:04, Kevin Wolf wrote:
> > > > > Directly call qdev_device_add_from_qdict() for QMP device_add instead
> > > > > of
> > > > > first going through QemuOpts and converting back to QDict.
> > > > >
> > > > > Note that this changes the behaviour of device_add, though in ways
> > > > > that
> > > > > should be considered bug fixes:
> > > > >
> > > > > QemuOpts ignores differences between data types, so you could
> > > > > successfully pass a string "123" for an integer property, or a string
> > > > > "on" for a boolean property (and vice versa). After this change, the
> > > > > correct data type for the property must be used in the JSON input.
> > > > >
> > > > > qemu_opts_from_qdict() also silently ignores any options whose value
> > > > > is
> > > > > a QDict, QList or QNull.
> > > > >
> > > > > To illustrate, the following QMP command was accepted before and is
> > > > > now
> > > > > rejected for both reasons:
> > > > >
> > > > > { "execute": "device_add",
> > > > > "arguments": { "driver": "scsi-cd",
> > > > >"drive": { "completely": "invalid" },
> > > > >"physical_block_size": "4096" } }
> > > > >
> > > > > Signed-off-by: Kevin Wolf
> > > > > ---
> > > > >softmmu/qdev-monitor.c | 18 +++---
> > > > >1 file changed, 11 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > > > > index c09b7430eb..8622ccade6 100644
> > > > > --- a/softmmu/qdev-monitor.c
> > > > > +++ b/softmmu/qdev-monitor.c
> > > > > @@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict
> > > > > *qdict)
> > > > >qdev_print_devinfos(true);
> > > > >}
> > > > > -void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> > > > > +static void monitor_device_add(QDict *qdict, QObject **ret_data,
> > > > > + bool from_json, Error **errp)
> > > > >{
> > > > >QemuOpts *opts;
> > > > >DeviceState *dev;
> > > > > @@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject
> > > > > **ret_data, Error **errp)
> > > > >qemu_opts_del(opts);
> > > > >return;
> > > > >}
> > > > > -dev = qdev_device_add(opts, errp);
> > > > > +qemu_opts_del(opts);
> > > > > +
> > > > > +dev = qdev_device_add_from_qdict(qdict, from_json, errp);
> > > >
> > > > Hi Kevin,
> > > >
> > > > I'm wandering if deleting the opts (which remove it from the "device"
> > > > opts
> > > > list) is really a no-op ?
> > >
> > > It's not exactly a no-op. Previously, the QemuOpts would only be freed
> > > when the device is destroying, now we delete it immediately after
> > > creating the device. This could matter in some cases.
> > >
> > > The one case I was aware of is that QemuOpts used to be responsible for
> > > checking for duplicate IDs. Obviously, it can't do this job any more
> > > when we call qemu_opts_del() right after creating the device. This is
> > > the reason for patch 6.
> > >
> > > > The opts list is, eg, traversed in hw/net/virtio-net.c in the function
> > > > failover_find_primary_device_id() which may be called during the
> > > > virtio_net_set_features() (a TYPE_VIRTIO_NET method).
> > > > I do not have the knowledge to tell when this method is called. But If
> > > > this
> > > > is after we create the devices. Then the list will be empty at this
> > > > point
> > > > now.
> > > >
> > > > It seems, there are 2 other calling sites of
&
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben:
> Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
> > On 9/24/21 11:04, Kevin Wolf wrote:
> > > Directly call qdev_device_add_from_qdict() for QMP device_add instead of
> > > first going through QemuOpts and converting back to QDict.
> > >
> > > Note that this changes the behaviour of device_add, though in ways that
> > > should be considered bug fixes:
> > >
> > > QemuOpts ignores differences between data types, so you could
> > > successfully pass a string "123" for an integer property, or a string
> > > "on" for a boolean property (and vice versa). After this change, the
> > > correct data type for the property must be used in the JSON input.
> > >
> > > qemu_opts_from_qdict() also silently ignores any options whose value is
> > > a QDict, QList or QNull.
> > >
> > > To illustrate, the following QMP command was accepted before and is now
> > > rejected for both reasons:
> > >
> > > { "execute": "device_add",
> > >"arguments": { "driver": "scsi-cd",
> > > "drive": { "completely": "invalid" },
> > > "physical_block_size": "4096" } }
> > >
> > > Signed-off-by: Kevin Wolf
> > > ---
> > > softmmu/qdev-monitor.c | 18 +++---
> > > 1 file changed, 11 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > > index c09b7430eb..8622ccade6 100644
> > > --- a/softmmu/qdev-monitor.c
> > > +++ b/softmmu/qdev-monitor.c
> > > @@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
> > > qdev_print_devinfos(true);
> > > }
> > > -void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> > > +static void monitor_device_add(QDict *qdict, QObject **ret_data,
> > > + bool from_json, Error **errp)
> > > {
> > > QemuOpts *opts;
> > > DeviceState *dev;
> > > @@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> > > Error **errp)
> > > qemu_opts_del(opts);
> > > return;
> > > }
> > > -dev = qdev_device_add(opts, errp);
> > > +qemu_opts_del(opts);
> > > +
> > > +dev = qdev_device_add_from_qdict(qdict, from_json, errp);
> >
> > Hi Kevin,
> >
> > I'm wandering if deleting the opts (which remove it from the "device" opts
> > list) is really a no-op ?
>
> It's not exactly a no-op. Previously, the QemuOpts would only be freed
> when the device is destroying, now we delete it immediately after
> creating the device. This could matter in some cases.
>
> The one case I was aware of is that QemuOpts used to be responsible for
> checking for duplicate IDs. Obviously, it can't do this job any more
> when we call qemu_opts_del() right after creating the device. This is
> the reason for patch 6.
>
> > The opts list is, eg, traversed in hw/net/virtio-net.c in the function
> > failover_find_primary_device_id() which may be called during the
> > virtio_net_set_features() (a TYPE_VIRTIO_NET method).
> > I do not have the knowledge to tell when this method is called. But If this
> > is after we create the devices. Then the list will be empty at this point
> > now.
> >
> > It seems, there are 2 other calling sites of
> > "qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c and
> > net/vhost-vdpa.c
>
> Yes, you are right. These callers probably need to be changed. Going
> through the command line options rather than looking at the actual
> device objects that exist doesn't feel entirely clean anyway.
So I tried to have a look at the virtio-net case, and ended up very
confused.
Obviously looking at command line options (even of a differrent device)
from within a device is very unclean. With a non-broken, i.e. type safe,
device-add (as well as with the JSON CLI option introduced by this
series), we can't have a QemuOpts any more that is by definition unsafe.
So this code needs a replacement.
My naive idea was that we just need to look at runtime state instead.
Don't search the options for a device with a matching 'failover_pair_id'
(which, by the way, would fail as soon as any other device introduces a
property with the same name), but search for actual PCIDevices in qdev
that have pci_dev->fa
Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()
Am 27.09.2021 um 12:33 hat Damien Hedde geschrieben: > Hi Kevin, > > I proposed a very similar patch in our rfc series because we needed some of > the cleaning you do here. > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html > I've added a bit of doc for the function, feel free to take it if you want. Thanks, I'm replacing my patch with yours for v2. Kevin
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Am 04.10.2021 um 14:18 hat Damien Hedde geschrieben: > > > On 10/1/21 16:42, Peter Krempa wrote: > > On Fri, Sep 24, 2021 at 11:04:25 +0200, Kevin Wolf wrote: > > > Directly call qdev_device_add_from_qdict() for QMP device_add instead of > > > first going through QemuOpts and converting back to QDict. > > > > > > Note that this changes the behaviour of device_add, though in ways that > > > should be considered bug fixes: > > > > > > QemuOpts ignores differences between data types, so you could > > > successfully pass a string "123" for an integer property, or a string > > > "on" for a boolean property (and vice versa). After this change, the > > > correct data type for the property must be used in the JSON input. > > > > > > qemu_opts_from_qdict() also silently ignores any options whose value is > > > a QDict, QList or QNull. > > > > Sorry for chiming in a bit late, but preferrably this commit should be > > postponed to at least the next release so that we decrease the amount of > > libvirt users broken by this. > > > > Granted users are supposed to use new libvirt with new qemu but that's > > not always the case. > > > > Anyways, libvirt is currently mangling all the types to strings with > > device_add. I'm currently working on fixing it and it will hopefully be > > done until next-month's release, but preferrably we increase the window > > of working combinations by postponing this until the next release. > > Switching to qdict is really an improvement I think. > > If we choose to keep legacy behavior working for now, I think we > should find a way to still do this switch. Maybe we can temporarily > keep the str_to_int and str_to_bool conversion when converting the > qdict to the qdev properties afterward? I guess we can keep the detour through QemuOpts for QMP for now, and make sure that the command line code bypasses this bit and still requires correct types for JSON input. It's only this patch that breaks compatibility with libvirt, patch 8 should still be okay. Kevin
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
Am 27.09.2021 um 14:52 hat Peter Maydell geschrieben: > On Mon, 27 Sept 2021 at 12:27, Kevin Wolf wrote: > > > > Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben: > > > On Fri, 24 Sept 2021 at 10:14, Kevin Wolf wrote: > > > > > > > > We want to switch both from QemuOpts to the keyval parser in the future, > > > > which results in some incompatibilities, mainly around list handling. > > > > Mark the non-JSON version of both as unstable syntax so that management > > > > tools switch to JSON and we can later make the change without breaking > > > > things. > > > > > > > > Signed-off-by: Kevin Wolf > > > > > > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > > > > +'' > > > > + > > > > +If you rely on a stable interface for ``-device`` and ``-object`` that > > > > doesn't > > > > +change incompatibly between QEMU versions (e.g. because you are using > > > > the QEMU > > > > +command line as a machine interface in scripts rather than > > > > interactively), use > > > > +JSON syntax for these options instead. > > > > + > > > > +There is no intention to remove support for non-JSON syntax entirely, > > > > but > > > > +future versions may change the way to spell some options. > > > > > > As it stands, this is basically saying "pretty much anybody > > > using the command line, your stuff may break in future, instead > > > use some other interface you've never heard of, which doesn't > > > appear to be documented in the manual and which none of the > > > documentation's examples use". > > > > The documentation is a valid criticism. We need to document the JSON > > interfaces properly (which will really mostly be a pointer to the > > existing QMP documentation at least for -object, but it's important to > > tell people where to look for the details). > > > > > Is there some more limited deprecation we can do rather than "the > > > entire commandline for almost all users" ? > > > > I don't think "almost all" users is true. > > > I see three groups of users > > ...all of whom "rely on a stable interface for -device and -object", > and only two of whom it's reasonable to say "use the JSON version" to. I'm not sure that the human interactive case requires unchanged syntax as strictly as the other cases do. After each distro upgrade (or even a browser upgrade within the same distro), some UI changes somewhere and I have to adapt. I don't think anyone ever makes promises like "this button is going to stay in the exact same place forever". And our policy is already that we're not making such promises for HMP either. > > 1. Using a management tool that is probably using libvirt. This is > >likely the vast majority of users. They won't notice a difference > >because libvirt abstracts it away. libvirt developers are actively > >asking us for JSON (and QAPI) based interfaces because using the same > >representation to describe configurations in QMP and on the CLI makes > >their life easier. > > Yes, absolutely we should be recommending that libvirt and > other management interfaces use the JSON. > > > 2. People starting QEMU on the command line manually. This is > >essentially the same situation as HMP: If something changes, you get > >an error message, you look up the new syntax, done. Small > >inconvenience, but that's it. This includes simple scripts that just > >start QEMU and are only used to store a long command line somewhere. > > It's a small inconvenience that we seem to be imposing on our > users on a pretty frequent basis. Moreover, each one of these > really needs to be its own deprecation, so that users actually > can have some advance notice if they need it and look up the > new syntax. We shouldn't hide them all under this umbrella > "we might break anything at any time" entry, which I think > will pretty much encourage breaking compatibility more often > because you don't have to think about "oh, we should deprecate > this and maybe print warnings about use of deprecated syntax > and then drop it later", you can just break things and point > to this "we said -device wasn't going to be stable" entry. Are you suggesting bringing back stricter compatibility rules for HMP then? The problem with the deprecation period is that you need to have a time wher
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
> On 9/24/21 11:04, Kevin Wolf wrote:
> > Directly call qdev_device_add_from_qdict() for QMP device_add instead of
> > first going through QemuOpts and converting back to QDict.
> >
> > Note that this changes the behaviour of device_add, though in ways that
> > should be considered bug fixes:
> >
> > QemuOpts ignores differences between data types, so you could
> > successfully pass a string "123" for an integer property, or a string
> > "on" for a boolean property (and vice versa). After this change, the
> > correct data type for the property must be used in the JSON input.
> >
> > qemu_opts_from_qdict() also silently ignores any options whose value is
> > a QDict, QList or QNull.
> >
> > To illustrate, the following QMP command was accepted before and is now
> > rejected for both reasons:
> >
> > { "execute": "device_add",
> >"arguments": { "driver": "scsi-cd",
> > "drive": { "completely": "invalid" },
> > "physical_block_size": "4096" } }
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > softmmu/qdev-monitor.c | 18 +++---
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index c09b7430eb..8622ccade6 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
> > qdev_print_devinfos(true);
> > }
> > -void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> > +static void monitor_device_add(QDict *qdict, QObject **ret_data,
> > + bool from_json, Error **errp)
> > {
> > QemuOpts *opts;
> > DeviceState *dev;
> > @@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> > Error **errp)
> > qemu_opts_del(opts);
> > return;
> > }
> > -dev = qdev_device_add(opts, errp);
> > +qemu_opts_del(opts);
> > +
> > +dev = qdev_device_add_from_qdict(qdict, from_json, errp);
>
> Hi Kevin,
>
> I'm wandering if deleting the opts (which remove it from the "device" opts
> list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed
when the device is destroying, now we delete it immediately after
creating the device. This could matter in some cases.
The one case I was aware of is that QemuOpts used to be responsible for
checking for duplicate IDs. Obviously, it can't do this job any more
when we call qemu_opts_del() right after creating the device. This is
the reason for patch 6.
> The opts list is, eg, traversed in hw/net/virtio-net.c in the function
> failover_find_primary_device_id() which may be called during the
> virtio_net_set_features() (a TYPE_VIRTIO_NET method).
> I do not have the knowledge to tell when this method is called. But If this
> is after we create the devices. Then the list will be empty at this point
> now.
>
> It seems, there are 2 other calling sites of
> "qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c and
> net/vhost-vdpa.c
Yes, you are right. These callers probably need to be changed. Going
through the command line options rather than looking at the actual
device objects that exist doesn't feel entirely clean anyway.
Kevin
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben: > On Fri, 24 Sept 2021 at 10:14, Kevin Wolf wrote: > > > > We want to switch both from QemuOpts to the keyval parser in the future, > > which results in some incompatibilities, mainly around list handling. > > Mark the non-JSON version of both as unstable syntax so that management > > tools switch to JSON and we can later make the change without breaking > > things. > > > > Signed-off-by: Kevin Wolf > > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > > +'' > > + > > +If you rely on a stable interface for ``-device`` and ``-object`` that > > doesn't > > +change incompatibly between QEMU versions (e.g. because you are using the > > QEMU > > +command line as a machine interface in scripts rather than interactively), > > use > > +JSON syntax for these options instead. > > + > > +There is no intention to remove support for non-JSON syntax entirely, but > > +future versions may change the way to spell some options. > > As it stands, this is basically saying "pretty much anybody > using the command line, your stuff may break in future, instead > use some other interface you've never heard of, which doesn't > appear to be documented in the manual and which none of the > documentation's examples use". The documentation is a valid criticism. We need to document the JSON interfaces properly (which will really mostly be a pointer to the existing QMP documentation at least for -object, but it's important to tell people where to look for the details). > Is there some more limited deprecation we can do rather than "the > entire commandline for almost all users" ? I don't think "almost all" users is true. I see three groups of users: 1. Using a management tool that is probably using libvirt. This is likely the vast majority of users. They won't notice a difference because libvirt abstracts it away. libvirt developers are actively asking us for JSON (and QAPI) based interfaces because using the same representation to describe configurations in QMP and on the CLI makes their life easier. 2. People starting QEMU on the command line manually. This is essentially the same situation as HMP: If something changes, you get an error message, you look up the new syntax, done. Small inconvenience, but that's it. This includes simple scripts that just start QEMU and are only used to store a long command line somewhere. 3. People writing more complex scripts or applications that invoke QEMU manually instead of using libvirt. They may actually be hurt by such changes. They should probably be using a proper machine interface, i.e. JSON mode, so the deprecation notice is for them to change their code. This is probably a small minority and not "almost all users". Yes, we could in theory do a more limited deprecation. The planned change from my side is just going from QemuOpts to the keyval parser, which doesn't change anything in the vast majority of cases. But we have the separation in the monitor between QMP and HMP for a good reason: Requirements for a good machine interface are different from a good human interface. The same is true for the command line. So it seems to make a lot of sense to me to have both a machine interface (JSON based) and a human interface (non-JSON) on the command line, too, and take the same liberties for evolving the human interface as we already do in HMP - which means that it's technically an unstable interface where compatibility doesn't prevent improvements, but not that it looks completely different in every QEMU version. Kevin
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
Am 27.09.2021 um 10:21 hat Daniel P. Berrangé geschrieben: > On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote: > > On 24/09/21 11:04, Kevin Wolf wrote: > > > We want to switch both from QemuOpts to the keyval parser in the future, > > > which results in some incompatibilities, mainly around list handling. > > > Mark the non-JSON version of both as unstable syntax so that management > > > tools switch to JSON and we can later make the change without breaking > > > things. > > > > Maybe we need a different section for unstable syntaxes, rather than > > overloading deprecated.rst? > > This case feels like it hits two scenarios - we want to declare it > unstable, which is something we should document in qemu-options.hx. Actually, I think a section for unstable syntaxes or generally compatibility promises wouldn't hurt. When I checked, I couldn't find any documentation about the support status of HMP either. Basically, I imagine HMP and non-JSON -device/-object would be on the same level: We don't change things without a reason, but if we do want to change things, compatibility isn't an argument against making the change. > We want to also to warn of specific breakage when the impl changes > which is something suitable for deprecations. We don't do this for HMP either for individual changes. Basically this deprecation notice was meant to make people aware that we're lowering the support status from a long-term stable interface to HMP-like. Kevin
Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts
Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.09.2021 12:04, Kevin Wolf wrote:
> > DeviceState.id is a pointer to a string that is stored in the QemuOpts
> > object DeviceState.opts and freed together with it. We want to create
> > devices without going through QemuOpts in the future, so make this a
> > separately allocated string.
> >
> > Signed-off-by: Kevin Wolf
>
> Interesting that in hw/xen/xen-legacy-backend.c
> g_strdup_printf-allocated id is passed to qdev_set_id prior this
> patch. So, the patch seems to fix that small leak. Worth to mention?
Ok, I can mention it explicitly.
> > ---
> > include/hw/qdev-core.h | 2 +-
> > include/monitor/qdev.h | 2 +-
> > hw/arm/virt.c | 2 +-
> > hw/core/qdev.c | 1 +
> > hw/pci-bridge/pci_expander_bridge.c | 2 +-
> > hw/ppc/e500.c | 2 +-
> > softmmu/qdev-monitor.c | 5 +++--
> > 7 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 34c8a7506a..1857d9698e 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -176,7 +176,7 @@ struct DeviceState {
> > Object parent_obj;
> > /*< public >*/
> > -const char *id;
> > +char *id;
> > char *canonical_path;
> > bool realized;
> > bool pending_deleted_event;
> > diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> > index eaa947d73a..389287eb44 100644
> > --- a/include/monitor/qdev.h
> > +++ b/include/monitor/qdev.h
> > @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
> > **errp);
> > int qdev_device_help(QemuOpts *opts);
> > DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
> > -void qdev_set_id(DeviceState *dev, const char *id);
> > +void qdev_set_id(DeviceState *dev, char *id);
> > #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 1d59f0e59f..f933d48d3b 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
> > MemoryRegion *sysmem = get_system_memory();
> > dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> > -dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > +dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> > qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
> > qdev_prop_set_uint32(dev, "mmio_size",
> > vms->memmap[VIRT_PLATFORM_BUS].size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index cefc5eaa0a..d918b50a1d 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
> > }
> > qemu_opts_del(dev->opts);
> > +g_free(dev->id);
> > }
> > static void device_class_base_init(ObjectClass *class, void *data)
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 7112dc3062..10e6e7c2ab 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
> > pcie, Error **errp)
> > } else {
> > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
> > TYPE_PXB_BUS);
> > bds = qdev_new("pci-bridge");
> > -bds->id = dev_name;
> > +bds->id = g_strdup(dev_name);
> > qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR,
> > pxb->bus_nr);
> > qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
> > }
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index 95451414dd..960e7efcd3 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
> > /* Platform Bus Device */
> > if (pmc->has_platform_bus) {
> > dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> > -dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > +dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> > qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
> > qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
> >
[PATCH 03/11] iotests/051: Fix typo
The iothread isn't called 'iothread0', but 'thread0'. Depending on the order that properties are parsed, the error message may change from the expected one to another one saying that the iothread doesn't exist. Signed-off-by: Kevin Wolf --- tests/qemu-iotests/051| 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 7bf29343d7..1d2fa93a11 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -199,7 +199,7 @@ case "$QEMU_DEFAULT_MACHINE" in # virtio-blk enables the iothread only when the driver initialises the # device, so a second virtio-blk device can't be added even with the # same iothread. virtio-scsi allows this. -run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on run_qemu $iothread -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on ;; *) diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index afe7632964..063e4fc584 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -183,9 +183,9 @@ Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id QEMU X.Y.Z monitor - type 'help' for more information (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: Cannot change iothread of active block backend -Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change iothread of active block backend +(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -- 2.31.1
[PATCH 00/11] qdev: Add JSON -device and fix QMP device_add
It's still a long way until we'll have QAPIfied devices, but there are some improvements that we can already make now to make the future switch easier. One important part of this is having code paths without QemuOpts, which we want to get rid of and replace with the keyval parser in the long run. This series adds support for JSON syntax to -device, which bypasses QemuOpts. While we're not using QAPI yet, devices are based on QOM, so we already do have type checks and an implied schema. JSON syntax supported now can be supported by QAPI later and regarding command line compatibility, actually switching to it becomes an implementation detail this way (of course, it will still add valuable user-visible features like introspection and documentation). Apart from making things more future proof, this also immediately adds a way to do non-scalar properties on the command line. nvme could have used list support recently, and the lack of it in -device led to some rather unnatural solution in the first version (doing the relationship between a device and objects backwards) and loss of features in the following. With this series, using a list as a device property should be possible without any weird tricks. Unfortunately, even QMP device_add goes through QemuOpts before this series, which destroys any type safety QOM provides and also can't support non-scalar properties. This is a bug that is fixed during this series, but that is technically an incompatible change. A similar change was made for netdev_add in commit db2a380c84. libvirt needs to make sure that it actually always passes the right type, because passing a wrong type will start to fail after this series. I hope that libvirt already does things correctly, so this won't cause any trouble, but if it does, there is the option of using the QemuOpts-less code path only for JSON -device and going through a deprecation period for QMP device_add. Kevin Wolf (11): qom: Reduce use of error_propagate() iotests/245: Fix type for iothread property iotests/051: Fix typo qdev: Avoid using string visitor for properties qdev: Make DeviceState.id independent of QemuOpts qdev: Add Error parameter to qdev_set_id() qemu-option: Allow deleting opts during qemu_opts_foreach() qdev: Base object creation on QDict rather than QemuOpts qdev: Avoid QemuOpts in QMP device_add vl: Enable JSON syntax for -device Deprecate stable non-JSON -device and -object qapi/qdev.json | 15 +++-- docs/about/deprecated.rst | 11 include/hw/qdev-core.h | 10 ++-- include/monitor/qdev.h | 2 +- hw/arm/virt.c | 2 +- hw/core/qdev.c | 6 +- hw/net/virtio-net.c | 4 +- hw/pci-bridge/pci_expander_bridge.c | 2 +- hw/ppc/e500.c | 2 +- hw/vfio/pci.c | 4 +- hw/xen/xen-legacy-backend.c | 3 +- qom/object.c| 7 +-- qom/object_interfaces.c | 17 ++ softmmu/qdev-monitor.c | 90 + softmmu/vl.c| 58 --- util/qemu-option.c | 4 +- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.pc.out | 4 +- tests/qemu-iotests/245 | 4 +- 19 files changed, 161 insertions(+), 86 deletions(-) -- 2.31.1
[PATCH 06/11] qdev: Add Error parameter to qdev_set_id()
object_property_add_child() fails (with _abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.
Signed-off-by: Kevin Wolf
---
include/monitor/qdev.h | 2 +-
hw/xen/xen-legacy-backend.c | 3 ++-
softmmu/qdev-monitor.c | 16 ++--
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char
*type, int dom,
xendev = g_malloc0(ops->size);
object_initialize(>qdev, ops->size, TYPE_XENBACKEND);
OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+_abort);
qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
}
/* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
{
if (id) {
dev->id = id;
}
if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
- OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral(), dev->id,
+ OBJECT(dev), errp);
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
- OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev), errp);
g_free(name);
}
}
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
{
+ERRP_GUARD();
DeviceClass *dc;
const char *driver, *path;
DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+if (*errp) {
+goto err_del_dev;
+}
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
2.31.1
[PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts
DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.
Signed-off-by: Kevin Wolf
---
include/hw/qdev-core.h | 2 +-
include/monitor/qdev.h | 2 +-
hw/arm/virt.c | 2 +-
hw/core/qdev.c | 1 +
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/ppc/e500.c | 2 +-
softmmu/qdev-monitor.c | 5 +++--
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 34c8a7506a..1857d9698e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
Object parent_obj;
/*< public >*/
-const char *id;
+char *id;
char *canonical_path;
bool realized;
bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
#endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1d59f0e59f..f933d48d3b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
MemoryRegion *sysmem = get_system_memory();
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
qdev_prop_set_uint32(dev, "mmio_size",
vms->memmap[VIRT_PLATFORM_BUS].size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
}
qemu_opts_del(dev->opts);
+g_free(dev->id);
}
static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
pcie, Error **errp)
} else {
bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
TYPE_PXB_BUS);
bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
/* Platform Bus Device */
if (pmc->has_platform_bus) {
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
return bus;
}
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
{
if (id) {
dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
2.31.1
[PATCH 04/11] qdev: Avoid using string visitor for properties
The only thing the string visitor adds compared to a keyval visitor is
list support. git grep for 'visit_start_list' and 'visit.*List' shows
that devices don't make use of this.
In a world with a QAPIfied command line interface, the keyval visitor is
used to parse the command line. In order to make sure that no devices
start using this feature that would make backwards compatibility harder,
just switch away from object_property_parse(), which internally uses the
string visitor, to a keyval visitor and object_property_set().
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f00846..034b999401 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -28,6 +28,8 @@
#include "qapi/qmp/dispatch.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
#include "qemu/help_option.h"
@@ -198,16 +200,28 @@ static int set_property(void *opaque, const char *name,
const char *value,
Error **errp)
{
Object *obj = opaque;
+QString *val;
+Visitor *v;
+int ret;
if (strcmp(name, "driver") == 0)
return 0;
if (strcmp(name, "bus") == 0)
return 0;
-if (!object_property_parse(obj, name, value, errp)) {
-return -1;
+val = qstring_from_str(value);
+v = qobject_input_visitor_new_keyval(QOBJECT(val));
+
+if (!object_property_set(obj, name, v, errp)) {
+ret = -1;
+goto out;
}
-return 0;
+
+ret = 0;
+out:
+visit_free(v);
+qobject_unref(val);
+return ret;
}
static const char *find_typename_by_alias(const char *alias)
--
2.31.1
[PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Directly call qdev_device_add_from_qdict() for QMP device_add instead of
first going through QemuOpts and converting back to QDict.
Note that this changes the behaviour of device_add, though in ways that
should be considered bug fixes:
QemuOpts ignores differences between data types, so you could
successfully pass a string "123" for an integer property, or a string
"on" for a boolean property (and vice versa). After this change, the
correct data type for the property must be used in the JSON input.
qemu_opts_from_qdict() also silently ignores any options whose value is
a QDict, QList or QNull.
To illustrate, the following QMP command was accepted before and is now
rejected for both reasons:
{ "execute": "device_add",
"arguments": { "driver": "scsi-cd",
"drive": { "completely": "invalid" },
"physical_block_size": "4096" } }
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c09b7430eb..8622ccade6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
qdev_print_devinfos(true);
}
-void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+static void monitor_device_add(QDict *qdict, QObject **ret_data,
+ bool from_json, Error **errp)
{
QemuOpts *opts;
DeviceState *dev;
@@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp)
qemu_opts_del(opts);
return;
}
-dev = qdev_device_add(opts, errp);
+qemu_opts_del(opts);
+
+dev = qdev_device_add_from_qdict(qdict, from_json, errp);
/*
* Drain all pending RCU callbacks. This is done because
@@ -838,13 +841,14 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
Error **errp)
*/
drain_call_rcu();
-if (!dev) {
-qemu_opts_del(opts);
-return;
-}
object_unref(OBJECT(dev));
}
+void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+{
+monitor_device_add(qdict, ret_data, true, errp);
+}
+
static DeviceState *find_device_state(const char *id, Error **errp)
{
Object *obj;
@@ -936,7 +940,7 @@ void hmp_device_add(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
-qmp_device_add((QDict *)qdict, NULL, );
+monitor_device_add((QDict *)qdict, NULL, false, );
hmp_handle_error(mon, err);
}
--
2.31.1
[PATCH 11/11] Deprecate stable non-JSON -device and -object
We want to switch both from QemuOpts to the keyval parser in the future, which results in some incompatibilities, mainly around list handling. Mark the non-JSON version of both as unstable syntax so that management tools switch to JSON and we can later make the change without breaking things. Signed-off-by: Kevin Wolf --- docs/about/deprecated.rst | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 3c2be84d80..42f6a478fb 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -160,6 +160,17 @@ Use ``-display sdl`` instead. Use ``-display curses`` instead. +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) +'' + +If you rely on a stable interface for ``-device`` and ``-object`` that doesn't +change incompatibly between QEMU versions (e.g. because you are using the QEMU +command line as a machine interface in scripts rather than interactively), use +JSON syntax for these options instead. + +There is no intention to remove support for non-JSON syntax entirely, but +future versions may change the way to spell some options. + Plugin argument passing through ``arg=`` (since 6.1) -- 2.31.1
[PATCH 10/11] vl: Enable JSON syntax for -device
Like we already do for -object, introduce support for JSON syntax in
-device, which can be kept stable in the long term and guarantees that a
single code path with identical behaviour is used for both QMP and the
command line. Compared to the QemuOpts based code, the parser contains
less surprises and has support for non-scalar options (lists and
structs). Switching management tools to JSON means that we can more
easily change the "human" CLI syntax from QemuOpts to the keyval parser
later.
In the QAPI schema, a feature flag is added to the device-add command to
allow management tools to detect support for this.
Signed-off-by: Kevin Wolf
---
qapi/qdev.json | 15 +
softmmu/vl.c | 58 --
2 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/qapi/qdev.json b/qapi/qdev.json
index b83178220b..cdc8f911b5 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -32,17 +32,23 @@
##
# @device_add:
#
+# Add a device.
+#
# @driver: the name of the new device's driver
#
# @bus: the device's parent bus (device tree path)
#
# @id: the device's ID, must be unique
#
-# Additional arguments depend on the type.
-#
-# Add a device.
+# Features:
+# @json-cli: If present, the "-device" command line option supports JSON
+#syntax with a structure identical to the arguments of this
+#command.
#
# Notes:
+#
+# Additional arguments depend on the type.
+#
# 1. For detailed information about this command, please refer to the
#'docs/qdev-device-use.txt' file.
#
@@ -67,7 +73,8 @@
##
{ 'command': 'device_add',
'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
- 'gen': false } # so we can get the additional arguments
+ 'gen': false, # so we can get the additional arguments
+ 'features': ['json-cli'] }
##
# @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97..7596d9da06 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -144,6 +144,12 @@ typedef struct ObjectOption {
QTAILQ_ENTRY(ObjectOption) next;
} ObjectOption;
+typedef struct DeviceOption {
+QDict *opts;
+Location loc;
+QTAILQ_ENTRY(DeviceOption) next;
+} DeviceOption;
+
static const char *cpu_option;
static const char *mem_path;
static const char *incoming;
@@ -151,6 +157,7 @@ static const char *loadvm;
static const char *accelerators;
static QDict *machine_opts_dict;
static QTAILQ_HEAD(, ObjectOption) object_opts =
QTAILQ_HEAD_INITIALIZER(object_opts);
+static QTAILQ_HEAD(, DeviceOption) device_opts =
QTAILQ_HEAD_INITIALIZER(device_opts);
static ram_addr_t maxram_size;
static uint64_t ram_slots;
static int display_remote;
@@ -494,21 +501,39 @@ const char *qemu_get_vm_name(void)
return qemu_name;
}
-static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+static void default_driver_disable(const char *driver)
{
-const char *driver = qemu_opt_get(opts, "driver");
int i;
-if (!driver)
-return 0;
+if (!driver) {
+return;
+}
+
for (i = 0; i < ARRAY_SIZE(default_list); i++) {
if (strcmp(default_list[i].driver, driver) != 0)
continue;
*(default_list[i].flag) = 0;
}
+}
+
+static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *driver = qemu_opt_get(opts, "driver");
+
+default_driver_disable(driver);
return 0;
}
+static void default_driver_check_json(void)
+{
+DeviceOption *opt;
+
+QTAILQ_FOREACH(opt, _opts, next) {
+const char *driver = qdict_get_try_str(opt->opts, "driver");
+default_driver_disable(driver);
+}
+}
+
static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
{
const char *proc_name;
@@ -1311,6 +1336,7 @@ static void qemu_disable_default_devices(void)
{
MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
+default_driver_check_json();
qemu_opts_foreach(qemu_find_opts("device"),
default_driver_check, NULL, NULL);
qemu_opts_foreach(qemu_find_opts("global"),
@@ -2637,6 +2663,8 @@ static void qemu_init_board(void)
static void qemu_create_cli_devices(void)
{
+DeviceOption *opt;
+
soundhw_init();
qemu_opts_foreach(qemu_find_opts("fw_cfg"),
@@ -2652,6 +2680,13 @@ static void qemu_create_cli_devices(void)
rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
qemu_opts_foreach(qemu_find_opts("device"),
device_init_func, NULL, _fatal);
+QTAILQ_FOREACH(opt, _opts, next) {
+QObject *ret_data;
+
+loc_push_restore(>loc);
+qmp_device_add(opt->opts, _data, _fatal);
+loc_pop(>loc);
+}
rom_reset_order_override();
}
@@ -3352,9 +3387,18 @@ void qemu_init(int argc, char **argv, char **envp)
add_device_config(DEV_USB, optarg);
[PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts
QDicts are both what QMP natively uses and what the keyval parser
produces. Going through QemuOpts isn't useful for either one, so switch
the main device creation function to QDicts. By sharing more code with
the -object/object-add code path, we can even reduce the code size a
bit.
This commit doesn't remove the detour through QemuOpts from any code
path yet, but it allows the following commits to do so.
Signed-off-by: Kevin Wolf
---
include/hw/qdev-core.h | 8 ++---
hw/core/qdev.c | 5 ++--
hw/net/virtio-net.c| 4 +--
hw/vfio/pci.c | 4 +--
softmmu/qdev-monitor.c | 67 +++---
5 files changed, 41 insertions(+), 47 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1857d9698e..5b3d4704a5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,7 +180,7 @@ struct DeviceState {
char *canonical_path;
bool realized;
bool pending_deleted_event;
-QemuOpts *opts;
+QDict *opts;
int hotplugged;
bool allow_unplug_during_migration;
BusState *parent_bus;
@@ -202,7 +202,7 @@ struct DeviceListener {
* hide a failover device depending for example on the device
* opts.
*/
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts);
+bool (*hide_device)(DeviceListener *listener, const QDict *device_opts);
QTAILQ_ENTRY(DeviceListener) link;
};
@@ -831,13 +831,13 @@ void device_listener_unregister(DeviceListener *listener);
/**
* @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict
*
* Check if a device should be added.
* When a device is added via qdev_device_add() this will be called,
* and return if the device should be added now or not.
*/
-bool qdev_should_hide_device(QemuOpts *opts);
+bool qdev_should_hide_device(const QDict *opts);
typedef enum MachineInitPhase {
/* current_machine is NULL. */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d918b50a1d..5b889866c5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -28,6 +28,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qapi/qapi-events-qdev.h"
+#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
@@ -211,7 +212,7 @@ void device_listener_unregister(DeviceListener *listener)
QTAILQ_REMOVE(_listeners, listener, link);
}
-bool qdev_should_hide_device(QemuOpts *opts)
+bool qdev_should_hide_device(const QDict *opts)
{
DeviceListener *listener;
@@ -955,7 +956,7 @@ static void device_finalize(Object *obj)
dev->canonical_path = NULL;
}
-qemu_opts_del(dev->opts);
+qobject_unref(dev->opts);
g_free(dev->id);
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..5684c2b2b7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3304,7 +3304,7 @@ static void virtio_net_migration_state_notifier(Notifier
*notifier, void *data)
}
static bool failover_hide_primary_device(DeviceListener *listener,
- QemuOpts *device_opts)
+ const QDict *device_opts)
{
VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
const char *standby_id;
@@ -3312,7 +3312,7 @@ static bool failover_hide_primary_device(DeviceListener
*listener,
if (!device_opts) {
return false;
}
-standby_id = qemu_opt_get(device_opts, "failover_pair_id");
+standby_id = qdict_get_try_str(device_opts, "failover_pair_id");
if (g_strcmp0(standby_id, n->netclient_name) != 0) {
return false;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4feaa1cb68..5cdf1d4298 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -29,10 +29,10 @@
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
#include "migration/vmstate.h"
+#include "qapi/qmp/qdict.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
#include "qemu/module.h"
-#include "qemu/option.h"
#include "qemu/range.h"
#include "qemu/units.h"
#include "sysemu/kvm.h"
@@ -941,7 +941,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
}
if (vfio_opt_rom_in_denylist(vdev)) {
-if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
+if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
warn_report("Device at %s is known to cause system instability"
" issues during option rom execution",
vdev->vbasedev.name);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c2af906df0..c09b7430eb 100644
--- a/softmmu/qde
[PATCH 07/11] qemu-option: Allow deleting opts during qemu_opts_foreach()
Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted
while iterating through the whole list.
Signed-off-by: Kevin Wolf
---
util/qemu-option.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 61cb4a97bd..eedd08929b 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1126,11 +1126,11 @@ int qemu_opts_foreach(QemuOptsList *list,
qemu_opts_loopfunc func,
void *opaque, Error **errp)
{
Location loc;
-QemuOpts *opts;
+QemuOpts *opts, *next;
int rc = 0;
loc_push_none();
-QTAILQ_FOREACH(opts, >head, next) {
+QTAILQ_FOREACH_SAFE(opts, >head, next, next) {
loc_restore(>loc);
rc = func(opaque, opts, errp);
if (rc) {
--
2.31.1
[PATCH 02/11] iotests/245: Fix type for iothread property
iothread is a string property, so None (= JSON null) is not a valid
value for it. Pass the empty string instead to get the default iothread.
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/245 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bf8261eec0..9b12b42eed 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1189,10 +1189,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.run_test_iothreads('iothread0', 'iothread0')
def test_iothreads_switch_backing(self):
-self.run_test_iothreads('iothread0', None)
+self.run_test_iothreads('iothread0', '')
def test_iothreads_switch_overlay(self):
-self.run_test_iothreads(None, 'iothread0')
+self.run_test_iothreads('', 'iothread0')
if __name__ == '__main__':
iotests.activate_logging()
--
2.31.1
[PATCH 01/11] qom: Reduce use of error_propagate()
ERRP_GUARD() makes debugging easier by making sure that _abort
still fails at the real origin of the error instead of
error_propagate().
Signed-off-by: Kevin Wolf
---
qom/object.c| 7 +++
qom/object_interfaces.c | 17 ++---
2 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name,
Visitor *v,
bool object_property_set(Object *obj, const char *name, Visitor *v,
Error **errp)
{
-Error *err = NULL;
+ERRP_GUARD();
ObjectProperty *prop = object_property_find_err(obj, name, errp);
if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name,
Visitor *v,
error_setg(errp, QERR_PERMISSION_DENIED);
return false;
}
-prop->set(obj, v, name, prop->opaque, );
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
}
bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..80691e88cd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
Visitor *v, Error **errp)
{
+ERRP_GUARD();
const QDictEntry *e;
-Error *local_err = NULL;
-if (!visit_start_struct(v, NULL, NULL, 0, _err)) {
-goto out;
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+return;
}
for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, _err)) {
+if (!object_property_set(obj, e->key, v, errp)) {
break;
}
}
-if (!local_err) {
-visit_check_struct(v, _err);
+if (!*errp) {
+visit_check_struct(v, errp);
}
visit_end_struct(v, NULL);
-
-out:
-if (local_err) {
-error_propagate(errp, local_err);
-}
}
void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
--
2.31.1
Re: [PATCH for-6.1? v2 5/7] job: Add job_cancel_requested()
[ Peter, the question for you is at the end. ] Am 04.08.2021 um 10:07 hat Max Reitz geschrieben: > On 03.08.21 16:25, Kevin Wolf wrote: > > Am 26.07.2021 um 16:46 hat Max Reitz geschrieben: > > > Most callers of job_is_cancelled() actually want to know whether the job > > > is on its way to immediate termination. For example, we refuse to pause > > > jobs that are cancelled; but this only makes sense for jobs that are > > > really actually cancelled. > > > > > > A mirror job that is cancelled during READY with force=false should > > > absolutely be allowed to pause. This "cancellation" (which is actually > > > a kind of completion) may take an indefinite amount of time, and so > > > should behave like any job during normal operation. For example, with > > > on-target-error=stop, the job should stop on write errors. (In > > > contrast, force-cancelled jobs should not get write errors, as they > > > should just terminate and not do further I/O.) > > > > > > Therefore, redefine job_is_cancelled() to only return true for jobs that > > > are force-cancelled (which as of HEAD^ means any job that interprets the > > > cancellation request as a request for immediate termination), and add > > > job_cancel_request() as the general variant, which returns true for any > > > jobs which have been requested to be cancelled, whether it be > > > immediately or after an arbitrarily long completion phase. > > > > > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462 > > > Signed-off-by: Max Reitz > > > --- > > > include/qemu/job.h | 8 +++- > > > block/mirror.c | 10 -- > > > job.c | 7 ++- > > > 3 files changed, 17 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/qemu/job.h b/include/qemu/job.h > > > index 8aa90f7395..032edf3c5f 100644 > > > --- a/include/qemu/job.h > > > +++ b/include/qemu/job.h > > > @@ -436,9 +436,15 @@ const char *job_type_str(const Job *job); > > > /** Returns true if the job should not be visible to the management > > > layer. */ > > > bool job_is_internal(Job *job); > > > -/** Returns whether the job is scheduled for cancellation. */ > > > +/** Returns whether the job is being cancelled. */ > > > bool job_is_cancelled(Job *job); > > > +/** > > > + * Returns whether the job is scheduled for cancellation (at an > > > + * indefinite point). > > > + */ > > > +bool job_cancel_requested(Job *job); > > I don't think non-force blockdev-cancel for mirror should actually be > > considered cancellation, so what is the question that this function > > answers? > > > > "Is this a cancelled job, or a mirror block job that is supposed to > > complete soon, but only if it doesn't switch over the users to the > > target on completion"? > > Well, technically yes, but it was more intended as “Has the user ever > invoked (block-)job-cancel on this job?”. I understand this, but is this much more useful to know than "Has the user ever called HMP 'change'?", if you know what I mean? > > Is this ever a reasonable question to ask, except maybe inside the > > mirror implementation itself? > > I asked myself the same for v3, but found two places in job.c where I > would like to keep it: > > First, there’s an assertion in job_completed_txn_abort(). All jobs > other than @job have been force-cancelled, and so job_is_cancelled() > would be true for them. As for @job itself, the function is mostly > called when the job’s return value is not 0, but a soft-cancelled > mirror does have a return value of 0 and so would not end up in that > function. > But job_cancel() invokes job_completed_txn_abort() if the job has been > deferred to the main loop, which mostly correlates with the job having > been completed (in which case the assertion is skipped), but not 100 % > (there’s a small window between setting deferred_to_main_loop and the > job changing to a completed state). > So I’d prefer to keep the assertion as-is functionally, i.e. to only > check job->cancelled. Well, you don't. It's still job_is_cancelled() after this patch. So the scenario you're concerned about is a job that has just finished successfully (job->ret = 0) and then gets a cancel request? With force=false, I'm pretty sure the code is wrong anyway because calling job_completed_txn_abort() is not the right response. It should return an error because you're trying to complete twice, possibly with conflicting completion modes. Second
Re: [PATCH 3/2] qemu-img: Improve error for rebase without backing format
Am 08.07.2021 um 17:52 hat Eric Blake geschrieben: > When removeing support for qemu-img being able to create backing > chains without embedded backing formats, we caused a poor error > message as caught by iotest 114. Improve the situation to inform the > user what went wrong. > > Suggested-by: Kevin Wolf > Signed-off-by: Eric Blake > diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out > index 172454401257..016e9ce3ecfb 100644 > --- a/tests/qemu-iotests/114.out > +++ b/tests/qemu-iotests/114.out > @@ -14,7 +14,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Could not open > backing file: Unknow > no file open, try 'help open' > read 4096/4096 bytes at offset 0 > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -qemu-img: Could not change the backing file to > '/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': Invalid > argument > +qemu-img: Could not change the backing file to > '/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': backing > format must be specified > read 4096/4096 bytes at offset 0 > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > *** done Wait, there is a problem in the image path... I'm squashing in the following for patch 2, and will do the obvious conflict resolution for this one. Kevin diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114 index 3e30b402bc..de6fd327ee 100755 --- a/tests/qemu-iotests/114 +++ b/tests/qemu-iotests/114 @@ -65,7 +65,7 @@ $QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_tes $QEMU_IO -c "open -o backing.driver=$IMGFMT $TEST_IMG" -c "read 0 4k" | _filter_qemu_io # Rebase the image, to show that backing format is required. -$QEMU_IMG rebase -u -b "$TEST_IMG.base" "$TEST_IMG" && echo "unexpected pass" +($QEMU_IMG rebase -u -b "$TEST_IMG.base" "$TEST_IMG" 2>&1 && echo "unexpected pass") | _filter_testdir $QEMU_IMG rebase -u -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG" $QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_testdir diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out index 1724544012..f51dd9d20a 100644 --- a/tests/qemu-iotests/114.out +++ b/tests/qemu-iotests/114.out @@ -14,7 +14,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknow no file open, try 'help open' read 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -qemu-img: Could not change the backing file to '/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': Invalid argument +qemu-img: Could not change the backing file to 'TEST_DIR/t.qcow2.base': backing format must be specified read 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) *** done
Re: [PATCH 3/2] qemu-img: Improve error for rebase without backing format
Am 08.07.2021 um 17:52 hat Eric Blake geschrieben: > When removeing support for qemu-img being able to create backing > chains without embedded backing formats, we caused a poor error > message as caught by iotest 114. Improve the situation to inform the > user what went wrong. > > Suggested-by: Kevin Wolf > Signed-off-by: Eric Blake Thanks, applied to the block branch. Kevin
Re: [PATCH 0/2] Remove deprecated qemu-img backing file without format
Am 07.07.2021 um 23:17 hat Eric Blake geschrieben: > On Mon, May 03, 2021 at 04:35:58PM -0500, Eric Blake wrote: > > We've gone enough release cycles without noticeable pushback on our > > intentions, so time to make it harder to create images that can form a > > security hole due to a need for format probing rather than an explicit > > format. > > > > Eric Blake (2): > > qcow2: Prohibit backing file changes in 'qemu-img amend' > > qemu-img: Require -F with -b backing image > > Ping. Thanks, applied to the block branch. For some reason, the CCs were missing on the cover letter. Please make sure to CC me (and qemu-block) for the whole series if you want it to go through my tree. Kevin
Re: [PATCH 2/2] qemu-img: Require -F with -b backing image
Am 03.05.2021 um 23:36 hat Eric Blake geschrieben: > @@ -17,7 +14,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Could not open > backing file: Unknow > no file open, try 'help open' > read 4096/4096 bytes at offset 0 > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -qemu-img: warning: Deprecated use of backing file without explicit backing > format, use of this image requires potentially unsafe format probing > +qemu-img: Could not change the backing file to > '/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': Invalid > argument > read 4096/4096 bytes at offset 0 > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) This is not exactly an improvement for the error message. Maybe worth a follow-up patch? Kevin
Re: Qemu block filter insertion/removal API
Am 19.05.2021 um 14:19 hat Vladimir Sementsov-Ogievskiy geschrieben: > 19.05.2021 14:44, Kevin Wolf wrote: > > Am 17.05.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Hi all! > > > > > > I'd like to be sure that we know where we are going to. > > > > > > In blockdev-era where qemu user is aware about block nodes, all nodes > > > have good names and controlled by user we can efficiently use block > > > filters. > > > > > > We already have some useful filters: copy-on-read, throttling, compress. > > > In my parallel series I make backup-top filter public and useful without > > > backup block jobs. But now filters could be inserted only together with > > > opening their child. We can specify filters in qemu cmdline, or filter > > > can take place in the block node chain created by blockdev-add. > > > > > > Still, it would be good to insert/remove filters on demand. > > > > > > Currently we are going to use x-blockdev-reopen for this. Still it can't > > > be used to insert a filter above root node (as x-blockdev-reopen can > > > change only block node options and their children). In my series "[PATCH > > > 00/21] block: publish backup-top filter" I propose (as Kevin suggested) > > > to modify qom-set, so that it can set drive option of running device. > > > That's not difficult, but it means that we have different scenario of > > > inserting/removing filters: > > > > > > 1. filter above root node X: > > > > > > inserting: > > > > > >- do blockdev-add to add a filter (and specify X as its child) > > >- do qom-set to set new filter as a rood node instead of X > > > > > > removing > > > > > >- do qom-set to make X a root node again > > >- do blockdev-del to drop a filter > > > > > > 2. filter between two block nodes P and X. (For example, X is a backing > > > child of P) > > > > > > inserting > > > > > >- do blockdev-add to add a filter (and specify X as its child) > > >- do blockdev-reopen to set P.backing = filter > > > > > > remvoing > > > > > >- do blockdev-reopen to set P.backing = X > > >- do blockdev-del to drop a filter > > > > > > > > > And, probably we'll want transaction support for all these things. > > > > > > > > > Is it OK? Or do we need some kind of additional blockdev-replace command, > > > that can replace one node by another, so in both cases we will do > > > > > > inserting: > > >- blockdev-add filter > > >- blockdev-replace (make all parents of X to point to the new filter > > > instead (except for the filter itself of course) > > > > > > removing > > >- blockdev-replace (make all parante of filter to be parents of X > > > instead) > > >- blockdev-del filter > > > > > > It's simple to implement, and it seems for me that it is simpler to use. > > > Any thoughts? > > > > One reason I remember why we didn't decide to go this way in the many > > "dynamic graph reconfiguration" discussions we had, is that it's not > > generic enough to cover all cases. But I'm not sure if we ever > > considered root nodes as a separate case. I acknowledge that having two > > different interfaces is inconvenient, and integrating qom-set in a > > transaction is rather unlikely to happen. > > > > The reason why it's not generic is that it restricts you to doing the > > same thing for all parents. Imagine this: > > > > +- virtio-blk > > | > > file <- qcow2 <-+ > > | > > +- NBD export > > > > Now you want to throttle the NBD export so that it doesn't interfere > > with your VM too much. With your simple blockdev-replace this isn't > > possible. You would have to add the filter to both users or to none. > > > > In theory, blockdev-replace could take a list of the edges that should > > be changed to the new node. The problem is that edges don't have names, > > and even the parents don't necessarily have one (and if they do, they > > are in separate namespaces, so a BlockBackend, a job and an export could > > all have the same name), so finding a good way to refer to them in QMP > > doesn't sound trivial. > > > &
Re: RFC: Qemu backup interface plans
Am 19.05.2021 um 13:49 hat Vladimir Sementsov-Ogievskiy geschrieben: > 19.05.2021 14:20, Kevin Wolf wrote: > > Am 19.05.2021 um 08:11 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > > 2. Test, that we can start backup job with source = (target of > > > > > backup-top filter), so that we have "push backup with fleecing". > > > > > Make an option for backup to start without a filter, when we don't > > > > > need copy-before-write operations, to not create extra superfluous > > > > > filter. > > > > > > > > OK, so the backup job is not really a backup job, but just anything > > > > that copies data. > > > > > > Not quite. For backup without a filter we should protect source from > > > changing, by unsharing WRITE permission on it. > > > > > > I'll try to avoid adding an option. The logic should work like in > > > commit job: if there are current writers on source we create filter. > > > If there no writers, we just unshare writes and go without a filter. > > > And for this copy-before-write filter should be able to do > > > WRITE_UNCHANGED in case of fleecing. > > > > If we ever get to the point where we would make a block-copy job visible > > to the user, I wouldn't copy the restrictions from the current jobs, but > > keep it really generic to cover all cases. > > > > There is no way for the QMP command starting the job to know what the > > user is planning to do with the image in the future. Even if it's > > currently read-only, the user may want to add a writer later. > > > > I think this means that we want to always add a filter node, and then > > possibly dynamically switch between modes if being read-only provides a > > significant advantage for the job. > > Still, in push-backup-with-fleecing scheme we really don't need the > second filter, so why to insert extra thing to block graph? > > I see your point still, that user may want to add writer later. Still, > I'd be surprised if such use-cases exist now. > > What about the following: > > add some source-mode tri-state parameter for backup: > > auto: insert filter iff there are existing writers [default] > filtered: insert filter unconditionally > immutable: don't insert filter. will fail if there are existing > writers, and creating writers during block-job would be impossible Yes, that's an option, too. Kevin
Re: Qemu block filter insertion/removal API
Am 17.05.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > Hi all! > > I'd like to be sure that we know where we are going to. > > In blockdev-era where qemu user is aware about block nodes, all nodes have > good names and controlled by user we can efficiently use block filters. > > We already have some useful filters: copy-on-read, throttling, compress. In > my parallel series I make backup-top filter public and useful without backup > block jobs. But now filters could be inserted only together with opening > their child. We can specify filters in qemu cmdline, or filter can take place > in the block node chain created by blockdev-add. > > Still, it would be good to insert/remove filters on demand. > > Currently we are going to use x-blockdev-reopen for this. Still it can't be > used to insert a filter above root node (as x-blockdev-reopen can change only > block node options and their children). In my series "[PATCH 00/21] block: > publish backup-top filter" I propose (as Kevin suggested) to modify qom-set, > so that it can set drive option of running device. That's not difficult, but > it means that we have different scenario of inserting/removing filters: > > 1. filter above root node X: > > inserting: > > - do blockdev-add to add a filter (and specify X as its child) > - do qom-set to set new filter as a rood node instead of X > > removing > > - do qom-set to make X a root node again > - do blockdev-del to drop a filter > > 2. filter between two block nodes P and X. (For example, X is a backing child > of P) > > inserting > > - do blockdev-add to add a filter (and specify X as its child) > - do blockdev-reopen to set P.backing = filter > > remvoing > > - do blockdev-reopen to set P.backing = X > - do blockdev-del to drop a filter > > > And, probably we'll want transaction support for all these things. > > > Is it OK? Or do we need some kind of additional blockdev-replace command, > that can replace one node by another, so in both cases we will do > > inserting: > - blockdev-add filter > - blockdev-replace (make all parents of X to point to the new filter > instead (except for the filter itself of course) > > removing > - blockdev-replace (make all parante of filter to be parents of X instead) > - blockdev-del filter > > It's simple to implement, and it seems for me that it is simpler to use. Any > thoughts? One reason I remember why we didn't decide to go this way in the many "dynamic graph reconfiguration" discussions we had, is that it's not generic enough to cover all cases. But I'm not sure if we ever considered root nodes as a separate case. I acknowledge that having two different interfaces is inconvenient, and integrating qom-set in a transaction is rather unlikely to happen. The reason why it's not generic is that it restricts you to doing the same thing for all parents. Imagine this: +- virtio-blk | file <- qcow2 <-+ | +- NBD export Now you want to throttle the NBD export so that it doesn't interfere with your VM too much. With your simple blockdev-replace this isn't possible. You would have to add the filter to both users or to none. In theory, blockdev-replace could take a list of the edges that should be changed to the new node. The problem is that edges don't have names, and even the parents don't necessarily have one (and if they do, they are in separate namespaces, so a BlockBackend, a job and an export could all have the same name), so finding a good way to refer to them in QMP doesn't sound trivial. Kevin
Re: RFC: Qemu backup interface plans
Am 19.05.2021 um 08:11 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 2. Test, that we can start backup job with source = (target of > > > backup-top filter), so that we have "push backup with fleecing". > > > Make an option for backup to start without a filter, when we don't > > > need copy-before-write operations, to not create extra superfluous > > > filter. > > > > OK, so the backup job is not really a backup job, but just anything > > that copies data. > > Not quite. For backup without a filter we should protect source from > changing, by unsharing WRITE permission on it. > > I'll try to avoid adding an option. The logic should work like in > commit job: if there are current writers on source we create filter. > If there no writers, we just unshare writes and go without a filter. > And for this copy-before-write filter should be able to do > WRITE_UNCHANGED in case of fleecing. If we ever get to the point where we would make a block-copy job visible to the user, I wouldn't copy the restrictions from the current jobs, but keep it really generic to cover all cases. There is no way for the QMP command starting the job to know what the user is planning to do with the image in the future. Even if it's currently read-only, the user may want to add a writer later. I think this means that we want to always add a filter node, and then possibly dynamically switch between modes if being read-only provides a significant advantage for the job. Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 15.03.2021 um 16:26 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> >> Markus Armbruster writes:
> >>
> >> > Paolo Bonzini writes:
> >> >
> >> >> On 11/03/21 15:08, Markus Armbruster wrote:
> >> >>>> I would rather keep the OptsVisitor here. Do the same check for JSON
> >> >>>> syntax that you have in qobject_input_visitor_new_str, and whenever
> >> >>>> you need to walk all -object arguments, use something like this:
> >> >>>>
> >> >>>> typedef struct ObjectArgument {
> >> >>>> const char *id;
> >> >>>> QDict *json;/* or NULL for QemuOpts */
> >> >>>> QSIMPLEQ_ENTRY(ObjectArgument) next;
> >> >>>> }
> >> >>>>
> >> >>>> I already had patches in my queue to store -object in a GSList of
> >> >>>> dictionaries, changing it to use the above is easy enough.
> >> >>>
> >> >>> I think I'd prefer following -display's precedence. See my reply to
> >> >>> Kevin for details.
> >> >>
> >> >> Yeah, I got independently to the same conclusion and posted patches
> >> >> for that. I was scared that visit_type_ObjectOptions was too much for
> >> >> OptsVisitor but it seems to work...
> >> >
> >> > We have reason to be scared. I'll try to cover this in my review.
> >>
> >> The opts visitor has serious limitations. From its header:
> >>
> >> * The Opts input visitor does not implement support for visiting QAPI
> >> * alternates, numbers (other than integers), null, or arbitrary
> >> * QTypes. It also requires a non-null list argument to
> >> * visit_start_list().
> >>
> >> This is retro-documentation for hairy code. I don't trust it. Commit
> >> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
> >> restrictions:
> >>
> >> The type tree in the schema, corresponding to an option with a
> >> discriminator, must have the following structure:
> >>
> >> struct
> >> scalar member for non-discriminated optarg 1 [*]
> >> list for repeating non-discriminated optarg 2 [*]
> >> wrapper struct
> >> single scalar member
> >> union
> >> struct for discriminator case 1
> >> scalar member for optarg 3 [*]
> >> list for repeating optarg 4 [*]
> >> wrapper struct
> >> single scalar member
> >> scalar member for optarg 5 [*]
> >> struct for discriminator case 2
> >> ...
> >
> > Is this a long-winded way of saying that it has to be flat, except that
> > it allows lists, i.e. there must be no nested objects on the "wire"?
>
> I think so.
>
> > The difference between structs and unions, and different branches inside
> > the union isn't visible for the visitor anyway.
>
> Yes, only the code using the visitor deals with that.
>
> >> The "type" optarg name is fixed for the discriminator role. Its schema
> >> representation is "union of structures", and each discriminator value
> >> must
> >> correspond to a member name in the union.
> >>
> >> If the option takes no "type" descriminator, then the type subtree
> >> rooted
> >> at the union must be absent from the schema (including the union
> >> itself).
> >>
> >> Optarg values can be of scalar types str / bool / integers / size.
> >>
> >> Unsupported visits are treated as programming error. Which is a nice
> >> way to say "they crash".
> >
> > The OptsVisitor never seems to crash explicitly by calling something
> > like abort().
> >
> > It may crash because of missing callbacks that are called without a NULL
> > check, like v->type_null.
>
> Correct.
>
> > This case should probably be fixed in
> > qapi/qapi-visit-core.c to do the check and simply return an error.
>
> I retro-documented what I inherited: qapi-visit-core.c code expects the
> visitors to implement the full visitor-impl.h interface, but some
> visitors don't. So I documented "method must be set to visit FOOs" in
> visitor-impl.h, and for the visitors that don't, I documented "can't
> visit FOOs".
>
> If the crashing behavior we've always had gets in the way, there are two
> ways to change it:
>
> 1. Complicate qapi-visit-core.c slightly to cope with incomplete visitor
>implementations.
>
> 2. Complete the visitor implementations: add dummy callbacks that fail.
>
> I prefer 2., because I feel it keeps the visitor-impl.h interface
> simpler, and puts the extra complications where they belong.
I suggested making the callbacks optional because I expected that there
might be more than one visitor that doesn't support a callback and I
wouldn't like duplicating dummy callbacks in multiple places. But if
it's only the OptsVisitor, then we wouldn't get any duplication either
way and it becomes a matter of taste.
Kevin
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Am 15.03.2021 um 15:15 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> >> Paolo Bonzini writes:
> >>
> >> > On 13/03/21 08:40, Markus Armbruster wrote:
> >> >>> +if (!user_creatable_add_from_str(optarg, _err))
> >> >>> {
> >> >>> +if (local_err) {
> >> >>> +error_report_err(local_err);
> >> >>> +exit(2);
> >> >>> +} else {
> >> >>> +/* Help was printed */
> >> >>> +exit(EXIT_SUCCESS);
> >> >>> +}
> >> >>> +}
> >> >>> +break;
> >> >>> }
> >> >>> -} break;
> >> >>> case OPTION_IMAGE_OPTS:
> >> >>> image_opts = true;
> >> >>> break;
> >> >> Why is this one different? The others all call
> >> >> user_creatable_process_cmdline().
> >> >>
> >> >>
> >> >
> >> > It's to exit with status code 2 instead of 1.
> >>
> >> I see. Worth a comment?
> >
> > There is a comment at the start of the function (which is just a few
> > lines above) that explains the exit codes:
> >
> > * Compares two images. Exit codes:
> > *
> > * 0 - Images are identical or the requested help was printed
> > * 1 - Images differ
> > * >1 - Error occurred
>
> I had in mind a comment that helps me over the "why is this not using
> user_creatable_process_cmdline()" hump. Like so:
>
> case OPTION_OBJECT:
> {
> /*
> * Can't use user_creatable_process_cmdline(), because
> * we need to exit(2) on error.
> */
> ... open-coded variation of
> user_creatable_process_cmdline() ...
> }
>
> Entirely up to you.
I see. This patch is already part of a pull request, but I wouldn't mind
a follow-up patch to add this comment if you want to send one.
Kevin
Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()
Am 13.03.2021 um 09:41 hat Markus Armbruster geschrieben: > Observation, not objection: > > 1. QMP core parses JSON text into QObject, passes to generated >marshaller. > > 2. Marshaller converts QObject to ObjectOptions with the QObject input >visitor, passes to qmp_object_add(). > > 3. qmp_object_add() wraps around user_creatable_add_qapi(). > > 4. user_creatable_add_qapi() converts right back to QObject with the >QObject output visitor. It splits the result into qom_type, id and >the rest, and passes all three to user_creatable_add_type(). > > 5. user_creatable_add_type() performs a virtual visit with the QObject >input visitor. The outermost object it visits itself, its children >it visits by calling object_property_set(). > > I sure hope we wouldn't write it this way from scratch :) > > I think your patch is a reasonable step towards a QOM that is at peace > with QAPI. But there's plenty of work left. Yes, obviously the conversion back to QObject is not great. There are two reasons why we currently need it: 1. user_creatable_add_type() wants to iterate over all properties without knowing which properties exist. This should be fixed by not visiting the top level in user_creatable_add_type(), but moving this part to object-specific code (in the final state probably code generated from the QAPI schema). 2. We have ObjectOptions, but need to pass a visitor. We don't have a general purpose visitor that visits both sides. The clone visitor seems somewhat similar to what we need, but I seem to remember it was more restricted to its particular use case. Kevin
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> Paolo Bonzini writes:
>
> > On 13/03/21 08:40, Markus Armbruster wrote:
> >>> +if (!user_creatable_add_from_str(optarg, _err)) {
> >>> +if (local_err) {
> >>> +error_report_err(local_err);
> >>> +exit(2);
> >>> +} else {
> >>> +/* Help was printed */
> >>> +exit(EXIT_SUCCESS);
> >>> +}
> >>> +}
> >>> +break;
> >>> }
> >>> -} break;
> >>> case OPTION_IMAGE_OPTS:
> >>> image_opts = true;
> >>> break;
> >> Why is this one different? The others all call
> >> user_creatable_process_cmdline().
> >>
> >>
> >
> > It's to exit with status code 2 instead of 1.
>
> I see. Worth a comment?
There is a comment at the start of the function (which is just a few
lines above) that explains the exit codes:
* Compares two images. Exit codes:
*
* 0 - Images are identical or the requested help was printed
* 1 - Images differ
* >1 - Error occurred
Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> Markus Armbruster writes:
>
> > Paolo Bonzini writes:
> >
> >> On 11/03/21 15:08, Markus Armbruster wrote:
> I would rather keep the OptsVisitor here. Do the same check for JSON
> syntax that you have in qobject_input_visitor_new_str, and whenever
> you need to walk all -object arguments, use something like this:
>
> typedef struct ObjectArgument {
> const char *id;
> QDict *json;/* or NULL for QemuOpts */
> QSIMPLEQ_ENTRY(ObjectArgument) next;
> }
>
> I already had patches in my queue to store -object in a GSList of
> dictionaries, changing it to use the above is easy enough.
> >>>
> >>> I think I'd prefer following -display's precedence. See my reply to
> >>> Kevin for details.
> >>
> >> Yeah, I got independently to the same conclusion and posted patches
> >> for that. I was scared that visit_type_ObjectOptions was too much for
> >> OptsVisitor but it seems to work...
> >
> > We have reason to be scared. I'll try to cover this in my review.
>
> The opts visitor has serious limitations. From its header:
>
> * The Opts input visitor does not implement support for visiting QAPI
> * alternates, numbers (other than integers), null, or arbitrary
> * QTypes. It also requires a non-null list argument to
> * visit_start_list().
>
> This is retro-documentation for hairy code. I don't trust it. Commit
> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
> restrictions:
>
> The type tree in the schema, corresponding to an option with a
> discriminator, must have the following structure:
>
> struct
> scalar member for non-discriminated optarg 1 [*]
> list for repeating non-discriminated optarg 2 [*]
> wrapper struct
> single scalar member
> union
> struct for discriminator case 1
> scalar member for optarg 3 [*]
> list for repeating optarg 4 [*]
> wrapper struct
> single scalar member
> scalar member for optarg 5 [*]
> struct for discriminator case 2
> ...
Is this a long-winded way of saying that it has to be flat, except that
it allows lists, i.e. there must be no nested objects on the "wire"?
The difference between structs and unions, and different branches inside
the union isn't visible for the visitor anyway.
> The "type" optarg name is fixed for the discriminator role. Its schema
> representation is "union of structures", and each discriminator value must
> correspond to a member name in the union.
>
> If the option takes no "type" descriminator, then the type subtree rooted
> at the union must be absent from the schema (including the union itself).
>
> Optarg values can be of scalar types str / bool / integers / size.
>
> Unsupported visits are treated as programming error. Which is a nice
> way to say "they crash".
The OptsVisitor never seems to crash explicitly by calling something
like abort().
It may crash because of missing callbacks that are called without a NULL
check, like v->type_null. This case should probably be fixed in
qapi/qapi-visit-core.c to do the check and simply return an error.
Any other cases?
> Before this series, we use it for -object as follows.
>
> user_creatable_add_opts() massages the QemuOpts into a QDict containing
> just the properties, then calls user_creatable_add_type() with the opts
> visitor wrapped around the QemuOpts, and the QDict.
>
> user_creatable_add_type() performs a virtual visit. The outermost
> object it visits itself. Then it visits members one by one by calling
> object_property_set(). It uses the QDict as a list of members to visit.
>
> As long as the object_property_set() only visit scalars other than
> floating-point numbers, we safely stay with the opts visitors'
> limitations.
Minor addition: This visits inside object_property_set() are
non-virtual, of course.
> After this series, we use the opts visitor to convert the option
> argument to a ObjectOption. This is a non-virtual visit. We then
> convert the ObjectOption to a QDict, and call user_creatable_add_type()
> with the QObject input visitor wrapped around the QDict, and the QDict.
>
> Here's the difference in opts visitor use: before the patch, we visit
> exactly the members in the optarg that actually name QOM properties (for
> the ones that don't, object_property_set() fails without visiting
> anything). Afterwards, we visit the members of ObjectOption, i.e.
> all QOM properties, by construction of ObjectOption.
>
> As long as ObjectOption's construction is correct, the series does not
> add new visits, i.e. we're no worse off than before.
>
> However, there is now a new way to mess things up: you can change (a
> branch of union) ObjectOption in a way that pushes it beyond the opts
> visitors
Re: [PATCH v3 27/30] hmp: QAPIfy object_add
Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> Paolo Bonzini writes:
>
> > On 13/03/21 14:28, Markus Armbruster wrote:
> >> Kevin Wolf writes:
> >>
> >>> This switches the HMP command object_add from a QemuOpts-based parser to
> >>> user_creatable_add_from_str() which uses a keyval parser and enforces
> >>> the QAPI schema.
> >>>
> >>> Apart from being a cleanup, this makes non-scalar properties and help
> >>> accessible. In order for help to be printed to the monitor instead of
> >>> stdout, the printf() calls in the help functions are changed to
> >>> qemu_printf().
> >>>
> >>> Signed-off-by: Kevin Wolf
> >>> Acked-by: Peter Krempa
> >>> Reviewed-by: Eric Blake
> >>> Reviewed-by: Dr. David Alan Gilbert
> >>> ---
> >>> monitor/hmp-cmds.c | 17 ++---
> >>> qom/object_interfaces.c | 11 ++-
> >>> hmp-commands.hx | 2 +-
> >>> 3 files changed, 9 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> >>> index 3c88a4faef..652cf9ff21 100644
> >>> --- a/monitor/hmp-cmds.c
> >>> +++ b/monitor/hmp-cmds.c
> >>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict
> >>> *qdict)
> >>>
> >>> void hmp_object_add(Monitor *mon, const QDict *qdict)
> >>> {
> >>> +const char *options = qdict_get_str(qdict, "object");
> >>> Error *err = NULL;
> >>> -QemuOpts *opts;
> >>> -Object *obj = NULL;
> >>> -
> >>> -opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, );
> >>> -if (err) {
> >>> -goto end;
> >>> -}
> >>>
> >>> -obj = user_creatable_add_opts(opts, );
> >>> -qemu_opts_del(opts);
> >>> -
> >>> -end:
> >>> +user_creatable_add_from_str(options, );
> >>> hmp_handle_error(mon, err);
> >>> -
> >>> -if (obj) {
> >>> -object_unref(obj);
> >>> -}
> >>> }
> >>
> >> Doesn't this break the list-valued properties (Memdev member host-nodes,
> >> NumaNodeOptions member cpus) exactly the same way that made us keep
> >> QemuOpts for qemu-system-FOO -object?
> >
> > Yes, it does. I guess it can just be documented, unlike for the command
> > line?
>
> Maybe. Judgement call, not mine to make.
>
> Do people create such objects in HMP? I figure we don't really know.
> Educated guess?
>
> If you try, how does it break? Is it confusing? Can you show an
> example?
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
Error: Invalid parameter type for 'host-nodes', expected: array
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
(qemu)
HMP is not a stable interface, so changing the syntax didn't feel like a
problem to me. I doubt many people do HMP memory hotplug while setting a
specific NUMA policy, but it wouldn't change my assessment anyway. I
should have made this explicit in the commit message, though.
Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben: > On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote: > > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben: > > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > > > On 10/03/21 15:22, Peter Krempa wrote: > > [...] > > > > -object > > > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind > > > > Oh, we have ranges, too... That makes the compatibility code even > > nastier to write. I doubt that we can implement this in the keyval > > parser alone without touching the visitor. :-/ > > > > > If any of the above is to be deprecated we'll need to adjust our > > > JSON->commandline generator accordignly. > > > > > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is > > > actually modular to allow plugging an array interpretor which actually > > > does the above conversion from a JSON array. > > > > > > So, what is the preferred syntax here? Additionally we might need a > > > witness property to detect when to use the new syntax if basing it on > > > the object-add qapification will not be enough. > > > > The list syntax supported by the keyval parser is the one you know from > > -blockdev: host-nodes.0=3,host-nodes.1=7, ... > > I think that should be easy enough to convert to. We could also support JSON syntax in QEMU. That would probably be even more convenient for libvirt? > Is it safe to do right away (with the old parser?). Otherwise we need > to agree on something which will let us detect when it's safe to > change. Neither keyval nor JSON syntax work with the old QemuOpts parser. What is the usual way to do this for command line options? If we don't have a good way there, we can always tie it to something in the QAPI schema. If we still get this solved in time for 6.0, we could use the existence of ObjectOptions. Or all else failing, we can use a feature flag. Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 11.03.2021 um 09:14 hat Paolo Bonzini geschrieben: > On 10/03/21 18:30, Kevin Wolf wrote: > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > On 10/03/21 15:22, Peter Krempa wrote: > > > > I've stumbled upon a regression with this patchset applied: > > > > > > > > error: internal error: process exited while connecting to monitor: > > > > qemu-system-x86_64: -object > > > > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: > > > > Invalid parameter type for 'host-nodes', expected: array > > > > > > This is the magic conversion of "string containing a list of integers" > > > to "list of integers". > > > > Ah, crap. This one wouldn't have been a problem when converting only > > object-add, and I trusted your audit that user creatable objects don't > > depend on any QemuOpts magic. I should have noticed it, too, of course, > > but during the convertion I didn't have QemuOpts in mind, only QOM and > > QAPI. > > Yeah, let's just drop the -object conversion for now. It will just remove a > few patches. I think it's only patch 29 and 30 that we would have to drop, actually. Unfortunately, that still removes one of the most immediately useful features, which is non-scalar properties for -object in the system emulator. But of course, a lot better than not merging it at all. > Who is going to include this series in the next pull request, Markus or > myself? The time is ticking for soft freeze. QOM is probably the right subsystem, so that would be you. Or I can just merge it myself as long as everyone is fine with it. Eric has some minor comments that I think could be addressed while applying the series. Or should I send a v4 for that (and for dropping patches 29 and 30)? Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben: > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > On 10/03/21 15:22, Peter Krempa wrote: > > [...] > > > The keyval parser would create a list if multiple values are given for > > the same key. Some care needs to be taken to avoid mixing the magic > > list feature with the normal indexed list options. > > > > The QAPI schema would then use an alternate between 'int' and ['int'], > > with the the memory-backend-ram implementation changed accordingly. > > > > We could consider immediately deprecating the syntax and printing a > > warning in the keyval parser when it automatically creates a list from > > two values for a key, so that users don't start using this syntax > > By 'creating a list from two values for a key' you mean: > > host-nodes=0,host-nodes=1 > > to be converted into [0, 1] ? > > > instead of the normal list syntax in other places. We'd probably still > > leave the implementation around for -device and other users of the same > > magic. > > There's three options actually that libvirt uses, visible in one our > test files [1] > > For a single value we format: > > -object > memory-backend-ram,id=ram-node0,size=20971520,host-nodes=3,policy=preferred > > For a contiguous list: > > -object > memory-backend-ram,id=ram-node1,size=676331520,host-nodes=0-7,policy=bind > > And for an interleaved list: > > -object > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind Oh, we have ranges, too... That makes the compatibility code even nastier to write. I doubt that we can implement this in the keyval parser alone without touching the visitor. :-/ > If any of the above is to be deprecated we'll need to adjust our > JSON->commandline generator accordignly. > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is > actually modular to allow plugging an array interpretor which actually > does the above conversion from a JSON array. > > So, what is the preferred syntax here? Additionally we might need a > witness property to detect when to use the new syntax if basing it on > the object-add qapification will not be enough. The list syntax supported by the keyval parser is the one you know from -blockdev: host-nodes.0=3,host-nodes.1=7, ... Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > On 10/03/21 15:22, Peter Krempa wrote: > > I've stumbled upon a regression with this patchset applied: > > > > error: internal error: process exited while connecting to monitor: > > qemu-system-x86_64: -object > > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: > > Invalid parameter type for 'host-nodes', expected: array > > This is the magic conversion of "string containing a list of integers" > to "list of integers". Ah, crap. This one wouldn't have been a problem when converting only object-add, and I trusted your audit that user creatable objects don't depend on any QemuOpts magic. I should have noticed it, too, of course, but during the convertion I didn't have QemuOpts in mind, only QOM and QAPI. I checked all object types, and it seems this is the only one that is affected. We have a second list in AuthZListProperties, but it contains structs, so it was never supported on the command line anyway. > The relevant code is in qapi/string-input-visitor.c, but I'm not sure where > to stick it in the keyval-based parsing flow (i.e. > qobject_input_visitor_new_keyval). Markus, any ideas? The best I can think of at the moment would be adding a flag to the keyval parser that would enable the feature only for -object (and only in the system emulator, because memory-backend-ram doesn't exist in the tools): The keyval parser would create a list if multiple values are given for the same key. Some care needs to be taken to avoid mixing the magic list feature with the normal indexed list options. The QAPI schema would then use an alternate between 'int' and ['int'], with the the memory-backend-ram implementation changed accordingly. We could consider immediately deprecating the syntax and printing a warning in the keyval parser when it automatically creates a list from two values for a key, so that users don't start using this syntax instead of the normal list syntax in other places. We'd probably still leave the implementation around for -device and other users of the same magic. Kevin
[PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()
The implementation for --object can be shared between
qemu-storage-daemon and other binaries, so move it into a function in
qom/object_interfaces.c that is accessible from everywhere.
This also requires moving the implementation of qmp_object_add() into a
new user_creatable_add_qapi(), because qom/qom-qmp-cmds.c is not linked
for tools.
user_creatable_print_help_from_qdict() can become static now.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
include/qom/object_interfaces.h | 41 +++
qom/object_interfaces.c | 50 +++-
qom/qom-qmp-cmds.c | 20 +--
storage-daemon/qemu-storage-daemon.c | 24 ++---
4 files changed, 80 insertions(+), 55 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 5299603f50..1e6c51b541 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -2,6 +2,7 @@
#define OBJECT_INTERFACES_H
#include "qom/object.h"
+#include "qapi/qapi-types-qom.h"
#include "qapi/visitor.h"
#define TYPE_USER_CREATABLE "user-creatable"
@@ -86,6 +87,18 @@ Object *user_creatable_add_type(const char *type, const char
*id,
const QDict *qdict,
Visitor *v, Error **errp);
+/**
+ * user_creatable_add_qapi:
+ * @options: the object definition
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object according to the
+ * options passed in @opts as described in the QAPI schema documentation.
+ *
+ * Returns: the newly created object or NULL on error
+ */
+void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
+
/**
* user_creatable_add_opts:
* @opts: the object definition
@@ -131,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const
char *type);
int user_creatable_add_opts_foreach(void *opaque,
QemuOpts *opts, Error **errp);
+/**
+ * user_creatable_process_cmdline:
+ * @optarg: the object definition string as passed on the command line
+ *
+ * Create an instance of the user creatable object by parsing optarg
+ * with a keyval parser and implicit key 'qom-type', converting the
+ * result to ObjectOptions and calling into qmp_object_add().
+ *
+ * If a help option is given, print help instead and exit.
+ *
+ * This function is only meant to be called during command line parsing.
+ * It exits the process on failure or after printing help.
+ */
+void user_creatable_process_cmdline(const char *optarg);
+
/**
* user_creatable_print_help:
* @type: the QOM type to be added
@@ -145,19 +173,6 @@ int user_creatable_add_opts_foreach(void *opaque,
*/
bool user_creatable_print_help(const char *type, QemuOpts *opts);
-/**
- * user_creatable_print_help_from_qdict:
- * @args: options to create
- *
- * Prints help considering the other options given in @args (if "qom-type" is
- * given and valid, print properties for the type, otherwise print valid types)
- *
- * In contrast to user_creatable_print_help(), this function can't return that
- * no help was requested. It should only be called if we know that help is
- * requested and it will always print some help.
- */
-void user_creatable_print_help_from_qdict(QDict *args);
-
/**
* user_creatable_del:
* @id: the unique ID for the object
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 02c3934329..2eaf9971f5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -2,10 +2,13 @@
#include "qemu/cutils.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-visit-qom.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qjson.h"
#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
#include "qom/object_interfaces.h"
#include "qemu/help_option.h"
#include "qemu/id.h"
@@ -113,6 +116,29 @@ out:
return obj;
}
+void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
+{
+Visitor *v;
+QObject *qobj;
+QDict *props;
+Object *obj;
+
+v = qobject_output_visitor_new();
+visit_type_ObjectOptions(v, NULL, , _abort);
+visit_complete(v, );
+visit_free(v);
+
+props = qobject_to(QDict, qobj);
+qdict_del(props, "qom-type");
+qdict_del(props, "id");
+
+v = qobject_input_visitor_new(QOBJECT(props));
+obj = user_creatable_add_type(ObjectType_str(options->qom_type),
+ options->id, props, v, errp);
+object_unref(obj);
+visit_free(v);
+}
+
Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
{
Visitor *v;
@@ -256,7 +282,7 @@ bool user_c
[PATCH v3 30/30] qom: Drop QemuOpts based interfaces
user_creatable_add_opts() has only a single user left, which is a test
case. Rewrite the test to use user_creatable_add_type() instead (which
is the remaining function that doesn't require a QAPI schema) and drop
the QemuOpts related functions.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
include/qom/object_interfaces.h | 59
qom/object_interfaces.c | 81 -
tests/check-qom-proplist.c | 42 -
3 files changed, 20 insertions(+), 162 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index fb32330901..ac6c33ceac 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -99,51 +99,6 @@ Object *user_creatable_add_type(const char *type, const char
*id,
*/
void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
-/**
- * user_creatable_add_opts:
- * @opts: the object definition
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object whose type
- * is defined in @opts by the 'qom-type' option, placing it
- * in the object composition tree with name provided by the
- * 'id' field. The remaining options in @opts are used to
- * initialize the object properties.
- *
- * Returns: the newly created object or NULL on error
- */
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp);
-
-
-/**
- * user_creatable_add_opts_predicate:
- * @type: the QOM type to be added
- *
- * A callback function to determine whether an object
- * of type @type should be created. Instances of this
- * callback should be passed to user_creatable_add_opts_foreach
- */
-typedef bool (*user_creatable_add_opts_predicate)(const char *type);
-
-/**
- * user_creatable_add_opts_foreach:
- * @opaque: a user_creatable_add_opts_predicate callback or NULL
- * @opts: options to create
- * @errp: unused
- *
- * An iterator callback to be used in conjunction with
- * the qemu_opts_foreach() method for creating a list of
- * objects from a set of QemuOpts
- *
- * The @opaque parameter can be passed a user_creatable_add_opts_predicate
- * callback to filter which types of object are created during iteration.
- * When it fails, report the error.
- *
- * Returns: 0 on success, -1 when an error was reported.
- */
-int user_creatable_add_opts_foreach(void *opaque,
-QemuOpts *opts, Error **errp);
-
/**
* user_creatable_parse_str:
* @optarg: the object definition string as passed on the command line
@@ -190,20 +145,6 @@ bool user_creatable_add_from_str(const char *optarg, Error
**errp);
*/
void user_creatable_process_cmdline(const char *optarg);
-/**
- * user_creatable_print_help:
- * @type: the QOM type to be added
- * @opts: options to create
- *
- * Prints help if requested in @type or @opts. Note that if @type is neither
- * "help"/"?" nor a valid user creatable type, no help will be printed
- * regardless of @opts.
- *
- * Returns: true if a help option was found and help was printed, false
- * otherwise.
- */
-bool user_creatable_print_help(const char *type, QemuOpts *opts);
-
/**
* user_creatable_del:
* @id: the unique ID for the object
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 62d7db7629..61d6d74a26 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -10,13 +10,10 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
#include "qom/object_interfaces.h"
-#include "qemu/help_option.h"
#include "qemu/id.h"
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/qemu-print.h"
-#include "qapi/opts-visitor.h"
-#include "qemu/config-file.h"
bool user_creatable_complete(UserCreatable *uc, Error **errp)
{
@@ -140,60 +137,6 @@ void user_creatable_add_qapi(ObjectOptions *options, Error
**errp)
visit_free(v);
}
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
-{
-Visitor *v;
-QDict *pdict;
-Object *obj;
-const char *id = qemu_opts_id(opts);
-char *type = qemu_opt_get_del(opts, "qom-type");
-
-if (!type) {
-error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-return NULL;
-}
-if (!id) {
-error_setg(errp, QERR_MISSING_PARAMETER, "id");
-qemu_opt_set(opts, "qom-type", type, _abort);
-g_free(type);
-return NULL;
-}
-
-qemu_opts_set_id(opts, NULL);
-pdict = qemu_opts_to_qdict(opts, NULL);
-
-v = opts_visitor_new(opts);
-obj = user_creatable_add_type(type, id, pdict, v, errp);
-visit_free(v);
-
-qemu_opts_set_id(opts, (char *) id);
-qemu_opt_set(opts, "qom-type", type, _abort);
-g_free(type);
-qobject_unref(pdict);
-return obj;
-}
-
[PATCH v3 29/30] vl: QAPIfy -object
This switches the system emulator from a QemuOpts-based parser for
-object to user_creatable_parse_str() which uses a keyval parser and
enforces the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties accessible.
This adopts a similar model as -blockdev uses: When parsing the option,
create the ObjectOptions and queue them. At the later point where we
used to create objects for the collected QemuOpts, the ObjectOptions
queue is processed instead.
A complication compared to -blockdev is that object definitions are
supported in -readconfig and -writeconfig.
After this patch, -readconfig still works, though it still goes through
the QemuOpts parser, which means that improvements like non-scalar
properties are still not available in config files.
-writeconfig stops working for -object. Tough luck. It has never
supported all options (not even the common ones), so supporting one less
isn't the end of the world. As object definitions from -readconfig still
go through QemuOpts, they are still included in -writeconfig output,
which at least prevents destroying your existing configuration when you
just wanted to add another option.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
softmmu/vl.c | 109 +++
1 file changed, 84 insertions(+), 25 deletions(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 10bd8a10a3..deb061cc78 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -113,6 +113,7 @@
#include "sysemu/replay.h"
#include "qapi/qapi-events-run-state.h"
#include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-visit-qom.h"
#include "qapi/qapi-visit-ui.h"
#include "qapi/qapi-commands-block-core.h"
#include "qapi/qapi-commands-migration.h"
@@ -132,6 +133,14 @@ typedef struct BlockdevOptionsQueueEntry {
typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
+typedef struct ObjectOptionsQueueEntry {
+ObjectOptions *options;
+Location loc;
+QTAILQ_ENTRY(ObjectOptionsQueueEntry) next;
+} ObjectOptionsQueueEntry;
+
+typedef QTAILQ_HEAD(, ObjectOptionsQueueEntry) ObjectOptionsQueue;
+
static const char *cpu_option;
static const char *mem_path;
static const char *incoming;
@@ -143,6 +152,7 @@ static int snapshot;
static bool preconfig_requested;
static QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list);
static BlockdevOptionsQueue bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
+static ObjectOptionsQueue obj_queue = QTAILQ_HEAD_INITIALIZER(obj_queue);
static bool nographic = false;
static int mem_prealloc; /* force preallocation of physical target memory */
static ram_addr_t ram_size;
@@ -1691,12 +1701,9 @@ static int machine_set_property(void *opaque,
* cannot be created here, as it depends on the chardev
* already existing.
*/
-static bool object_create_early(const char *type, QemuOpts *opts)
+static bool object_create_early(ObjectOptions *options)
{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-
+const char *type = ObjectType_str(options->qom_type);
/*
* Objects should not be made "delayed" without a reason. If you
* add one, state the reason in a comment!
@@ -1744,6 +1751,56 @@ static bool object_create_early(const char *type,
QemuOpts *opts)
return true;
}
+static void object_queue_create(bool early)
+{
+ObjectOptionsQueueEntry *entry, *next;
+
+QTAILQ_FOREACH_SAFE(entry, _queue, next, next) {
+if (early != object_create_early(entry->options)) {
+continue;
+}
+QTAILQ_REMOVE(_queue, entry, next);
+loc_push_restore(>loc);
+user_creatable_add_qapi(entry->options, _fatal);
+loc_pop(>loc);
+qapi_free_ObjectOptions(entry->options);
+g_free(entry);
+}
+}
+
+/*
+ * -readconfig still parses things into QemuOpts. Convert any such
+ * configurations to an ObjectOptionsQueueEntry.
+ *
+ * This is more restricted than the normal -object parser because QemuOpts
+ * parsed things, so no support for non-scalar properties. Help is also not
+ * supported (but this shouldn't be requested in a config file anyway).
+ */
+static int object_readconfig_to_qapi(void *opaque, QemuOpts *opts, Error
**errp)
+{
+ERRP_GUARD();
+ObjectOptionsQueueEntry *entry;
+ObjectOptions *options;
+QDict *args = qemu_opts_to_qdict(opts, NULL);
+Visitor *v;
+
+v = qobject_input_visitor_new_keyval(QOBJECT(args));
+visit_type_ObjectOptions(v, NULL, , errp);
+visit_free(v);
+qobject_unref(args);
+
+if (*errp) {
+return -1;
+}
+
+entry = g_new0(ObjectOptionsQueueEntry, 1);
+entry->options = options;
+loc_save(>loc);
+QTAILQ_INSERT_TAIL(_queue, entry, next);
+
+return 0;
+}
+
static void qemu_apply_machine_options(void)
{
MachineClass *machine_class
[PATCH v3 28/30] qom: Add user_creatable_parse_str()
The system emulator has a more complicated way of handling command line
options in that it reorders options before it processes them. This means
that parsing object options and creating the object happen at two
different points. Split the parsing part into a separate function that
can be reused by the system emulator command line.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
include/qom/object_interfaces.h | 15 +++
qom/object_interfaces.c | 20 ++--
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 07511e6cff..fb32330901 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -144,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const
char *type);
int user_creatable_add_opts_foreach(void *opaque,
QemuOpts *opts, Error **errp);
+/**
+ * user_creatable_parse_str:
+ * @optarg: the object definition string as passed on the command line
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Parses the option for the user creatable object with a keyval parser and
+ * implicit key 'qom-type', converting the result to ObjectOptions.
+ *
+ * If a help option is given, print help instead.
+ *
+ * Returns: ObjectOptions on success, NULL when an error occurred (*errp is set
+ * then) or help was printed (*errp is not set).
+ */
+ObjectOptions *user_creatable_parse_str(const char *optarg, Error **errp);
+
/**
* user_creatable_add_from_str:
* @optarg: the object definition string as passed on the command line
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 6dcab60f09..62d7db7629 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -292,7 +292,7 @@ static void user_creatable_print_help_from_qdict(QDict
*args)
}
}
-bool user_creatable_add_from_str(const char *optarg, Error **errp)
+ObjectOptions *user_creatable_parse_str(const char *optarg, Error **errp)
{
ERRP_GUARD();
QDict *args;
@@ -302,12 +302,12 @@ bool user_creatable_add_from_str(const char *optarg,
Error **errp)
args = keyval_parse(optarg, "qom-type", , errp);
if (*errp) {
-return false;
+return NULL;
}
if (help) {
user_creatable_print_help_from_qdict(args);
qobject_unref(args);
-return false;
+return NULL;
}
v = qobject_input_visitor_new_keyval(QOBJECT(args));
@@ -315,12 +315,20 @@ bool user_creatable_add_from_str(const char *optarg,
Error **errp)
visit_free(v);
qobject_unref(args);
-if (*errp) {
-goto out;
+return options;
+}
+
+bool user_creatable_add_from_str(const char *optarg, Error **errp)
+{
+ERRP_GUARD();
+ObjectOptions *options;
+
+options = user_creatable_parse_str(optarg, errp);
+if (!options) {
+return false;
}
user_creatable_add_qapi(options, errp);
-out:
qapi_free_ObjectOptions(options);
return !*errp;
}
--
2.29.2
[PATCH v3 16/30] qapi/qom: Add ObjectOptions for input-*
This adds a QAPI schema for the properties of the input-* objects.
ui.json cannot be included in qom.json because the storage daemon can't
use it, so move GrabToggleKeys to common.json.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/common.json | 12 ++
qapi/qom.json| 59
qapi/ui.json | 13 +--
3 files changed, 72 insertions(+), 12 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index b87e7f9039..7c976296f0 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -185,3 +185,15 @@
##
{ 'enum': 'NetFilterDirection',
'data': [ 'all', 'rx', 'tx' ] }
+
+##
+# @GrabToggleKeys:
+#
+# Keys to toggle input-linux between host and guest.
+#
+# Since: 4.0
+#
+##
+{ 'enum': 'GrabToggleKeys',
+ 'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
+'ctrl-scrolllock' ] }
diff --git a/qapi/qom.json b/qapi/qom.json
index ad72dbdec2..6b96e9b0b3 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -444,6 +444,61 @@
'base': 'NetfilterProperties',
'data': { '*vnet_hdr_support': 'bool' } }
+##
+# @InputBarrierProperties:
+#
+# Properties for input-barrier objects.
+#
+# @name: the screen name as declared in the screens section of barrier.conf
+#
+# @server: hostname of the Barrier server (default: "localhost")
+#
+# @port: TCP port of the Barrier server (default: "24800")
+#
+# @x-origin: x coordinate of the leftmost pixel on the guest screen
+#(default: "0")
+#
+# @y-origin: y coordinate of the topmost pixel on the guest screen
+#(default: "0")
+#
+# @width: the width of secondary screen in pixels (default: "1920")
+#
+# @height: the height of secondary screen in pixels (default: "1080")
+#
+# Since: 4.2
+##
+{ 'struct': 'InputBarrierProperties',
+ 'data': { 'name': 'str',
+'*server': 'str',
+'*port': 'str',
+'*x-origin': 'str',
+'*y-origin': 'str',
+'*width': 'str',
+'*height': 'str' } }
+
+##
+# @InputLinuxProperties:
+#
+# Properties for input-linux objects.
+#
+# @evdev: the path of the host evdev device to use
+#
+# @grab_all: if true, grab is toggled for all devices (e.g. both keyboard and
+#mouse) instead of just one device (default: false)
+#
+# @repeat: enables auto-repeat events (default: false)
+#
+# @grab-toggle: the key or key combination that toggles device grab
+# (default: ctrl-ctrl)
+#
+# Since: 2.6
+##
+{ 'struct': 'InputLinuxProperties',
+ 'data': { 'evdev': 'str',
+'*grab_all': 'bool',
+'*repeat': 'bool',
+'*grab-toggle': 'GrabToggleKeys' } }
+
##
# @IothreadProperties:
#
@@ -691,6 +746,8 @@
'filter-redirector',
'filter-replay',
'filter-rewriter',
+'input-barrier',
+'input-linux',
'iothread',
'memory-backend-file',
'memory-backend-memfd',
@@ -744,6 +801,8 @@
'filter-redirector': 'FilterRedirectorProperties',
'filter-replay': 'NetfilterProperties',
'filter-rewriter':'FilterRewriterProperties',
+ 'input-barrier': 'InputBarrierProperties',
+ 'input-linux':'InputLinuxProperties',
'iothread': 'IothreadProperties',
'memory-backend-file':'MemoryBackendFileProperties',
'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
diff --git a/qapi/ui.json b/qapi/ui.json
index d08d72b439..cc1882108b 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -6,6 +6,7 @@
# = Remote desktop
##
+{ 'include': 'common.json' }
{ 'include': 'sockets.json' }
##
@@ -1021,18 +1022,6 @@
'*head' : 'int',
'events' : [ 'InputEvent' ] } }
-##
-# @GrabToggleKeys:
-#
-# Keys to toggle input-linux between host and guest.
-#
-# Since: 4.0
-#
-##
-{ 'enum': 'GrabToggleKeys',
- 'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
-'ctrl-scrolllock' ] }
-
##
# @DisplayGTK:
#
--
2.29.2
[PATCH v3 27/30] hmp: QAPIfy object_add
This switches the HMP command object_add from a QemuOpts-based parser to
user_creatable_add_from_str() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties and help
accessible. In order for help to be printed to the monitor instead of
stdout, the printf() calls in the help functions are changed to
qemu_printf().
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
Reviewed-by: Dr. David Alan Gilbert
---
monitor/hmp-cmds.c | 17 ++---
qom/object_interfaces.c | 11 ++-
hmp-commands.hx | 2 +-
3 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 3c88a4faef..652cf9ff21 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
void hmp_object_add(Monitor *mon, const QDict *qdict)
{
+const char *options = qdict_get_str(qdict, "object");
Error *err = NULL;
-QemuOpts *opts;
-Object *obj = NULL;
-
-opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, );
-if (err) {
-goto end;
-}
-obj = user_creatable_add_opts(opts, );
-qemu_opts_del(opts);
-
-end:
+user_creatable_add_from_str(options, );
hmp_handle_error(mon, err);
-
-if (obj) {
-object_unref(obj);
-}
}
void hmp_getfd(Monitor *mon, const QDict *qdict)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf9f8cd2c6..6dcab60f09 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -14,6 +14,7 @@
#include "qemu/id.h"
#include "qemu/module.h"
#include "qemu/option.h"
+#include "qemu/qemu-print.h"
#include "qapi/opts-visitor.h"
#include "qemu/config-file.h"
@@ -221,11 +222,11 @@ static void user_creatable_print_types(void)
{
GSList *l, *list;
-printf("List of user creatable objects:\n");
+qemu_printf("List of user creatable objects:\n");
list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
for (l = list; l != NULL; l = l->next) {
ObjectClass *oc = OBJECT_CLASS(l->data);
-printf(" %s\n", object_class_get_name(oc));
+qemu_printf(" %s\n", object_class_get_name(oc));
}
g_slist_free(list);
}
@@ -256,12 +257,12 @@ static bool user_creatable_print_type_properites(const
char *type)
}
g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
if (array->len > 0) {
-printf("%s options:\n", type);
+qemu_printf("%s options:\n", type);
} else {
-printf("There are no options for %s.\n", type);
+qemu_printf("There are no options for %s.\n", type);
}
for (i = 0; i < array->len; i++) {
-printf("%s\n", (char *)array->pdata[i]);
+qemu_printf("%s\n", (char *)array->pdata[i]);
}
g_ptr_array_set_free_func(array, g_free);
g_ptr_array_free(array, true);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..6f5d9ce2fb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1337,7 +1337,7 @@ ERST
{
.name = "object_add",
-.args_type = "object:O",
+.args_type = "object:S",
.params = "[qom-type=]type,id=str[,prop=value][,...]",
.help = "create QOM object",
.cmd= hmp_object_add,
--
2.29.2
[PATCH v3 24/30] qemu-nbd: Use user_creatable_process_cmdline() for --object
This switches qemu-nbd from a QemuOpts-based parser for --object to
user_creatable_process_cmdline() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties accessible.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qemu-nbd.c | 34 +++---
1 file changed, 3 insertions(+), 31 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b1b9430a8f..93ef4e288f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -401,24 +401,6 @@ static QemuOptsList file_opts = {
},
};
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_nbd_object_print_help(const char *type, QemuOpts *opts)
-{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-return true;
-}
-
-
static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, bool list,
Error **errp)
{
@@ -594,7 +576,6 @@ int main(int argc, char **argv)
qcrypto_init(_fatal);
module_call_init(MODULE_INIT_QOM);
-qemu_add_opts(_object_opts);
qemu_add_opts(_trace_opts);
qemu_init_exec_dir(argv[0]);
@@ -747,14 +728,9 @@ int main(int argc, char **argv)
case '?':
error_report("Try `%s --help' for more information.", argv[0]);
exit(EXIT_FAILURE);
-case QEMU_NBD_OPT_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
- optarg, true);
-if (!opts) {
-exit(EXIT_FAILURE);
-}
-} break;
+case QEMU_NBD_OPT_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
case QEMU_NBD_OPT_TLSCREDS:
tlscredsid = optarg;
break;
@@ -802,10 +778,6 @@ int main(int argc, char **argv)
export_name = "";
}
-qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_nbd_object_print_help, _fatal);
-
if (!trace_init_backends()) {
exit(1);
}
--
2.29.2
[PATCH v3 25/30] qom: Add user_creatable_add_from_str()
This is a version of user_creatable_process_cmdline() with an Error
parameter that never calls exit() and is therefore usable in HMP.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
include/qom/object_interfaces.h | 16
qom/object_interfaces.c | 29 -
2 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 1e6c51b541..07511e6cff 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -144,6 +144,22 @@ typedef bool (*user_creatable_add_opts_predicate)(const
char *type);
int user_creatable_add_opts_foreach(void *opaque,
QemuOpts *opts, Error **errp);
+/**
+ * user_creatable_add_from_str:
+ * @optarg: the object definition string as passed on the command line
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object by parsing optarg
+ * with a keyval parser and implicit key 'qom-type', converting the
+ * result to ObjectOptions and calling into qmp_object_add().
+ *
+ * If a help option is given, print help instead.
+ *
+ * Returns: true when an object was successfully created, false when an error
+ * occurred (*errp is set then) or help was printed (*errp is not set).
+ */
+bool user_creatable_add_from_str(const char *optarg, Error **errp);
+
/**
* user_creatable_process_cmdline:
* @optarg: the object definition string as passed on the command line
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 2eaf9971f5..bf9f8cd2c6 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -291,26 +291,45 @@ static void user_creatable_print_help_from_qdict(QDict
*args)
}
}
-void user_creatable_process_cmdline(const char *optarg)
+bool user_creatable_add_from_str(const char *optarg, Error **errp)
{
+ERRP_GUARD();
QDict *args;
bool help;
Visitor *v;
ObjectOptions *options;
-args = keyval_parse(optarg, "qom-type", , _fatal);
+args = keyval_parse(optarg, "qom-type", , errp);
+if (*errp) {
+return false;
+}
if (help) {
user_creatable_print_help_from_qdict(args);
-exit(EXIT_SUCCESS);
+qobject_unref(args);
+return false;
}
v = qobject_input_visitor_new_keyval(QOBJECT(args));
-visit_type_ObjectOptions(v, NULL, , _fatal);
+visit_type_ObjectOptions(v, NULL, , errp);
visit_free(v);
qobject_unref(args);
-user_creatable_add_qapi(options, _fatal);
+if (*errp) {
+goto out;
+}
+
+user_creatable_add_qapi(options, errp);
+out:
qapi_free_ObjectOptions(options);
+return !*errp;
+}
+
+void user_creatable_process_cmdline(const char *optarg)
+{
+if (!user_creatable_add_from_str(optarg, _fatal)) {
+/* Help was printed */
+exit(EXIT_SUCCESS);
+}
}
bool user_creatable_del(const char *id, Error **errp)
--
2.29.2
[PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
This switches qemu-img from a QemuOpts-based parser for --object to
user_creatable_process_cmdline() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties accessible.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
---
qemu-img.c | 251 ++---
1 file changed, 45 insertions(+), 206 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index e2952fe955..babb5573ab 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -226,23 +226,6 @@ static void QEMU_NORETURN help(void)
exit(EXIT_SUCCESS);
}
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
-{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-return true;
-}
-
/*
* Is @optarg safe for accumulate_options()?
* It is when multiple of them can be joined together separated by ','.
@@ -566,14 +549,9 @@ static int img_create(int argc, char **argv)
case 'u':
flags |= BDRV_O_NO_BACKING;
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
- optarg, true);
-if (!opts) {
-goto fail;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
}
}
@@ -589,12 +567,6 @@ static int img_create(int argc, char **argv)
}
optind++;
-if (qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, _fatal)) {
-goto fail;
-}
-
/* Get image size, if specified */
if (optind < argc) {
int64_t sval;
@@ -804,14 +776,9 @@ static int img_check(int argc, char **argv)
case 'U':
force_share = true;
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
- optarg, true);
-if (!opts) {
-return 1;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
case OPTION_IMAGE_OPTS:
image_opts = true;
break;
@@ -831,12 +798,6 @@ static int img_check(int argc, char **argv)
return 1;
}
-if (qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, _fatal)) {
-return 1;
-}
-
ret = bdrv_parse_cache_mode(cache, , );
if (ret < 0) {
error_report("Invalid source cache option: %s", cache);
@@ -1034,14 +995,9 @@ static int img_commit(int argc, char **argv)
return 1;
}
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
- optarg, true);
-if (!opts) {
-return 1;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
case OPTION_IMAGE_OPTS:
image_opts = true;
break;
@@ -1058,12 +1014,6 @@ static int img_commit(int argc, char **argv)
}
filename = argv[optind++];
-if (qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, _fatal)) {
-return 1;
-}
-
flags = BDRV_O_RDWR | BDRV_O_UNMAP;
ret = bdrv_parse_cache_mode(cache, , );
if (ret < 0) {
@@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t
offset,
/*
* Compares two images. Exit codes:
*
- * 0 - Images are identical
+ * 0 - Images are identical or the requested help was printed
* 1 - Images differ
* >1 - Error occurred
*/
@@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
case 'U':
force_share = true;
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
- optarg, true);
-if (!opts) {
-ret = 2;
-goto out4;
+case OPTION_OBJECT:
+{
+Error *local_err = NULL;
+
+if (!user_creatable_add_from_str(optarg, _err)) {
+if (local_err) {
+error_report_err(l
[PATCH v3 23/30] qemu-io: Use user_creatable_process_cmdline() for --object
This switches qemu-io from a QemuOpts-based parser for --object to
user_creatable_process_cmdline() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties accessible.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qemu-io.c | 33 +++--
1 file changed, 3 insertions(+), 30 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index ac88d8bd40..bf902302e9 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -477,23 +477,6 @@ enum {
OPTION_IMAGE_OPTS = 257,
};
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_io_object_print_help(const char *type, QemuOpts *opts)
-{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-return true;
-}
-
static QemuOptsList file_opts = {
.name = "file",
.implied_opt_name = "file",
@@ -550,7 +533,6 @@ int main(int argc, char **argv)
qcrypto_init(_fatal);
module_call_init(MODULE_INIT_QOM);
-qemu_add_opts(_object_opts);
qemu_add_opts(_trace_opts);
bdrv_init();
@@ -612,14 +594,9 @@ int main(int argc, char **argv)
case 'U':
force_share = true;
break;
-case OPTION_OBJECT: {
-QemuOpts *qopts;
-qopts = qemu_opts_parse_noisily(_object_opts,
-optarg, true);
-if (!qopts) {
-exit(1);
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
case OPTION_IMAGE_OPTS:
imageOpts = true;
break;
@@ -644,10 +621,6 @@ int main(int argc, char **argv)
exit(1);
}
-qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_io_object_print_help, _fatal);
-
if (!trace_init_backends()) {
exit(1);
}
--
2.29.2
[PATCH v3 08/30] qapi/qom: Add ObjectOptions for throttle-group
This adds a QAPI schema for the properties of the throttle-group object.
The only purpose of the x-* properties is to make the nested options in
'limits' available for a command line parser that doesn't support
structs. Any parser that will use the QAPI schema will supports structs,
though, so they will not be needed in the schema in the future.
To keep the conversion straightforward, add them to the schema anyway.
We can then remove the options and adjust documentation, test cases etc.
in a separate patch.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/block-core.json | 27 +++
qapi/qom.json| 7 +--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9f555d5c1d..a67fa0cc59 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2504,6 +2504,33 @@
'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
'*iops-size' : 'int' } }
+##
+# @ThrottleGroupProperties:
+#
+# Properties for throttle-group objects.
+#
+# The options starting with x- are aliases for the same key without x- in
+# the @limits object. As indicated by the x- prefix, this is not a stable
+# interface and may be removed or changed incompatibly in the future. Use
+# @limits for a supported stable interface.
+#
+# @limits: limits to apply for this throttle group
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleGroupProperties',
+ 'data': { '*limits': 'ThrottleLimits',
+'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
+'*x-iops-total-max-length' : 'int', '*x-iops-read' : 'int',
+'*x-iops-read-max' : 'int', '*x-iops-read-max-length' : 'int',
+'*x-iops-write' : 'int', '*x-iops-write-max' : 'int',
+'*x-iops-write-max-length' : 'int', '*x-bps-total' : 'int',
+'*x-bps-total-max' : 'int', '*x-bps-total-max-length' : 'int',
+'*x-bps-read' : 'int', '*x-bps-read-max' : 'int',
+'*x-bps-read-max-length' : 'int', '*x-bps-write' : 'int',
+'*x-bps-write-max' : 'int', '*x-bps-write-max-length' : 'int',
+'*x-iops-size' : 'int' } }
+
##
# @block-stream:
#
diff --git a/qapi/qom.json b/qapi/qom.json
index 6d3b8c4fe0..0721a636f9 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -5,6 +5,7 @@
# See the COPYING file in the top-level directory.
{ 'include': 'authz.json' }
+{ 'include': 'block-core.json' }
{ 'include': 'common.json' }
##
@@ -449,7 +450,8 @@
'memory-backend-ram',
'rng-builtin',
'rng-egd',
-'rng-random'
+'rng-random',
+'throttle-group'
] }
##
@@ -484,7 +486,8 @@
'memory-backend-ram': 'MemoryBackendProperties',
'rng-builtin':'RngProperties',
'rng-egd':'RngEgdProperties',
- 'rng-random': 'RngRandomProperties'
+ 'rng-random': 'RngRandomProperties',
+ 'throttle-group': 'ThrottleGroupProperties'
} }
##
--
2.29.2
[PATCH v3 21/30] qom: Remove user_creatable_add_dict()
This function is now unused and can be removed.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
include/qom/object_interfaces.h | 18 --
qom/object_interfaces.c | 32
2 files changed, 50 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 9b9938b8c0..5299603f50 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -86,24 +86,6 @@ Object *user_creatable_add_type(const char *type, const char
*id,
const QDict *qdict,
Visitor *v, Error **errp);
-/**
- * user_creatable_add_dict:
- * @qdict: the object definition
- * @keyval: if true, use a keyval visitor for processing @qdict (i.e.
- * assume that all @qdict values are strings); otherwise, use
- * the normal QObject visitor (i.e. assume all @qdict values
- * have the QType expected by the QOM object type)
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object that is defined by
- * @qdict. The object type is taken from the QDict key 'qom-type', its
- * ID from the key 'id'. The remaining entries in @qdict are used to
- * initialize the object properties.
- *
- * Returns: %true on success, %false on failure.
- */
-bool user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp);
-
/**
* user_creatable_add_opts:
* @opts: the object definition
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index d4df2334b7..02c3934329 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -113,38 +113,6 @@ out:
return obj;
}
-bool user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp)
-{
-Visitor *v;
-Object *obj;
-g_autofree char *type = NULL;
-g_autofree char *id = NULL;
-
-type = g_strdup(qdict_get_try_str(qdict, "qom-type"));
-if (!type) {
-error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-return false;
-}
-qdict_del(qdict, "qom-type");
-
-id = g_strdup(qdict_get_try_str(qdict, "id"));
-if (!id) {
-error_setg(errp, QERR_MISSING_PARAMETER, "id");
-return false;
-}
-qdict_del(qdict, "id");
-
-if (keyval) {
-v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
-} else {
-v = qobject_input_visitor_new(QOBJECT(qdict));
-}
-obj = user_creatable_add_type(type, id, qdict, v, errp);
-visit_free(v);
-object_unref(obj);
-return !!obj;
-}
-
Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
{
Visitor *v;
--
2.29.2
[PATCH v3 20/30] qemu-storage-daemon: Implement --object with qmp_object_add()
This QAPIfies --object and ensures that QMP and the command line option
behave the same.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
storage-daemon/qemu-storage-daemon.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index a1bcbacf05..4ab7e73053 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -38,6 +38,7 @@
#include "qapi/qapi-visit-block-core.h"
#include "qapi/qapi-visit-block-export.h"
#include "qapi/qapi-visit-control.h"
+#include "qapi/qapi-visit-qom.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qstring.h"
#include "qapi/qobject-input-visitor.h"
@@ -134,15 +135,6 @@ enum {
extern QemuOptsList qemu_chardev_opts;
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
static void init_qmp_commands(void)
{
qmp_init_marshal(_commands);
@@ -282,14 +274,22 @@ static void process_options(int argc, char *argv[])
{
QDict *args;
bool help;
+Visitor *v;
+ObjectOptions *options;
args = keyval_parse(optarg, "qom-type", , _fatal);
if (help) {
user_creatable_print_help_from_qdict(args);
exit(EXIT_SUCCESS);
}
-user_creatable_add_dict(args, true, _fatal);
+
+v = qobject_input_visitor_new_keyval(QOBJECT(args));
+visit_type_ObjectOptions(v, NULL, , _fatal);
+visit_free(v);
qobject_unref(args);
+
+qmp_object_add(options, _fatal);
+qapi_free_ObjectOptions(options);
break;
}
case OPTION_PIDFILE:
@@ -338,7 +338,6 @@ int main(int argc, char *argv[])
module_call_init(MODULE_INIT_QOM);
module_call_init(MODULE_INIT_TRACE);
-qemu_add_opts(_object_opts);
qemu_add_opts(_trace_opts);
qcrypto_init(_fatal);
bdrv_init();
--
2.29.2
[PATCH v3 19/30] qom: Make "object" QemuOptsList optional
This code is going away anyway, but for a few more commits, we'll be in
a state where some binaries still use QemuOpts and others don't. If the
"object" QemuOptsList doesn't even exist, we don't have to remove (or
fail to remove, and therefore abort) a user creatable object from it.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qom/object_interfaces.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 7661270b98..d4df2334b7 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -299,6 +299,7 @@ void user_creatable_print_help_from_qdict(QDict *args)
bool user_creatable_del(const char *id, Error **errp)
{
+QemuOptsList *opts_list;
Object *container;
Object *obj;
@@ -318,8 +319,10 @@ bool user_creatable_del(const char *id, Error **errp)
* if object was defined on the command-line, remove its corresponding
* option group entry
*/
-qemu_opts_del(qemu_opts_find(qemu_find_opts_err("object", _abort),
- id));
+opts_list = qemu_find_opts_err("object", NULL);
+if (opts_list) {
+qemu_opts_del(qemu_opts_find(opts_list, id));
+}
object_unparent(obj);
return true;
--
2.29.2
[PATCH v3 18/30] qapi/qom: QAPIfy object-add
This converts object-add from 'gen': false to the ObjectOptions QAPI
type. As an immediate benefit, clients can now use QAPI schema
introspection for user creatable QOM objects.
It is also the first step towards making the QAPI schema the only
external interface for the creation of user creatable objects. Once all
other places (HMP and command lines of the system emulator and all
tools) go through QAPI, too, some object implementations can be
simplified because some checks (e.g. that mandatory options are set) are
already performed by QAPI, and in another step, QOM boilerplate code
could be generated from the schema.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json| 11 +--
include/qom/object_interfaces.h | 7 ---
hw/block/xen-block.c | 16
monitor/misc.c | 2 --
qom/qom-qmp-cmds.c | 25 +++--
storage-daemon/qemu-storage-daemon.c | 2 --
6 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 0fd8563693..5b8a5da16f 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -844,13 +844,6 @@
#
# Create a QOM object.
#
-# @qom-type: the class name for the object to be created
-#
-# @id: the name of the new object
-#
-# Additional arguments depend on qom-type and are passed to the backend
-# unchanged.
-#
# Returns: Nothing on success
# Error if @qom-type is not a valid class name
#
@@ -864,9 +857,7 @@
# <- { "return": {} }
#
##
-{ 'command': 'object-add',
- 'data': {'qom-type': 'str', 'id': 'str'},
- 'gen': false } # so we can get the additional arguments
+{ 'command': 'object-add', 'data': 'ObjectOptions', 'boxed': true }
##
# @object-del:
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 07d5cc8832..9b9938b8c0 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -196,11 +196,4 @@ bool user_creatable_del(const char *id, Error **errp);
*/
void user_creatable_cleanup(void);
-/**
- * qmp_object_add:
- *
- * QMP command handler for object-add. See the QAPI schema for documentation.
- */
-void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp);
-
#endif
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a3b69e2709..ac82d54063 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -836,17 +836,17 @@ static XenBlockIOThread *xen_block_iothread_create(const
char *id,
{
ERRP_GUARD();
XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
-QDict *opts;
-QObject *ret_data = NULL;
+ObjectOptions *opts;
iothread->id = g_strdup(id);
-opts = qdict_new();
-qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
-qdict_put_str(opts, "id", id);
-qmp_object_add(opts, _data, errp);
-qobject_unref(opts);
-qobject_unref(ret_data);
+opts = g_new(ObjectOptions, 1);
+*opts = (ObjectOptions) {
+.qom_type = OBJECT_TYPE_IOTHREAD,
+.id = g_strdup(id),
+};
+qmp_object_add(opts, errp);
+qapi_free_ObjectOptions(opts);
if (*errp) {
g_free(iothread->id);
diff --git a/monitor/misc.c b/monitor/misc.c
index a7650ed747..42efd9e2ab 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -235,8 +235,6 @@ static void monitor_init_qmp_commands(void)
qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
qmp_register_command(_commands, "device_add", qmp_device_add,
QCO_NO_OPTIONS);
-qmp_register_command(_commands, "object-add", qmp_object_add,
- QCO_NO_OPTIONS);
QTAILQ_INIT(_cap_negotiation_commands);
qmp_register_command(_cap_negotiation_commands, "qmp_capabilities",
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 19fd5e117f..e577a96adf 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -19,8 +19,11 @@
#include "qapi/error.h"
#include "qapi/qapi-commands-qdev.h"
#include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-visit-qom.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
#include "qemu/cutils.h"
#include "qom/object_interfaces.h"
#include "qom/qom-qobject.h"
@@ -223,9 +226,27 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char
*typename,
return prop_list;
}
-void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
+void qmp_object_add(ObjectOptions *options, Error **errp)
{
-user_creatable_add_dict(qdict, false, errp);
+Visitor *v;
+QObject *qobj;
+QDict *props;
+Object *obj;
+
+v = qobject_output_visitor_new();
+visit_type_ObjectOptions
[PATCH v3 17/30] qapi/qom: Add ObjectOptions for x-remote-object
This adds a QAPI schema for the properties of the x-remote-object
object.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 6b96e9b0b3..0fd8563693 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -644,6 +644,20 @@
{ 'struct': 'PrManagerHelperProperties',
'data': { 'path': 'str' } }
+##
+# @RemoteObjectProperties:
+#
+# Properties for x-remote-object objects.
+#
+# @fd: file descriptor name previously passed via 'getfd' command
+#
+# @devid: the id of the device to be associated with the file descriptor
+#
+# Since: 6.0
+##
+{ 'struct': 'RemoteObjectProperties',
+ 'data': { 'fd': 'str', 'devid': 'str' } }
+
##
# @RngProperties:
#
@@ -765,7 +779,8 @@
'tls-creds-anon',
'tls-creds-psk',
'tls-creds-x509',
-'tls-cipher-suites'
+'tls-cipher-suites',
+'x-remote-object'
] }
##
@@ -820,7 +835,8 @@
'tls-creds-anon': 'TlsCredsAnonProperties',
'tls-creds-psk': 'TlsCredsPskProperties',
'tls-creds-x509': 'TlsCredsX509Properties',
- 'tls-cipher-suites': 'TlsCredsProperties'
+ 'tls-cipher-suites': 'TlsCredsProperties',
+ 'x-remote-object':'RemoteObjectProperties'
} }
##
--
2.29.2
[PATCH v3 15/30] qapi/qom: Add ObjectOptions for confidential-guest-support
This adds a QAPI schema for the properties of the objects implementing
the confidential-guest-support interface.
pef-guest and s390x-pv-guest don't have any properties, so they only
need to be added to the ObjectType enum without adding a new branch to
ObjectOptions.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 37 +
1 file changed, 37 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 6afac9169f..ad72dbdec2 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -635,6 +635,38 @@
'base': 'RngProperties',
'data': { '*filename': 'str' } }
+##
+# @SevGuestProperties:
+#
+# Properties for sev-guest objects.
+#
+# @sev-device: SEV device to use (default: "/dev/sev")
+#
+# @dh-cert-file: guest owners DH certificate (encoded with base64)
+#
+# @session-file: guest owners session parameters (encoded with base64)
+#
+# @policy: SEV policy value (default: 0x1)
+#
+# @handle: SEV firmware handle (default: 0)
+#
+# @cbitpos: C-bit location in page table entry (default: 0)
+#
+# @reduced-phys-bits: number of bits in physical addresses that become
+# unavailable when SEV is enabled
+#
+# Since: 2.12
+##
+{ 'struct': 'SevGuestProperties',
+ 'data': { '*sev-device': 'str',
+'*dh-cert-file': 'str',
+'*session-file': 'str',
+'*policy': 'uint32',
+'*handle': 'uint32',
+'*cbitpos': 'uint32',
+'reduced-phys-bits': 'uint32' },
+ 'if': 'defined(CONFIG_SEV)' }
+
##
# @ObjectType:
#
@@ -663,12 +695,15 @@
'memory-backend-file',
'memory-backend-memfd',
'memory-backend-ram',
+{'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
'pr-manager-helper',
'rng-builtin',
'rng-egd',
'rng-random',
'secret',
'secret_keyring',
+{'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
+'s390-pv-guest',
'throttle-group',
'tls-creds-anon',
'tls-creds-psk',
@@ -720,6 +755,8 @@
'rng-random': 'RngRandomProperties',
'secret': 'SecretProperties',
'secret_keyring': 'SecretKeyringProperties',
+ 'sev-guest': { 'type': 'SevGuestProperties',
+ 'if': 'defined(CONFIG_SEV)' },
'throttle-group': 'ThrottleGroupProperties',
'tls-creds-anon': 'TlsCredsAnonProperties',
'tls-creds-psk': 'TlsCredsPskProperties',
--
2.29.2
[PATCH v3 14/30] qapi/qom: Add ObjectOptions for pr-manager-helper
This adds a QAPI schema for the properties of the pr-manager-helper
object.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 14 ++
1 file changed, 14 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 6fe775bd83..6afac9169f 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -577,6 +577,18 @@
'*hugetlbsize': 'size',
'*seal': 'bool' } }
+##
+# @PrManagerHelperProperties:
+#
+# Properties for pr-manager-helper objects.
+#
+# @path: the path to a Unix domain socket for connecting to the external helper
+#
+# Since: 2.11
+##
+{ 'struct': 'PrManagerHelperProperties',
+ 'data': { 'path': 'str' } }
+
##
# @RngProperties:
#
@@ -651,6 +663,7 @@
'memory-backend-file',
'memory-backend-memfd',
'memory-backend-ram',
+'pr-manager-helper',
'rng-builtin',
'rng-egd',
'rng-random',
@@ -701,6 +714,7 @@
'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
'if': 'defined(CONFIG_LINUX)' },
'memory-backend-ram': 'MemoryBackendProperties',
+ 'pr-manager-helper': 'PrManagerHelperProperties',
'rng-builtin':'RngProperties',
'rng-egd':'RngEgdProperties',
'rng-random': 'RngRandomProperties',
--
2.29.2
[PATCH v3 13/30] qapi/qom: Add ObjectOptions for filter-*
This adds a QAPI schema for the properties of the filter-* objects.
Some parts of the interface (in particular NetfilterProperties.position)
are very unusual for QAPI, but for now just describe the existing
interface.
net.json can't be included in qom.json because the storage daemon
doesn't have it. NetFilterDirection is still required in the new object
property definitions in qom.json, so move this enum to common.json.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/common.json | 20 +++
qapi/net.json| 20 ---
qapi/qom.json| 143 +++
3 files changed, 163 insertions(+), 20 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index 2dad4fadc3..b87e7f9039 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -165,3 +165,23 @@
##
{ 'enum': 'HostMemPolicy',
'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
+
+##
+# @NetFilterDirection:
+#
+# Indicates whether a netfilter is attached to a netdev's transmit queue or
+# receive queue or both.
+#
+# @all: the filter is attached both to the receive and the transmit
+# queue of the netdev (default).
+#
+# @rx: the filter is attached to the receive queue of the netdev,
+# where it will receive packets sent to the netdev.
+#
+# @tx: the filter is attached to the transmit queue of the netdev,
+# where it will receive packets sent by the netdev.
+#
+# Since: 2.5
+##
+{ 'enum': 'NetFilterDirection',
+ 'data': [ 'all', 'rx', 'tx' ] }
diff --git a/qapi/net.json b/qapi/net.json
index c31748c87f..af3f5b0fda 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -492,26 +492,6 @@
'vhost-user': 'NetdevVhostUserOptions',
'vhost-vdpa': 'NetdevVhostVDPAOptions' } }
-##
-# @NetFilterDirection:
-#
-# Indicates whether a netfilter is attached to a netdev's transmit queue or
-# receive queue or both.
-#
-# @all: the filter is attached both to the receive and the transmit
-# queue of the netdev (default).
-#
-# @rx: the filter is attached to the receive queue of the netdev,
-# where it will receive packets sent to the netdev.
-#
-# @tx: the filter is attached to the transmit queue of the netdev,
-# where it will receive packets sent by the netdev.
-#
-# Since: 2.5
-##
-{ 'enum': 'NetFilterDirection',
- 'data': [ 'all', 'rx', 'tx' ] }
-
##
# @RxState:
#
diff --git a/qapi/qom.json b/qapi/qom.json
index a34ae43cb9..6fe775bd83 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -313,6 +313,137 @@
'data': { 'addr': 'str' ,
'*id-list': 'str' } }
+##
+# @NetfilterInsert:
+#
+# Indicates where to insert a netfilter relative to a given other filter.
+#
+# @before: insert before the specified filter
+#
+# @behind: insert behind the specified filter
+#
+# Since: 5.0
+##
+{ 'enum': 'NetfilterInsert',
+ 'data': [ 'before', 'behind' ] }
+
+##
+# @NetfilterProperties:
+#
+# Properties for objects of classes derived from netfilter.
+#
+# @netdev: id of the network device backend to filter
+#
+# @queue: indicates which queue(s) to filter (default: all)
+#
+# @status: indicates whether the filter is enabled ("on") or disabled ("off")
+# (default: "on")
+#
+# @position: specifies where the filter should be inserted in the filter list.
+#"head" means the filter is inserted at the head of the filter
list,
+#before any existing filters.
+#"tail" means the filter is inserted at the tail of the filter
list,
+#behind any existing filters (default).
+#"id=" means the filter is inserted before or behind the filter
+#specified by , depending on the @insert property.
+#(default: "tail")
+#
+# @insert: where to insert the filter relative to the filter given in
@position.
+# Ignored if @position is "head" or "tail". (default: behind)
+#
+# Since: 2.5
+##
+{ 'struct': 'NetfilterProperties',
+ 'data': { 'netdev': 'str',
+'*queue': 'NetFilterDirection',
+'*status': 'str',
+'*position': 'str',
+'*insert': 'NetfilterInsert' } }
+
+##
+# @FilterBufferProperties:
+#
+# Properties for filter-buffer objects.
+#
+# @interval: a non-zero interval in microseconds. All packets arriving in the
+#given interval are delayed until the end of the interval.
+#
+# Since: 2.5
+##
+{ 'struct': 'FilterBufferProperties',
+ 'base': 'NetfilterProperties',
+ 'data': { 'interval': 'uint32' } }
+
+##
+# @FilterDumpProperties:
+#
+# Properties for filter-dump objects.
+#
+# @file: the filename where the dumped packets should be stored
+#
+# @maxlen: maximum number of bytes in a packet that are stored (default: 65536)
+#
+# Since: 2.5
+##
+{ 'struct': 'FilterDumpProperties',
+ 'base': 'NetfilterProperties',
+ 'data': { 'file': 'str',
+'*maxlen': 'uint32' } }
+
+##
+# @FilterMirrorProp
[PATCH v3 12/30] qapi/qom: Add ObjectOptions for colo-compare
This adds a QAPI schema for the properties of the colo-compare object.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 49 +
1 file changed, 49 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index fd87896bca..a34ae43cb9 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -222,6 +222,53 @@
'data': { 'if': 'str',
'canbus': 'str' } }
+##
+# @ColoCompareProperties:
+#
+# Properties for colo-compare objects.
+#
+# @primary_in: name of the character device backend to use for the primary
+# input (incoming packets are redirected to @outdev)
+#
+# @secondary_in: name of the character device backend to use for secondary
+#input (incoming packets are only compared to the input on
+#@primary_in and then dropped)
+#
+# @outdev: name of the character device backend to use for output
+#
+# @iothread: name of the iothread to run in
+#
+# @notify_dev: name of the character device backend to be used to communicate
+# with the remote colo-frame (only for Xen COLO)
+#
+# @compare_timeout: the maximum time to hold a packet from @primary_in for
+# comparison with an incoming packet on @secondary_in in
+# milliseconds (default: 3000)
+#
+# @expired_scan_cycle: the interval at which colo-compare checks whether
+# packets from @primary have timed out, in milliseconds
+# (default: 3000)
+#
+# @max_queue_size: the maximum number of packets to keep in the queue for
+# comparing with incoming packets from @secondary_in. If the
+# queue is full and addtional packets are received, the
+# addtional packets are dropped. (default: 1024)
+#
+# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
+#
+# Since: 2.8
+##
+{ 'struct': 'ColoCompareProperties',
+ 'data': { 'primary_in': 'str',
+'secondary_in': 'str',
+'outdev': 'str',
+'iothread': 'str',
+'*notify_dev': 'str',
+'*compare_timeout': 'uint64',
+'*expired_scan_cycle': 'uint32',
+'*max_queue_size': 'uint32',
+'*vnet_hdr_support': 'bool' } }
+
##
# @CryptodevBackendProperties:
#
@@ -458,6 +505,7 @@
'authz-simple',
'can-bus',
'can-host-socketcan',
+'colo-compare',
'cryptodev-backend',
'cryptodev-backend-builtin',
'cryptodev-vhost-user',
@@ -499,6 +547,7 @@
'authz-pam': 'AuthZPAMProperties',
'authz-simple': 'AuthZSimpleProperties',
'can-host-socketcan': 'CanHostSocketcanProperties',
+ 'colo-compare': 'ColoCompareProperties',
'cryptodev-backend': 'CryptodevBackendProperties',
'cryptodev-backend-builtin': 'CryptodevBackendProperties',
'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
--
2.29.2
[PATCH v3 11/30] qapi/qom: Add ObjectOptions for can-*
This adds a QAPI schema for the properties of the can-* objects.
can-bus doesn't have any properties, so it only needs to be added to the
ObjectType enum without adding a new branch to ObjectOptions.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 18 ++
1 file changed, 18 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 512d8fce12..fd87896bca 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -207,6 +207,21 @@
'returns': [ 'ObjectPropertyInfo' ],
'allow-preconfig': true }
+##
+# @CanHostSocketcanProperties:
+#
+# Properties for can-host-socketcan objects.
+#
+# @if: interface name of the host system CAN bus to connect to
+#
+# @canbus: object ID of the can-bus object to connect to the host interface
+#
+# Since: 2.12
+##
+{ 'struct': 'CanHostSocketcanProperties',
+ 'data': { 'if': 'str',
+'canbus': 'str' } }
+
##
# @CryptodevBackendProperties:
#
@@ -441,6 +456,8 @@
'authz-listfile',
'authz-pam',
'authz-simple',
+'can-bus',
+'can-host-socketcan',
'cryptodev-backend',
'cryptodev-backend-builtin',
'cryptodev-vhost-user',
@@ -481,6 +498,7 @@
'authz-listfile': 'AuthZListFileProperties',
'authz-pam': 'AuthZPAMProperties',
'authz-simple': 'AuthZSimpleProperties',
+ 'can-host-socketcan': 'CanHostSocketcanProperties',
'cryptodev-backend': 'CryptodevBackendProperties',
'cryptodev-backend-builtin': 'CryptodevBackendProperties',
'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
--
2.29.2
[PATCH v3 10/30] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'
This adds a QAPI schema for the properties of the tls-* objects.
The 'loaded' property doesn't seem to make sense as an external
interface: It is automatically set to true in ucc->complete, and
explicitly setting it to true earlier just means that additional options
will be silently ignored.
In other words, the 'loaded' property is useless. Mark it as deprecated
in the schema from the start.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/crypto.json | 98
qapi/qom.json| 12 +-
2 files changed, 108 insertions(+), 2 deletions(-)
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 0fef3de66d..7116ae9a46 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -442,3 +442,101 @@
{ 'struct': 'SecretKeyringProperties',
'base': 'SecretCommonProperties',
'data': { 'serial': 'int32' } }
+
+##
+# @TlsCredsProperties:
+#
+# Properties for objects of classes derived from tls-creds.
+#
+# @verify-peer: if true the peer credentials will be verified once the
+# handshake is completed. This is a no-op for anonymous
+# credentials. (default: true)
+#
+# @dir: the path of the directory that contains the credential files
+#
+# @endpoint: whether the QEMU network backend that uses the credentials will be
+#acting as a client or as a server (default: client)
+#
+# @priority: a gnutls priority string as described at
+#https://gnutls.org/manual/html_node/Priority-Strings.html
+#
+# Since: 2.5
+##
+{ 'struct': 'TlsCredsProperties',
+ 'data': { '*verify-peer': 'bool',
+'*dir': 'str',
+'*endpoint': 'QCryptoTLSCredsEndpoint',
+'*priority': 'str' } }
+
+##
+# @TlsCredsAnonProperties:
+#
+# Properties for tls-creds-anon objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+# option and will ignore options that are processed later. Don't use;
+# only provided for compatibility. (default: false)
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 2.5
+##
+{ 'struct': 'TlsCredsAnonProperties',
+ 'base': 'TlsCredsProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] } } }
+
+##
+# @TlsCredsPskProperties:
+#
+# Properties for tls-creds-psk objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+# option and will ignore options that are processed later. Don't use;
+# only provided for compatibility. (default: false)
+#
+# @username: the username which will be sent to the server. For clients only.
+#If absent, "qemu" is sent and the property will read back as an
+#empty string.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 3.0
+##
+{ 'struct': 'TlsCredsPskProperties',
+ 'base': 'TlsCredsProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+'*username': 'str' } }
+
+##
+# @TlsCredsX509Properties:
+#
+# Properties for tls-creds-x509 objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+# option and will ignore options that are processed later. Don't use;
+# only provided for compatibility. (default: false)
+#
+# @sanity-check: if true, perform some sanity checks before using the
+#credentials (default: true)
+#
+# @passwordid: For the server-key.pem and client-key.pem files which contain
+# sensitive private keys, it is possible to use an encrypted
+# version by providing the @passwordid parameter. This provides
+# the ID of a previously created secret object containing the
+# password for decryption.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 2.5
+##
+{ 'struct': 'TlsCredsX509Properties',
+ 'base': 'TlsCredsProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+'*sanity-check': 'bool',
+'*passwordid': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index e4bbddd986..512d8fce12 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -454,7 +454,11 @@
'rng-random',
'secret',
'secret_keyring',
-'throttle-group'
+'throttle-group',
+'tls-creds-anon',
+'tls-creds-psk',
+'tls-creds-x509',
+'tls-cipher-suites'
] }
##
@@ -492,7 +496,11 @@
'rng-random': 'RngRandomProperties',
'secret': 'SecretProperties',
'secret_keyring': 'SecretKeyringProperties',
- 'throttle-group': 'ThrottleGroupProperties'
+ 't
[PATCH v3 09/30] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'
This adds a QAPI schema for the properties of the secret* objects.
The 'loaded' property doesn't seem to make sense as an external
interface: It is automatically set to true in ucc->complete, and
explicitly setting it to true earlier just means that additional options
will be silently ignored.
In other words, the 'loaded' property is useless. Mark it as deprecated
in the schema from the start.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/crypto.json | 61 ++
qapi/qom.json | 5
docs/system/deprecated.rst | 11 +++
3 files changed, 77 insertions(+)
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 2aebe6fa20..0fef3de66d 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -381,3 +381,64 @@
'discriminator': 'format',
'data': {
'luks': 'QCryptoBlockAmendOptionsLUKS' } }
+
+##
+# @SecretCommonProperties:
+#
+# Properties for objects of classes derived from secret-common.
+#
+# @loaded: if true, the secret is loaded immediately when applying this option
+# and will probably fail when processing the next option. Don't use;
+# only provided for compatibility. (default: false)
+#
+# @format: the data format that the secret is provided in (default: raw)
+#
+# @keyid: the name of another secret that should be used to decrypt the
+# provided data. If not present, the data is assumed to be unencrypted.
+#
+# @iv: the random initialization vector used for encryption of this particular
+# secret. Should be a base64 encrypted string of the 16-byte IV. Mandatory
+# if @keyid is given. Ignored if @keyid is absent.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 2.6
+##
+{ 'struct': 'SecretCommonProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+'*format': 'QCryptoSecretFormat',
+'*keyid': 'str',
+'*iv': 'str' } }
+
+##
+# @SecretProperties:
+#
+# Properties for secret objects.
+#
+# Either @data or @file must be provided, but not both.
+#
+# @data: the associated with the secret from
+#
+# @file: the filename to load the data associated with the secret from
+#
+# Since: 2.6
+##
+{ 'struct': 'SecretProperties',
+ 'base': 'SecretCommonProperties',
+ 'data': { '*data': 'str',
+'*file': 'str' } }
+
+##
+# @SecretKeyringProperties:
+#
+# Properties for secret_keyring objects.
+#
+# @serial: serial number that identifies a key to get from the kernel
+#
+# Since: 5.1
+##
+{ 'struct': 'SecretKeyringProperties',
+ 'base': 'SecretCommonProperties',
+ 'data': { 'serial': 'int32' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 0721a636f9..e4bbddd986 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -7,6 +7,7 @@
{ 'include': 'authz.json' }
{ 'include': 'block-core.json' }
{ 'include': 'common.json' }
+{ 'include': 'crypto.json' }
##
# = QEMU Object Model (QOM)
@@ -451,6 +452,8 @@
'rng-builtin',
'rng-egd',
'rng-random',
+'secret',
+'secret_keyring',
'throttle-group'
] }
@@ -487,6 +490,8 @@
'rng-builtin':'RngProperties',
'rng-egd':'RngEgdProperties',
'rng-random': 'RngRandomProperties',
+ 'secret': 'SecretProperties',
+ 'secret_keyring': 'SecretKeyringProperties',
'throttle-group': 'ThrottleGroupProperties'
} }
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 3dac79f600..f4e8226963 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -162,6 +162,17 @@ other options have been processed. This will either have
no effect (if
``opened`` was the last option) or cause errors. The property is therefore
useless and should not be specified.
+``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)
+''
+
+The only effect of specifying ``loaded=on`` in the command line or QMP
+``object-add`` is that the secret is loaded immediately, possibly before all
+other options have been processed. This will either have no effect (if
+``loaded`` was the last option) or cause options to be effectively ignored as
+if they were not given. The property is therefore useless and should not be
+specified.
+
+
QEMU Machine Protocol (QMP) commands
--
2.29.2
[PATCH v3 07/30] qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened'
This adds a QAPI schema for the properties of the rng-* objects.
The 'opened' property doesn't seem to make sense as an external
interface: It is automatically set to true in ucc->complete, and
explicitly setting it to true earlier just means that trying to set
additional options will result in an error. After the property has once
been set to true (i.e. when the object construction has completed), it
can never be reset to false. In other words, the 'opened' property is
useless. Mark it as deprecated in the schema from the start.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 56 --
docs/system/deprecated.rst | 9 ++
2 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 8c0e06c198..6d3b8c4fe0 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -382,6 +382,52 @@
'*hugetlbsize': 'size',
'*seal': 'bool' } }
+##
+# @RngProperties:
+#
+# Properties for objects of classes derived from rng.
+#
+# @opened: if true, the device is opened immediately when applying this option
+# and will probably fail when processing the next option. Don't use;
+# only provided for compatibility. (default: false)
+#
+# Features:
+# @deprecated: Member @opened is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 1.3
+##
+{ 'struct': 'RngProperties',
+ 'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
+
+##
+# @RngEgdProperties:
+#
+# Properties for rng-egd objects.
+#
+# @chardev: the name of a character device backend that provides the connection
+# to the RNG daemon
+#
+# Since: 1.3
+##
+{ 'struct': 'RngEgdProperties',
+ 'base': 'RngProperties',
+ 'data': { 'chardev': 'str' } }
+
+##
+# @RngRandomProperties:
+#
+# Properties for rng-random objects.
+#
+# @filename: the filename of the device on the host to obtain entropy from
+#(default: "/dev/urandom")
+#
+# Since: 1.3
+##
+{ 'struct': 'RngRandomProperties',
+ 'base': 'RngProperties',
+ 'data': { '*filename': 'str' } }
+
##
# @ObjectType:
#
@@ -400,7 +446,10 @@
'iothread',
'memory-backend-file',
'memory-backend-memfd',
-'memory-backend-ram'
+'memory-backend-ram',
+'rng-builtin',
+'rng-egd',
+'rng-random'
] }
##
@@ -432,7 +481,10 @@
'memory-backend-file':'MemoryBackendFileProperties',
'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
'if': 'defined(CONFIG_LINUX)' },
- 'memory-backend-ram': 'MemoryBackendProperties'
+ 'memory-backend-ram': 'MemoryBackendProperties',
+ 'rng-builtin':'RngProperties',
+ 'rng-egd':'RngEgdProperties',
+ 'rng-random': 'RngRandomProperties'
} }
##
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 893f3e8579..3dac79f600 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -153,6 +153,15 @@ The ``-writeconfig`` option is not able to serialize the
entire contents
of the QEMU command line. It is thus considered a failed experiment
and deprecated, with no current replacement.
+``opened`` property of ``rng-*`` objects (since 6.0.0)
+''
+
+The only effect of specifying ``opened=on`` in the command line or QMP
+``object-add`` is that the device is opened immediately, possibly before all
+other options have been processed. This will either have no effect (if
+``opened`` was the last option) or cause errors. The property is therefore
+useless and should not be specified.
+
QEMU Machine Protocol (QMP) commands
--
2.29.2
[PATCH v3 06/30] qapi/qom: Add ObjectOptions for memory-backend-*
This adds a QAPI schema for the properties of the memory-backend-*
objects.
HostMemPolicy has to be moved to an include file that can be used by the
storage daemon, too, because ObjectOptions must be the same in all
binaries if we don't want to compile the whole code multiple times.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
---
qapi/common.json | 20
qapi/machine.json | 22 +
qapi/qom.json | 121 +-
3 files changed, 141 insertions(+), 22 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index 716712d4b3..2dad4fadc3 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -145,3 +145,23 @@
##
{ 'enum': 'PCIELinkWidth',
'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
+
+##
+# @HostMemPolicy:
+#
+# Host memory policy types
+#
+# @default: restore default policy, remove any nondefault policy
+#
+# @preferred: set the preferred host nodes for allocation
+#
+# @bind: a strict policy that restricts memory allocation to the
+#host nodes specified
+#
+# @interleave: memory allocations are interleaved across the set
+# of host nodes specified
+#
+# Since: 2.1
+##
+{ 'enum': 'HostMemPolicy',
+ 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
diff --git a/qapi/machine.json b/qapi/machine.json
index 330189efe3..4322aee782 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -8,6 +8,8 @@
# = Machines
##
+{ 'include': 'common.json' }
+
##
# @SysEmuTarget:
#
@@ -897,26 +899,6 @@
'policy': 'HmatCacheWritePolicy',
'line': 'uint16' }}
-##
-# @HostMemPolicy:
-#
-# Host memory policy types
-#
-# @default: restore default policy, remove any nondefault policy
-#
-# @preferred: set the preferred host nodes for allocation
-#
-# @bind: a strict policy that restricts memory allocation to the
-#host nodes specified
-#
-# @interleave: memory allocations are interleaved across the set
-# of host nodes specified
-#
-# Since: 2.1
-##
-{ 'enum': 'HostMemPolicy',
- 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
-
##
# @memsave:
#
diff --git a/qapi/qom.json b/qapi/qom.json
index 942654e05c..8c0e06c198 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -5,6 +5,7 @@
# See the COPYING file in the top-level directory.
{ 'include': 'authz.json' }
+{ 'include': 'common.json' }
##
# = QEMU Object Model (QOM)
@@ -272,6 +273,115 @@
'*poll-grow': 'int',
'*poll-shrink': 'int' } }
+##
+# @MemoryBackendProperties:
+#
+# Properties for objects of classes derived from memory-backend.
+#
+# @merge: if true, mark the memory as mergeable (default depends on the machine
+# type)
+#
+# @dump: if true, include the memory in core dumps (default depends on the
+#machine type)
+#
+# @host-nodes: the list of NUMA host nodes to bind the memory to
+#
+# @policy: the NUMA policy (default: 'default')
+#
+# @prealloc: if true, preallocate memory (default: false)
+#
+# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
+#
+# @share: if false, the memory is private to QEMU; if true, it is shared
+# (default: false)
+#
+# @size: size of the memory region in bytes
+#
+# @x-use-canonical-path-for-ramblock-id: if true, the canoncial path is used
+#for ramblock-id. Disable this for 4.0
+#machine types or older to allow
+#migration with newer QEMU versions.
+#This option is considered stable
+#despite the x- prefix. (default:
+#false generally, but true for machine
+#types <= 4.0)
+#
+# Since: 2.1
+##
+{ 'struct': 'MemoryBackendProperties',
+ 'data': { '*dump': 'bool',
+'*host-nodes': ['uint16'],
+'*merge': 'bool',
+'*policy': 'HostMemPolicy',
+'*prealloc': 'bool',
+'*prealloc-threads': 'uint32',
+'*share': 'bool',
+'size': 'size',
+'*x-use-canonical-path-for-ramblock-id': 'bool' } }
+
+##
+# @MemoryBackendFileProperties:
+#
+# Properties for memory-backend-file objects.
+#
+# @align: the base address alignment when QEMU mmap(2)s @mem-path. Some
+# backend stores specified by @mem-path require an alignment different
+# than the default one used by QEMU, e.g. the device DAX /dev/dax0.0
+# requires 2M alignment rather than 4K. In such cases, users can
+# specify the required alignment via this option.
+# 0 selects a default alignment (currently the page size). (default: 0)
+#
+# @discard-data: if true, the file contents can be destroyed when QEMU exits,
+#to avoid unnecessarily flushing data to the backing file. Note
+#that ``discard-data`` is only an optimizat
[PATCH v3 05/30] qapi/qom: Add ObjectOptions for dbus-vmstate
This adds a QAPI schema for the properties of the dbus-vmstate object.
A list represented as a comma separated string is clearly not very
QAPI-like, but for now just describe the existing interface.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 18 ++
1 file changed, 18 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 46c2cdc6cf..942654e05c 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -232,6 +232,22 @@
'base': 'CryptodevBackendProperties',
'data': { 'chardev': 'str' } }
+##
+# @DBusVMStateProperties:
+#
+# Properties for dbus-vmstate objects.
+#
+# @addr: the name of the DBus bus to connect to
+#
+# @id-list: a comma separated list of DBus IDs of helpers whose data should be
+# included in the VM state on migration
+#
+# Since: 5.0
+##
+{ 'struct': 'DBusVMStateProperties',
+ 'data': { 'addr': 'str' ,
+'*id-list': 'str' } }
+
##
# @IothreadProperties:
#
@@ -270,6 +286,7 @@
'cryptodev-backend',
'cryptodev-backend-builtin',
'cryptodev-vhost-user',
+'dbus-vmstate',
'iothread'
] }
@@ -297,6 +314,7 @@
'cryptodev-backend-builtin': 'CryptodevBackendProperties',
'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
'if': 'defined(CONFIG_VIRTIO_CRYPTO) &&
defined(CONFIG_VHOST_CRYPTO)' },
+ 'dbus-vmstate': 'DBusVMStateProperties',
'iothread': 'IothreadProperties'
} }
--
2.29.2
[PATCH v3 04/30] qapi/qom: Add ObjectOptions for cryptodev-*
This adds a QAPI schema for the properties of the cryptodev-* objects.
These interfaces have some questionable aspects (cryptodev-backend is
really an abstract base class without function, and the queues option
only makes sense for cryptodev-vhost-user), but as the goal is to
represent the existing interface in QAPI, leave these things in place.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
---
qapi/qom.json | 35 +++
1 file changed, 35 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 30ed179bc1..46c2cdc6cf 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -204,6 +204,34 @@
'returns': [ 'ObjectPropertyInfo' ],
'allow-preconfig': true }
+##
+# @CryptodevBackendProperties:
+#
+# Properties for cryptodev-backend and cryptodev-backend-builtin objects.
+#
+# @queues: the number of queues for the cryptodev backend. Ignored for
+# cryptodev-backend and must be 1 for cryptodev-backend-builtin.
+# (default: 1)
+#
+# Since: 2.8
+##
+{ 'struct': 'CryptodevBackendProperties',
+ 'data': { '*queues': 'uint32' } }
+
+##
+# @CryptodevVhostUserProperties:
+#
+# Properties for cryptodev-vhost-user objects.
+#
+# @chardev: the name of a Unix domain socket character device that connects to
+# the vhost-user server
+#
+# Since: 2.12
+##
+{ 'struct': 'CryptodevVhostUserProperties',
+ 'base': 'CryptodevBackendProperties',
+ 'data': { 'chardev': 'str' } }
+
##
# @IothreadProperties:
#
@@ -239,6 +267,9 @@
'authz-listfile',
'authz-pam',
'authz-simple',
+'cryptodev-backend',
+'cryptodev-backend-builtin',
+'cryptodev-vhost-user',
'iothread'
] }
@@ -262,6 +293,10 @@
'authz-listfile': 'AuthZListFileProperties',
'authz-pam': 'AuthZPAMProperties',
'authz-simple': 'AuthZSimpleProperties',
+ 'cryptodev-backend': 'CryptodevBackendProperties',
+ 'cryptodev-backend-builtin': 'CryptodevBackendProperties',
+ 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
+ 'if': 'defined(CONFIG_VIRTIO_CRYPTO) &&
defined(CONFIG_VHOST_CRYPTO)' },
'iothread': 'IothreadProperties'
} }
--
2.29.2
[PATCH v3 03/30] qapi/qom: Add ObjectOptions for authz-*
This adds a QAPI schema for the properties of the authz-* objects.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/authz.json | 61 +---
qapi/qom.json| 10 +
storage-daemon/qapi/qapi-schema.json | 1 +
3 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/qapi/authz.json b/qapi/authz.json
index 42afe752d1..51845e37cc 100644
--- a/qapi/authz.json
+++ b/qapi/authz.json
@@ -50,12 +50,63 @@
'*format': 'QAuthZListFormat'}}
##
-# @QAuthZListRuleListHack:
+# @AuthZListProperties:
#
-# Not exposed via QMP; hack to generate QAuthZListRuleList
-# for use internally by the code.
+# Properties for authz-list objects.
+#
+# @policy: Default policy to apply when no rule matches (default: deny)
+#
+# @rules: Authorization rules based on matching user
+#
+# Since: 4.0
+##
+{ 'struct': 'AuthZListProperties',
+ 'data': { '*policy': 'QAuthZListPolicy',
+'*rules': ['QAuthZListRule'] } }
+
+##
+# @AuthZListFileProperties:
+#
+# Properties for authz-listfile objects.
+#
+# @filename: File name to load the configuration from. The file must
+#contain valid JSON for AuthZListProperties.
+#
+# @refresh: If true, inotify is used to monitor the file, automatically
+# reloading changes. If an error occurs during reloading, all
+# authorizations will fail until the file is next successfully
+# loaded. (default: true if the binary was built with
+# CONFIG_INOTIFY1, false otherwise)
+#
+# Since: 4.0
+##
+{ 'struct': 'AuthZListFileProperties',
+ 'data': { 'filename': 'str',
+'*refresh': 'bool' } }
+
+##
+# @AuthZPAMProperties:
+#
+# Properties for authz-pam objects.
+#
+# @service: PAM service name to use for authorization
+#
+# Since: 4.0
+##
+{ 'struct': 'AuthZPAMProperties',
+ 'data': { 'service': 'str' } }
+
+##
+# @AuthZSimpleProperties:
+#
+# Properties for authz-simple objects.
+#
+# @identity: Identifies the allowed user. Its format depends on the network
+#service that authorization object is associated with. For
+#authorizing based on TLS x509 certificates, the identity must be
+#the x509 distinguished name.
#
# Since: 4.0
##
-{ 'struct': 'QAuthZListRuleListHack',
- 'data': { 'unused': ['QAuthZListRule'] } }
+{ 'struct': 'AuthZSimpleProperties',
+ 'data': { 'identity': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index bf2ecb34be..30ed179bc1 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -4,6 +4,8 @@
# This work is licensed under the terms of the GNU GPL, version 2 or later.
# See the COPYING file in the top-level directory.
+{ 'include': 'authz.json' }
+
##
# = QEMU Object Model (QOM)
##
@@ -233,6 +235,10 @@
##
{ 'enum': 'ObjectType',
'data': [
+'authz-list',
+'authz-listfile',
+'authz-pam',
+'authz-simple',
'iothread'
] }
@@ -252,6 +258,10 @@
'id': 'str' },
'discriminator': 'qom-type',
'data': {
+ 'authz-list': 'AuthZListProperties',
+ 'authz-listfile': 'AuthZListFileProperties',
+ 'authz-pam': 'AuthZPAMProperties',
+ 'authz-simple': 'AuthZSimpleProperties',
'iothread': 'IothreadProperties'
} }
diff --git a/storage-daemon/qapi/qapi-schema.json
b/storage-daemon/qapi/qapi-schema.json
index 28117c3aac..67749d1101 100644
--- a/storage-daemon/qapi/qapi-schema.json
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -26,6 +26,7 @@
{ 'include': '../../qapi/crypto.json' }
{ 'include': '../../qapi/introspect.json' }
{ 'include': '../../qapi/job.json' }
+{ 'include': '../../qapi/authz.json' }
{ 'include': '../../qapi/qom.json' }
{ 'include': '../../qapi/sockets.json' }
{ 'include': '../../qapi/transaction.json' }
--
2.29.2
[PATCH v3 02/30] qapi/qom: Add ObjectOptions for iothread
Add an ObjectOptions union that will eventually describe the options of
all user creatable object types. As unions can't exist without any
branches, also add the first object type.
This adds a QAPI schema for the properties of the iothread object.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json | 53 +++
1 file changed, 53 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 96c91c1faf..bf2ecb34be 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -202,6 +202,59 @@
'returns': [ 'ObjectPropertyInfo' ],
'allow-preconfig': true }
+##
+# @IothreadProperties:
+#
+# Properties for iothread objects.
+#
+# @poll-max-ns: the maximum number of nanoseconds to busy wait for events.
+# 0 means polling is disabled (default: 32768 on POSIX hosts,
+# 0 otherwise)
+#
+# @poll-grow: the multiplier used to increase the polling time when the
+# algorithm detects it is missing events due to not polling long
+# enough. 0 selects a default behaviour (default: 0)
+#
+# @poll-shrink: the divisor used to decrease the polling time when the
+# algorithm detects it is spending too long polling without
+# encountering events. 0 selects a default behaviour (default: 0)
+#
+# Since: 2.0
+##
+{ 'struct': 'IothreadProperties',
+ 'data': { '*poll-max-ns': 'int',
+'*poll-grow': 'int',
+'*poll-shrink': 'int' } }
+
+##
+# @ObjectType:
+#
+# Since: 6.0
+##
+{ 'enum': 'ObjectType',
+ 'data': [
+'iothread'
+ ] }
+
+##
+# @ObjectOptions:
+#
+# Describes the options of a user creatable QOM object.
+#
+# @qom-type: the class name for the object to be created
+#
+# @id: the name of the new object
+#
+# Since: 6.0
+##
+{ 'union': 'ObjectOptions',
+ 'base': { 'qom-type': 'ObjectType',
+'id': 'str' },
+ 'discriminator': 'qom-type',
+ 'data': {
+ 'iothread': 'IothreadProperties'
+ } }
+
##
# @object-add:
#
--
2.29.2
[PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
This series adds a QAPI type for the properties of all user creatable QOM types and finally makes the --object command line option (in all binaries) and the object-add monitor commands (in QMP and HMP) use the new ObjectOptions union. This change improves things in more than just one way: 1. Documentation for QOM object types has always been lacking. Adding the schema, we get documentation for every property. 2. It prevents bugs by performing parts of the input validation (e.g. checking presence of mandatory properties) already in QAPI instead of relying on separate manual implementations in each class. 3. It provides QAPI introspection for user creatable objects. 4. Non-scalar properties are now supported everywhere because the command line parsers (including HMP) use the keyval parser now. If you are in the CC list and didn't expect this series, it's probably because you're the maintainer of one of the objects for which I'm adding a QAPI schema description. Please just have a look at the specific patch for your object and check whether the schema and its documentation make sense to you. You can ignore all other patches. In a next step after this series, we can add make use of the QAPI structs in the implementation of the object and separate their configuration from the runtime state. Specifically, the plan is to add a .configure() callback to ObjectClass that allows configuring the object in one place at creation time and keeping QOM property setters only for properties that can actually be changed at runtime. Paolo made an example of what the state could look like after this: https://wiki.qemu.org/Features/QOM-QAPI_integration Finally, the intention is to extend the QAPI schema to have separate 'object' entities and generate some of the code that was written manually in the intermediate state before. This series is available as a git tag at: https://repo.or.cz/qemu/kevin.git qapi-object-v3 v3: - Removed now useless QAuthZListRuleListHack - Made some more ObjectOptions branches conditional - Improved documentation for some properties - Fixed 'qemu-img compare' exit code for option parsing failure v2: - Convert not only object-add, but all external interfaces so that the schema will always be enforced and mismatch between implementation and schema can't go unnoticed. - Rebased, covering properties and object types added since v1 (yes, things do become outdated rather quickly when you touch all user creatable objects) - Changed the "Since:" version number in the schema documentation to refer to the version when the object was introduced rather than 6.0 where the schema will (hopefully) be added - Probably some other minor changes Kevin Wolf (30): qapi/qom: Drop deprecated 'props' from object-add qapi/qom: Add ObjectOptions for iothread qapi/qom: Add ObjectOptions for authz-* qapi/qom: Add ObjectOptions for cryptodev-* qapi/qom: Add ObjectOptions for dbus-vmstate qapi/qom: Add ObjectOptions for memory-backend-* qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened' qapi/qom: Add ObjectOptions for throttle-group qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded' qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded' qapi/qom: Add ObjectOptions for can-* qapi/qom: Add ObjectOptions for colo-compare qapi/qom: Add ObjectOptions for filter-* qapi/qom: Add ObjectOptions for pr-manager-helper qapi/qom: Add ObjectOptions for confidential-guest-support qapi/qom: Add ObjectOptions for input-* qapi/qom: Add ObjectOptions for x-remote-object qapi/qom: QAPIfy object-add qom: Make "object" QemuOptsList optional qemu-storage-daemon: Implement --object with qmp_object_add() qom: Remove user_creatable_add_dict() qom: Factor out user_creatable_process_cmdline() qemu-io: Use user_creatable_process_cmdline() for --object qemu-nbd: Use user_creatable_process_cmdline() for --object qom: Add user_creatable_add_from_str() qemu-img: Use user_creatable_process_cmdline() for --object hmp: QAPIfy object_add qom: Add user_creatable_parse_str() vl: QAPIfy -object qom: Drop QemuOpts based interfaces qapi/authz.json | 61 ++- qapi/block-core.json | 27 ++ qapi/common.json | 52 +++ qapi/crypto.json | 159 +++ qapi/machine.json| 22 +- qapi/net.json| 20 - qapi/qom.json| 644 ++- qapi/ui.json | 13 +- docs/system/deprecated.rst | 25 +- docs/system/removed-features.rst | 5 + include/qom/object_interfaces.h | 106 ++--- hw/block/xen-block.c | 16 +- monitor/hmp-cmds.c | 17 +- monitor/misc.c | 2 - qemu-img.c | 251 ++- qemu-io.c
[PATCH v3 01/30] qapi/qom: Drop deprecated 'props' from object-add
The option has been deprecated in QEMU 5.0, remove it.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
qapi/qom.json| 6 +-
docs/system/deprecated.rst | 5 -
docs/system/removed-features.rst | 5 +
qom/qom-qmp-cmds.c | 21 -
4 files changed, 6 insertions(+), 31 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 0b0b92944b..96c91c1faf 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -211,10 +211,6 @@
#
# @id: the name of the new object
#
-# @props: a dictionary of properties to be passed to the backend. Deprecated
-# since 5.0, specify the properties on the top level instead. It is an
-# error to specify the same option both on the top level and in @props.
-#
# Additional arguments depend on qom-type and are passed to the backend
# unchanged.
#
@@ -232,7 +228,7 @@
#
##
{ 'command': 'object-add',
- 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'},
+ 'data': {'qom-type': 'str', 'id': 'str'},
'gen': false } # so we can get the additional arguments
##
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 561c916da2..893f3e8579 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -206,11 +206,6 @@ Use ``migrate-set-parameters`` and
``query-migrate-parameters`` instead.
Use arguments ``base-node`` and ``top-node`` instead.
-``object-add`` option ``props`` (since 5.0)
-'''
-
-Specify the properties for the object as top-level arguments instead.
-
``query-named-block-nodes`` and ``query-block`` result dirty-bitmaps[i].status
(since 4.0)
''
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index c8481cafbd..95f3fb2912 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -58,6 +58,11 @@ documentation of ``query-hotpluggable-cpus`` for additional
details.
Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
+``object-add`` option ``props`` (removed in 6.0)
+
+
+Specify the properties for the object as top-level arguments instead.
+
Human Monitor Protocol (HMP) commands
-
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index b40ac39f30..19fd5e117f 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -225,27 +225,6 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char
*typename,
void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
{
-QObject *props;
-QDict *pdict;
-
-props = qdict_get(qdict, "props");
-if (props) {
-pdict = qobject_to(QDict, props);
-if (!pdict) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
-return;
-}
-qobject_ref(pdict);
-qdict_del(qdict, "props");
-qdict_join(qdict, pdict, false);
-if (qdict_size(pdict) != 0) {
-error_setg(errp, "Option in 'props' conflicts with top level");
-qobject_unref(pdict);
-return;
-}
-qobject_unref(pdict);
-}
-
user_creatable_add_dict(qdict, false, errp);
}
--
2.29.2
