From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55425) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOMmg-0002dp-4s for qemu-devel@nongnu.org; Wed, 27 Jan 2016 04:53:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOMmd-00018O-TS for qemu-devel@nongnu.org; Wed, 27 Jan 2016 04:53:06 -0500 Received: from mail-wm0-x244.google.com ([2a00:1450:400c:c09::244]:35688) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOMmd-00018K-I5 for qemu-devel@nongnu.org; Wed, 27 Jan 2016 04:53:03 -0500 Received: by mail-wm0-x244.google.com with SMTP id 123so2280158wmz.2 for ; Wed, 27 Jan 2016 01:53:03 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20160126221913.GA13460@redhat.com> References: <1453710287-12706-1-git-send-email-valentin.rakush@gmail.com> <20160126153538.GN3869@thinpad.lan.raisama.net> <20160126155121.GI15172@redhat.com> <20160126172635.GO3869@thinpad.lan.raisama.net> <20160126221913.GA13460@redhat.com> Date: Wed, 27 Jan 2016 12:53:02 +0300 Message-ID: From: Valentin Rakush Content-Type: multipart/alternative; boundary=001a114545d69517ce052a4dc46d Subject: Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Eduardo Habkost , qemu-devel@nongnu.org, Markus Armbruster , lcapitulino@redhat.com, asmetanin@virtuozzo.com, "Denis V. Lunev" , =?UTF-8?Q?Andreas_F=C3=A4rber?= --001a114545d69517ce052a4dc46d Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 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 > > > > > Cc: Luiz Capitulino > > > > > Cc: Eric Blake > > > > > Cc: Markus Armbruster > > > > > Cc: Andreas F=C3=A4rber > > > > > Cc: Daniel P. Berrange > > > > > Cc: Eduardo Habkost > > > > > --- > > > > [...] > > > > > 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 =3D NULL; > > > > > + ObjectProperty *prop; > > > > > + ObjectPropertyIterator iter; > > > > > + > > > > > + klass =3D 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 =3D object_property_iter_next(&iter))) { > > > > > + ObjectPropertyInfoList *entry =3D > g_new0(ObjectPropertyInfoList, 1); > > > > > + > > > > > + if (entry) { > > > > > + entry->value =3D g_new0(ObjectPropertyInfo, 1); > > > > > + entry->next =3D props; > > > > > + props =3D entry; > > > > > + > > > > > + entry->value->name =3D g_strdup(prop->name); > > > > > + entry->value->type =3D g_strdup(prop->type); > > > > > + } > > > > > + } > > > > > + > > > > > + return props; > > > > > +} > > > > > + > > > > > > > > We already have "-device ,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 refus= e > > > 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 whic= h > > > is best avoided. > > > > > > This new API is reporting properties that are statically registered > > > against the *class* rather than than object instance. It is guarantee= d > > > 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=3Dfalse 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.or= g > :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr= / > :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c > :| > --001a114545d69517ce052a4dc46d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi all,

My five cents... I a= m checking for possible problems in case we want to use=C2=A0qmp_device_lis= t_properties() for listing all class properties. Here are couple concerns:<= br>

- for example, we want to list class propertie= s for "pc-q35-2.4-machine" typename. This is not DeviceClass, the= refore 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 perh= aps we should rename this function to something like qmp_type_list_properti= es. In this case we should refactor source code that already uses qmp_devic= e_list_properties. For example, libvirt is already uses device-list-propert= ies command.

I will do more research.
Regards,
Valentin
On Wed, Jan 27, 2016 at 1:19 AM, Daniel P. Ber= range <berrange@redhat.com> 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:<= br> > > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wr= ote:
> > > > This patch adds support for qom-type-prop-list command = to list object
> > > > class properties. A later patch will use this functiona= lity to
> > > > implement x86_64-cpu properties.
> > > >
> > > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > > Cc: Eric Blake <eblake@redhat.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > Cc: Andreas F=C3=A4rber <afaerber@suse.de>
> > > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > > Cc: Eduardo Habkost <ehabkost@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_t= ypes(bool has_implements,
> > > >=C2=A0 =C2=A0 =C2=A0 return ret;
> > > >=C2=A0 }
> > > >
> > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const c= har *typename, Error **errp)
> > > > +{
> > > > +=C2=A0 =C2=A0 ObjectClass *klass;
> > > > +=C2=A0 =C2=A0 ObjectPropertyInfoList *props =3D NULL;<= br> > > > > +=C2=A0 =C2=A0 ObjectProperty *prop;
> > > > +=C2=A0 =C2=A0 ObjectPropertyIterator iter;
> > > > +
> > > > +=C2=A0 =C2=A0 klass =3D object_class_by_name(typename)= ;
> > > > +=C2=A0 =C2=A0 if (!klass) {
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 error_set(errp, ERROR_CLAS= S_DEVICE_NOT_FOUND,
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 "Object class '%s' not found", typename);
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL;
> > > > +=C2=A0 =C2=A0 }
> > > > +
> > > > +=C2=A0 =C2=A0 object_class_property_iter_init(&ite= r, klass);
> > > > +=C2=A0 =C2=A0 while ((prop =3D object_property_iter_ne= xt(&iter))) {
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ObjectPropertyInfoList *en= try =3D g_new0(ObjectPropertyInfoList, 1);
> > > > +
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (entry) {
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 entry->va= lue =3D g_new0(ObjectPropertyInfo, 1);
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 entry->ne= xt =3D props;
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 props =3D en= try;
> > > > +
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 entry->va= lue->name =3D g_strdup(prop->name);
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 entry->va= lue->type =3D g_strdup(prop->type);
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > > > +=C2=A0 =C2=A0 }
> > > > +
> > > > +=C2=A0 =C2=A0 return props;
> > > > +}
> > > > +
> > >
> > > We already have "-device <type>,help", and i= t uses a completely
> > > different mechanism for listing properties. There's no r= eason for
> > > having two arbitrarily different APIs for listing properties=
> > > returning different results.
> > >
> > > If qmp_device_list_properties() is not enough for you, pleas= e
> > > clarify why, so we can consider improving it.
> >
> > qmp_device_list_properties() has to actually instantiate an insta= nce
> > of objects it is reporting properties against, since it is report= ing
> > 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 in= herantly
> > limiting to the command because there are some objects that canno= t be
> > instantiated for this purpose. eg abstract objects and objects ma= rked
> > "cannot_destroy_with_object_finalize_yet". Finally ther= e 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 register= ed
> > against the *class* rather than than object instance. It is guara= nteed
> > 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<= br> > 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=3Dfalse 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.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * * *
>
> 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 i= s
really the critical thing that's missing right now, with abstract
types a close second.

Regards,
Daniel
--
|: htt= p://berrange.com=C2=A0 =C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 htt= p://www.flickr.com/photos/dberrange/ :|
|: http= ://libvirt.org=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 -o-=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0http://virt-manager.org :| |: ht= tp://autobuild.org=C2=A0 =C2=A0 =C2=A0 =C2=A0-o-=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org=C2=A0 =C2=A0 =C2=A0 =C2=A0-o-=C2=A0 =C2=A0= =C2=A0 =C2=A0http://live.gnome.org/gtk-vnc :|

--001a114545d69517ce052a4dc46d--