Hi Eduardo, hi Daniel,
I checked most of the classes that are used for x86_64 qemu simulation with
this command line:
x86_64-softmmu/qemu-system-x86_64 -qmp tcp:localhost:4444,server,nowait
-machine pc -cpu core2duo
Here are some of the classes that cannot provide properties with
device_list_properties call:
/object/machine/generic-pc-machine/pc-0.13-machine
/object/bus/i2c-bus
/interface/user-creatable
/object/tls-creds/tls-creds-anon
/object/memory-backend/memory-backend-file
/object/qemu:memory-region
/object/rng-backend/rng-random
/object/tpm-backend/tpm-passthrough
/object/tls-creds/tls-creds-x509
/object/secret
They cannot provide properties because these classes cannot be casted to
TYPE_DEVICE. This is done intentionally because TYPE_DEVICE has its own
properties. Also TYPE_MACHINE has own properties of type GlobalProperty.
Here are two ways (AFAICS):
- we refactor TYPE_DEVICE and TYPE_MACHINE so they store their properties
in the ObjectClass properties.
- we change device_list_properties so it process different classes
differently.
The disadvantage of the second approach, is that it is complicating code in
favor of simplifying qapi interface. I like first approach with
refactoring, although it is more complex. The first approach should put all
properties in the base classes and then use this properties everywhere
(command line help, qmp etc.) The simplest way the refactoring can be done,
is by moving TYPE_DEVICE properties to ObjectClass and merging them somehow
with TYPE_MACHINE GlobalProperty. Then we will use these properties for all
other types of classes.
Of course, we can leave device_list_properties as it is and use
qom-type-prop-list instead.
What do you think? Does these design options make sense for you?
Thank you,
Valentin
On Wed, Jan 27, 2016 at 6:23 PM, Daniel P. Berrange <berra...@redhat.com>
wrote:
> On Wed, Jan 27, 2016 at 01:09:37PM -0200, Eduardo Habkost wrote:
> > On Tue, Jan 26, 2016 at 10:19:13PM +0000, Daniel P. Berrange wrote:
> > > On Tue, Jan 26, 2016 at 03:26:35PM -0200, Eduardo Habkost wrote:
> > > > On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> > > > > On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > > > > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > > > > > This patch adds support for qom-type-prop-list command to list
> object
> > > > > > > class properties. A later patch will use this functionality to
> > > > > > > implement x86_64-cpu properties.
> > > > > > >
> > > > > > > Signed-off-by: Valentin Rakush <valentin.rak...@gmail.com>
> > > > > > > Cc: Luiz Capitulino <lcapitul...@redhat.com>
> > > > > > > Cc: Eric Blake <ebl...@redhat.com>
> > > > > > > Cc: Markus Armbruster <arm...@redhat.com>
> > > > > > > Cc: Andreas Färber <afaer...@suse.de>
> > > > > > > Cc: Daniel P. Berrange <berra...@redhat.com>
> > > > > > > Cc: Eduardo Habkost <ehabk...@redhat.com>
> > > > > > > ---
> > > > > > [...]
> > > > > > > diff --git a/qmp.c b/qmp.c
> > > > > > > index 53affe2..baf25c0 100644
> > > > > > > --- a/qmp.c
> > > > > > > +++ b/qmp.c
> > > > > > > @@ -460,6 +460,37 @@ ObjectTypeInfoList
> *qmp_qom_list_types(bool has_implements,
> > > > > > > return ret;
> > > > > > > }
> > > > > > >
> > > > > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char
> *typename, Error **errp)
> > > > > > > +{
> > > > > > > + ObjectClass *klass;
> > > > > > > + ObjectPropertyInfoList *props = NULL;
> > > > > > > + ObjectProperty *prop;
> > > > > > > + ObjectPropertyIterator iter;
> > > > > > > +
> > > > > > > + klass = object_class_by_name(typename);
> > > > > > > + if (!klass) {
> > > > > > > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > > > > + "Object class '%s' not found", typename);
> > > > > > > + return NULL;
> > > > > > > + }
> > > > > > > +
> > > > > > > + object_class_property_iter_init(&iter, klass);
> > > > > > > + while ((prop = object_property_iter_next(&iter))) {
> > > > > > > + ObjectPropertyInfoList *entry =
> g_new0(ObjectPropertyInfoList, 1);
> > > > > > > +
> > > > > > > + if (entry) {
> > > > > > > + entry->value = g_new0(ObjectPropertyInfo, 1);
> > > > > > > + entry->next = props;
> > > > > > > + props = entry;
> > > > > > > +
> > > > > > > + entry->value->name = g_strdup(prop->name);
> > > > > > > + entry->value->type = g_strdup(prop->type);
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + return props;
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > We already have "-device <type>,help", and it uses a completely
> > > > > > different mechanism for listing properties. There's no reason for
> > > > > > having two arbitrarily different APIs for listing properties
> > > > > > returning different results.
> > > > > >
> > > > > > If qmp_device_list_properties() is not enough for you, please
> > > > > > clarify why, so we can consider improving it.
> > > > >
> > > > > qmp_device_list_properties() has to actually instantiate an
> instance
> > > > > of objects it is reporting properties against, since it is
> reporting
> > > > > properties registered against object instances. In fact it only
> > > > > reports properties against things which are TYPE_DEVICE - it'll
> refuse
> > > > > to report other object types. Having to instantiate objects is
> inherantly
> > > > > limiting to the command because there are some objects that cannot
> be
> > > > > instantiated for this purpose. eg abstract objects and objects
> marked
> > > > > "cannot_destroy_with_object_finalize_yet". Finally there is also a
> > > > > performance and memory overhead in having to instantiate objects
> which
> > > > > is best avoided.
> > > > >
> > > > > This new API is reporting properties that are statically registered
> > > > > against the *class* rather than than object instance. It is
> guaranteed
> > > > > that you can always report these properties for any class without
> any
> > > > > restrictions, nor any chance of side effects during instantiation.
> > > >
> > > > The existing implementation has its limitations, but we can
> > > > address those limitations without exporting a new API that return
> > > > arbitrarily different results (that aren't even a superset of the
> > > > existing API).
> > > >
> > > > About the existing qmp_device_list_properties() limitations:
> > > >
> > > > cannot_destroy_with_object_finalize_yet is supposed to eventually
> > > > go away. If there are use cases that depend on listing properties
> > > > for cannot_destroy_with_object_finalize_yet classes, we can fix
> > > > that.
> > > >
> > > > The TYPE_DEVICE requirement can be removed, as long as the
> > > > non-device QOM classes are object_new()-safe like the existing
> > > > cannot_destroy_with_object_finalize_yet=false device classes
> > > > (they are supposed to be).
> > > >
> > > > About having to instantiate objects: if optimizing that is so
> > > > important, we can gradually convert the existing classes to use
> > > > class-properties. While we convert them, we can even have a
> > > > doesnt_need_to_instantiate_object_to_query_properties flag to
> > > > indicate classes that were already converted. No need to export a
> > > > new API.
> > > >
> > > > Abstract classes are harder, but if they are important we can
> > > > make them a special case inside the existing implementation
> > > > instead of having two APIs.
> > > >
> > > > * * *
> > > >
> > > > So, now we have enumerated the current API limitations. Can we
> > > > enumerate the real world use cases that are affected by them, so
> > > > we know which ones we need to address first?
> > >
> > > Being able to list properties of arbitrary non-device objects is
> > > really the critical thing that's missing right now, with abstract
> > > types a close second.
> >
> > About abstract types: I thought we didn't export any class
> > hierarchy information. Should we do it? I guess we wouldn't want
> > clients to make assumptions about the class hierarchy.
>
> Actually I guess as long as there is one non-abstract subclass we can
> query it doesn't matter.
>
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/
> :|
> |: http://libvirt.org -o- http://virt-manager.org
> :|
> |: http://autobuild.org -o- http://search.cpan.org/~danberr/
> :|
> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc
> :|
>
--
Best Regards,
Valentin Rakush.
