From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50338) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aPsFO-00054A-QL for qemu-devel@nongnu.org; Sun, 31 Jan 2016 08:41:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aPsFM-0006AQ-Cd for qemu-devel@nongnu.org; Sun, 31 Jan 2016 08:40:58 -0500 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:35708) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aPsFL-0006AJ-Tm for qemu-devel@nongnu.org; Sun, 31 Jan 2016 08:40:56 -0500 Received: by mail-wm0-x243.google.com with SMTP id l66so5065317wml.2 for ; Sun, 31 Jan 2016 05:40:55 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20160129152801.GZ3869@thinpad.lan.raisama.net> 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> <20160127152359.GC14935@redhat.com> <20160129152801.GZ3869@thinpad.lan.raisama.net> Date: Sun, 31 Jan 2016 16:40:54 +0300 Message-ID: From: Valentin Rakush Content-Type: multipart/alternative; boundary=001a114a421ce31a78052aa16af5 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: Eduardo Habkost Cc: Markus Armbruster , qemu-devel@nongnu.org, lcapitulino@redhat.com, asmetanin@virtuozzo.com, "Denis V. Lunev" , =?UTF-8?Q?Andreas_F=C3=A4rber?= --001a114a421ce31a78052aa16af5 Content-Type: text/plain; charset=UTF-8 Hi Eduardo, I will try to answer some of your questions at this email and will answer other questions later. > Can you clarify what you mean by "TYPE_DEVICE has its own > properties"? TYPE_DEVICE properties are registered as normal QOM > properties. It is possible that I do not understand object model correctly.... This commit 16bf7f522a2f adds GHashTable *properties; to the ObjectClass struct in the include/qom/object.h The typedef struct DeviceClass from include/hw/qdev-core.h is inherited from ObjectClass. Also DeviceClass has it own properties Property *props. In the device_list_properties we call static DevicePropertyInfo *make_device_property_info Which tries to downcast class to DEVICE_CLASS for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) { So we are using Property *props, defined in the DeviceClass, but we do not use GHashTable * properties, defined in the ObjectClass. Here I mean that DeviceClass has its own properties. > I don't understand what you mean, here. GlobalProperties are not > machine properties, they are just property=value pairs to be > registered as global properties. They are unrelated to the > properties TYPE_MACHINE actually has. Same here. The struct MachineClass is defined in the include/hw/boards.h It has a member GlobalProperty *compat_props; But after commit 16bf7f522a2f it would be better to use ObjectClass properties. IMHO. I did not check how compat_props are used in the code yet. > Could you clarify what you mean by "process different classes > differently"? In the list_device_properties function we should have several conditional statements like if (machine = object_class_dynamic_cast(class, TYPE_MACHINE)) { /* process machine properties using MachineClass GlobalProperty *compat_props; */ } else if (machine = object_class_dynamic_cast(class, TYPE_DEVICE)) { /* process device class properties, using DeviceClass Property *props; */ } else if (machine = object_class_dynamic_cast(class, TYPE_CPU)) { /* process CPU, using ObjectClass GHashTable *properties; */ } > 5) -cpu options: > > Ditto. the list will be incomplete unless all CPU subclasses are > converted to use only class-properties, or the new command uses > object_new(). This is a use case that I initially tried to implement. Regards, Valentin On Fri, Jan 29, 2016 at 6:28 PM, Eduardo Habkost wrote: > On Fri, Jan 29, 2016 at 01:03:38PM +0300, Valentin Rakush wrote: > > 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. > > Can you clarify what you mean by "TYPE_DEVICE has its own > properties"? TYPE_DEVICE properties are registered as normal QOM > properties. > > We can still add a new command that's not specific for > TYPE_DEVICE (if necessary). The point is that it shouldn't return > arbitrarily different (and incomplete) data from the existing > mechanism to list properties. > > In other words, I don't see why the output of "qom-type-prop-list > " can't be as good as the output of "device-list-properties > ". If we make return only class-properties, it will be less > complete and less useful. > > > > Also TYPE_MACHINE has own properties of type GlobalProperty. > > I don't understand what you mean, here. GlobalProperties are not > machine properties, they are just property=value pairs to be > registered as global properties. They are unrelated to the > properties TYPE_MACHINE actually has. > > > Here are two ways (AFAICS): > > - we refactor TYPE_DEVICE and TYPE_MACHINE so they store their properties > > in the ObjectClass properties. > > Too many classes need to be converted. We would still need > something to use during the transiation. > > > - we change device_list_properties so it process different classes > > differently. > > Could you clarify what you mean by "process different classes > differently"? > > A third option is to just use object_new(), like > qmp_device_list_properties() already does. > > > > > 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? > > We can add a new command if we don't want to change how > device-list-properties work. But first I would like to understand > the actual reasons for the new command, because we can't argue > about it if we don't know what the command output will be used > for. How exactly would callers qom-type-prop-list use that > information? > > I see 3 cases where property names are used: > > 1) QMP QOM commands (qom-get/qom-set): > > These properties are available using qom-list, already. > > 2) -device/device_add options: > > These properties are available in device-list-properties, > already. > > 3) -object/object-add options: > > In this case, if you want to return complete data, you only have > two options: a) convert all TYPE_USER_CREATABLE classes to use > class-properties; or b) use the same approach used by > qmp_device_list_properties() (object_new() followed by > enumeration of properteis). > > 4) -machine options: > > This is similar to -object: the list will be incomplete unless > all machine subclasses are converted to use only > class-properties, or the new command uses object_new(). > > 5) -cpu options: > > Ditto. the list will be incomplete unless all CPU subclasses are > converted to use only class-properties, or the new command uses > object_new(). > > > That doesn't mean we don't want to convert other classes to use > class-properties later to simplify internal QEMU code. But if you > want to propose a new QMP command, let's make one that returns > useful data for real use cases. > > I am not sure the list above is complete, so I would like to > understand how exactly the data you want to return will be used. > So for each of the classes you mentioned, I would like to know: > > > /object/machine/generic-pc-machine/pc-0.13-machine > > What exactly do you think the caller use the output of > "qom-type-prop-list pc-0.13-machine" for? How exactly? Would it > use them in the QEMU command-line? In other QMP commands? Which > ones? > > > /object/bus/i2c-bus > > Ditto, what exactly do you tink the caller would do with the > output of "qom-type-prop-list i2c-bus"? > > > /interface/user-creatable > > user-creatable is an interface. If you want to allow interfaces > to register class-properties, you probably need special code for > them during object creation. > > Also, see my question about abstract classes in my previous reply > to Daniel. We can deal with interfaces and abstract classes > later, as we don't even expose class hierarchy information to the > outside (AFAIK). > > > /object/tls-creds/tls-creds-anon > > What exactly do you tink the caller would do with the output of > "qom-type-prop-list 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 > > Ditto for each of the classes above. > > -- > Eduardo > -- Best Regards, Valentin Rakush. --001a114a421ce31a78052aa16af5 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Eduardo,

I will try to answer some o= f your questions at this email and will answer other questions later.
=

> Can you clarify w= hat you mean by "TYPE_DEVICE has its own
> properties"? TYPE_DEVICE= properties are registered as normal QOM
> properties.
<= span style=3D"font-size:12.8px">
It is possible that I do not understand object model correctly.= ...

This commit=C2=A016bf7f522a2f adds GHashT= able *properties; to the ObjectClass struct in the include/qom/object.h
The typedef struct DeviceCla= ss from include/hw/qdev-core.h is inherited from ObjectClass. Also DeviceCl= ass has it own properties
Property *props.

In the device_list_proper= ties we call=C2=A0

static DevicePropertyInfo *make= _device_property_info

Which tries to downcast = class to DEVICE_CLASS

for (prop =3D DEVICE_CLASS(k= lass)->props; prop && prop->name; prop++) {
So we are using Property *props, defined in the DeviceClass, bu= t we do not use GHashTable * properties, defined in the ObjectClass. Here I= mean that DeviceClass has its own properties.

> I don't understand what you mean, he= re. GlobalProperties are not
> machine properties, they are just property=3Dvalue pairs to be=
>= registered as global properties. They are unrelated to the
> properties TYPE= _MACHINE actually has.

Same here. The str= uct MachineClass is defined in the include/hw/boards.h It has a member=C2= =A0GlobalProperty *compat_props;
But after commit=C2=A016bf7= f522a2f it would be better to use ObjectClass properties. IMHO. I did not c= heck how compat_props are used in the code yet.

=
> Could you clarify what you mean = by "process different classes
> differently"?
In the list_device_pro= perties function we should have several conditional statements like<= /div>

if (machine =3D object_class_dynamic_cast(class, TYP= E_MACHINE)) {
/* process = machine properties using MachineClass GlobalProperty *compat_props; */
}
else if (mac= hine =3D object_class_dynamic_cast(class, TYPE_DEVICE)) {
= /* process device class properties, using = DeviceClass Property *props; */
}
else if (= machine =3D object_class_dynamic_cast(clas= s, TYPE_CPU)) {
/* proces= s CPU, using ObjectClass GHashTable *properties; */
}

> 5) -cpu opt= ions:
>=C2=A0
> Ditto. the list will be inco= mplete unless all CPU subclasses are
<= span style=3D"font-size:12.8px">> converted to use only class-properties= , or the new command uses
> object_new().

This is a use case that I initially tried to implement.

Regards,
Valentin
<= br>
On Fri, Jan 29, 2016 at 6:28 PM, Eduardo Habk= ost <ehabkost@redhat.com> wrote:
On Fri, Jan 29, 2016 at 01:03:38PM +0300, Valenti= n Rakush wrote:
> 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,nowai= t
> -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 ow= n
> properties.

Can you clarify what you mean by "TYPE_DEVICE has its own
properties"? TYPE_DEVICE properties are registered as normal QOM
properties.

We can still add a new command that's not specific for
TYPE_DEVICE (if necessary). The point is that it shouldn't return
arbitrarily different (and incomplete) data from the existing
mechanism to list properties.

In other words, I don't see why the output of "qom-type-prop-list<= br> <type>" can't be as good as the output of "device-list-= properties
<type>". If we make return only class-properties, it will be les= s
complete and less useful.


> Also TYPE_MACHINE has own properties of type GlobalProperty.

I don't understand what you mean, here. GlobalProperties are not=
machine properties, they are just property=3Dvalue pairs to be
registered as global properties. They are unrelated to the
properties TYPE_MACHINE actually has.

> Here are two ways (AFAICS):
> - we refactor TYPE_DEVICE and TYPE_MACHINE so they store their propert= ies
> in the ObjectClass properties.

Too many classes need to be converted. We would still need
something to use during the transiation.

> - we change device_list_properties so it process different classes
> differently.

Could you clarify what you mean by "process different classes differently"?

A third option is to just use object_new(), like
qmp_device_list_properties() already does.

>
> The disadvantage of the second approach, is that it is complicating co= de in
> favor of simplifying qapi interface. I like first approach with
> refactoring, although it is more complex. The first approach should pu= t 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 so= mehow
> with TYPE_MACHINE GlobalProperty. Then we will use these properties fo= r 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?

We can add a new command if we don't want to change how
device-list-properties work. But first I would like to understand
the actual reasons for the new command, because we can't argue
about it if we don't know what the command output will be used
for. How exactly would callers qom-type-prop-list use that
information?

I see 3 cases where property names are used:

1) QMP QOM commands (qom-get/qom-set):

