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: Sun, 31 Jan 2016 16:40:54 +0300

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 <address@hidden> 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
<type>" can't be as good as the output of "device-list-properties
<type>". 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.

reply via email to

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