Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush <valentin.rakush@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8496 bytes --]

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 <ehabkost@redhat.com>
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.

[-- Attachment #2: Type: text/html, Size: 12245 bytes --]