These properties are available using qom-list, already.

2) -device/device_add options:

These properties are available in device-list-properties,
already.

3) -object/object-add options:

In this case, if you want to return complete data, you only have
two options: a) convert all TYPE_USER_CREATABLE classes to use
class-properties; or b) use the same approach used by
qmp_device_list_properties() (object_new() followed by
enumeration of properteis).

4) -machine options:

This is similar to -object: the list will be incomplete unless
all machine subclasses are converted to use only
class-properties, or the new command uses object_new().

5) -cpu options:

Ditto. the list will be incomplete unless all CPU subclasses are
converted to use only class-properties, or the new command uses
object_new().


That doesn't mean we don't want to convert other classes to use
class-properties later to simplify internal QEMU code. But if you
want to propose a new QMP command, let's make one that returns
useful data for real use cases.

I am not sure the list above is complete, so I would like to
understand how exactly the data you want to return will be used.
So for each of the classes you mentioned, I would like to know:

> /object/machine/generic-pc-machine/pc-0.13-machine

What exactly do you think the caller use the output of
"qom-type-prop-list pc-0.13-machine" for? How exactly? Would it use them in the QEMU command-line? In other QMP commands? Which
ones?

> /object/bus/i2c-bus

Ditto, what exactly do you tink the caller would do with the
output of "qom-type-prop-list i2c-bus"?

> /interface/user-creatable

user-creatable is an interface. If you want to allow interfaces
to register class-properties, you probably need special code for
them during object creation.

Also, see my question about abstract classes in my previous reply
to Daniel. We can deal with interfaces and abstract classes
later, as we don't even expose class hierarchy information to the
outside (AFAIK).

> /object/tls-creds/tls-creds-anon

What exactly do you tink the caller would do with the output of
"qom-type-prop-list 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

Ditto for each of the classes above.

--
Eduardo



--
Best Regards,
Valentin Rakush.
--001a114a421ce31a78052aa16af5--