qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-li


From: Valentin Rakush
Subject: Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
Date: Wed, 27 Jan 2016 12:53:02 +0300

Hi all,

My five cents... I am checking for possible problems in case we want to use qmp_device_list_properties() for listing all class properties. Here are couple concerns:

- for example, we want to list class properties for "pc-q35-2.4-machine" typename. This is not DeviceClass, therefore we have to change qmp_device_list_properties to accept all classes. >From another side, qmp_device_list_properties is used for "-device FOO,help" (as far as I understand from comments in qdev-core.h). Then use case "-device FOO,help" will lose typecheck for DeviceClass. We will probably need a separate implementation of '-device FOO,help' to check/assert command parameters.

- if we willl use qmp_device_list_properties to list properties of all classes, then perhaps we should rename this function to something like qmp_type_list_properties. In this case we should refactor source code that already uses qmp_device_list_properties. For example, libvirt is already uses device-list-properties command.

I will do more research.

Regards,
Valentin

On Wed, Jan 27, 2016 at 1:19 AM, Daniel P. Berrange <address@hidden> 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 <address@hidden>
> > > > Cc: Luiz Capitulino <address@hidden>
> > > > Cc: Eric Blake <address@hidden>
> > > > Cc: Markus Armbruster <address@hidden>
> > > > Cc: Andreas Färber <address@hidden>
> > > > Cc: Daniel P. Berrange <address@hidden>
> > > > Cc: Eduardo Habkost <address@hidden>
> > > > ---
> > > [...]
> > > > 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.

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 :|


reply via email to

[Prev in Thread] Current Thread [Next in Thread]