From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34347) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aORx4-0000Zs-1h for qemu-devel@nongnu.org; Wed, 27 Jan 2016 10:24:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aORwz-0006w9-7i for qemu-devel@nongnu.org; Wed, 27 Jan 2016 10:24:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43694) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aORwz-0006w5-0N for qemu-devel@nongnu.org; Wed, 27 Jan 2016 10:24:05 -0500 Date: Wed, 27 Jan 2016 15:23:59 +0000 From: "Daniel P. Berrange" Message-ID: <20160127152359.GC14935@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> <20160127150937.GQ3869@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160127150937.GQ3869@thinpad.lan.raisama.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, armbru@redhat.com, lcapitulino@redhat.com, afaerber@suse.de, asmetanin@virtuozzo.com, den@openvz.org, Valentin Rakush 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 lis= t object > > > > > > class properties. A later patch will use this functionality t= o > > > > > > implement x86_64-cpu properties. > > > > > >=20 > > > > > > 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(b= ool has_implements, > > > > > > return ret; > > > > > > } > > > > > > =20 > > > > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *t= ypename, 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(ObjectPrope= rtyInfoList, 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; > > > > > > +} > > > > > > + > > > > >=20 > > > > > We already have "-device ,help", and it uses a completely > > > > > different mechanism for listing properties. There's no reason f= or > > > > > having two arbitrarily different APIs for listing properties > > > > > returning different results. > > > > >=20 > > > > > If qmp_device_list_properties() is not enough for you, please > > > > > clarify why, so we can consider improving it. > > > >=20 > > > > 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 r= efuse > > > > 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 there is also = a > > > > performance and memory overhead in having to instantiate objects = which > > > > is best avoided. > > > >=20 > > > > 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= . > > >=20 > > > 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). > > >=20 > > > About the existing qmp_device_list_properties() limitations: > > >=20 > > > 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. > > >=20 > > > 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). > > >=20 > > > 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. > > >=20 > > > 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. > > > > > > * * * > > >=20 > > > 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? > >=20 > > 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. >=20 > 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 --=20 |: 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 :|