[v4,7/8] qmp: Support abstract classes on device-list-properties

Commit Message

Eduardo Habkost Oct. 29, 2016, 1:48 a.m. UTC

When an abstract class is used on device-list-properties, we can
simply return the class properties registered for the class.

This will be useful if management software needs to query for
supported options that apply to all devices of a given type (e.g.
options supported by all CPU models, options supported by all PCI
devices).

Signed-off-by: Eduardo Habkost <[e-mail]>
---
Changes series v1 -> v2:
* (none)

Changes series v2 -> v3:
* Reworded commit message

Changes series v3 -> v4:
* (none)
---
 qmp.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Comments

Igor Mammedov Oct. 31, 2016, 2:07 p.m. UTC | #1

On Fri, 28 Oct 2016 23:48:06 -0200
Eduardo Habkost <[e-mail]> wrote:

> When an abstract class is used on device-list-properties, we can
> simply return the class properties registered for the class.
> 
> This will be useful if management software needs to query for
> supported options that apply to all devices of a given type (e.g.
> options supported by all CPU models, options supported by all PCI
> devices).
Patch looks fine to me but I'm not qmp interface guru
so I'd leave review up to maintainers.

One question though,
How would management software discover typename of abstract class?

Perhaps this patch should be part of some other series.

> Signed-off-by: Eduardo Habkost <[e-mail]>
> ---
> Changes series v1 -> v2:
> * (none)
> 
> Changes series v2 -> v3:
> * Reworded commit message
> 
> Changes series v3 -> v4:
> * (none)
> ---
>  qmp.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/qmp.c b/qmp.c
> index a06cb7b..1e7e60d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -518,7 +518,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
>                                                     Error **errp)
>  {
>      ObjectClass *klass;
> -    Object *obj;
> +    Object *obj = NULL;
>      ObjectProperty *prop;
>      ObjectPropertyIterator iter;
>      DevicePropertyInfoList *prop_list = NULL;
> @@ -537,19 +537,16 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
>      }
>  
>      if (object_class_is_abstract(klass)) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name",
> -                   "non-abstract device type");
> -        return NULL;
> -    }
> -
> -    if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
> -        error_setg(errp, "Can't list properties of device '%s'", typename);
> -        return NULL;
> +        object_class_property_iter_init(&iter, klass);
> +    } else {
> +        if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
> +            error_setg(errp, "Can't list properties of device '%s'", typename);
> +            return NULL;
> +        }
> +        obj = object_new(typename);
> +        object_property_iter_init(&iter, obj);
>      }
>  
> -    obj = object_new(typename);
> -
> -    object_property_iter_init(&iter, obj);
>      while ((prop = object_property_iter_next(&iter))) {
>          DevicePropertyInfo *info;
>          DevicePropertyInfoList *entry;

Eduardo Habkost Oct. 31, 2016, 2:36 p.m. UTC | #2

(CCing libvirt people, as I forgot to CC them)

On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
> On Fri, 28 Oct 2016 23:48:06 -0200
> Eduardo Habkost <[e-mail]> wrote:
> 
> > When an abstract class is used on device-list-properties, we can
> > simply return the class properties registered for the class.
> > 
> > This will be useful if management software needs to query for
> > supported options that apply to all devices of a given type (e.g.
> > options supported by all CPU models, options supported by all PCI
> > devices).
> Patch looks fine to me but I'm not qmp interface guru
> so I'd leave review up to maintainers.
> 
> One question though,
> How would management software discover typename of abstract class?

It depends on the use case. On some cases, management may already
have bus-specific logic that will know what's the base type it
needs to query (e.g. it may query "pci-device" to find out if all
PCI devices support a given option). On other cases, it may be
discovered using other commands.

For the CPU case, I will propose adding the base QOM CPU typename
in the query-target command.

> 
> Perhaps this patch should be part of some other series.

This is a valid point. In this case, it depends on the approach
we want to take: do we want to provide a flexible interface for
management, let them find ways to make it useful and give us
feedback on what is lacking; or do we want to provide the new
interface only after we have specified the complete solution for
the problem?

I don't know the answer. I will let the qdev/QOM/QMP maintainers
answer that.

> 
> > Signed-off-by: Eduardo Habkost <[e-mail]>
> > ---
> > Changes series v1 -> v2:
> > * (none)
> > 
> > Changes series v2 -> v3:
> > * Reworded commit message
> > 
> > Changes series v3 -> v4:
> > * (none)
> > ---
> >  qmp.c | 21 +++++++++------------
> >  1 file changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/qmp.c b/qmp.c
> > index a06cb7b..1e7e60d 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -518,7 +518,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
> >                                                     Error **errp)
> >  {
> >      ObjectClass *klass;
> > -    Object *obj;
> > +    Object *obj = NULL;
> >      ObjectProperty *prop;
> >      ObjectPropertyIterator iter;
> >      DevicePropertyInfoList *prop_list = NULL;
> > @@ -537,19 +537,16 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
> >      }
> >  
> >      if (object_class_is_abstract(klass)) {
> > -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name",
> > -                   "non-abstract device type");
> > -        return NULL;
> > -    }
> > -
> > -    if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
> > -        error_setg(errp, "Can't list properties of device '%s'", typename);
> > -        return NULL;
> > +        object_class_property_iter_init(&iter, klass);
> > +    } else {
> > +        if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
> > +            error_setg(errp, "Can't list properties of device '%s'", typename);
> > +            return NULL;
> > +        }
> > +        obj = object_new(typename);
> > +        object_property_iter_init(&iter, obj);
> >      }
> >  
> > -    obj = object_new(typename);
> > -
> > -    object_property_iter_init(&iter, obj);
> >      while ((prop = object_property_iter_next(&iter))) {
> >          DevicePropertyInfo *info;
> >          DevicePropertyInfoList *entry;
>

Markus Armbruster Nov. 4, 2016, 3:45 p.m. UTC | #3

Eduardo Habkost <[e-mail]> writes:

> (CCing libvirt people, as I forgot to CC them)
>
> On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
>> On Fri, 28 Oct 2016 23:48:06 -0200
>> Eduardo Habkost <[e-mail]> wrote:
>> 
>> > When an abstract class is used on device-list-properties, we can
>> > simply return the class properties registered for the class.
>> > 
>> > This will be useful if management software needs to query for
>> > supported options that apply to all devices of a given type (e.g.
>> > options supported by all CPU models, options supported by all PCI
>> > devices).
>> Patch looks fine to me but I'm not qmp interface guru
>> so I'd leave review up to maintainers.
>> 
>> One question though,
>> How would management software discover typename of abstract class?
>
> It depends on the use case. On some cases, management may already
> have bus-specific logic that will know what's the base type it
> needs to query (e.g. it may query "pci-device" to find out if all
> PCI devices support a given option). On other cases, it may be
> discovered using other commands.

The stated purpose of this feature is to let management software "query
for supported options that apply to all devices of a given type".  I
suspect that when management software has a notion of "a given type", it
knows its name.

Will management software go fishing for subtype relationships beyond the
types it knows?  I doubt it.  Of course, management software developers
are welcome to educate me :)

> For the CPU case, I will propose adding the base QOM CPU typename
> in the query-target command.

Does this type name vary?  If yes, can you give examples?

>> Perhaps this patch should be part of some other series.
>
> This is a valid point. In this case, it depends on the approach
> we want to take: do we want to provide a flexible interface for
> management, let them find ways to make it useful and give us
> feedback on what is lacking; or do we want to provide the new
> interface only after we have specified the complete solution for
> the problem?
>
> I don't know the answer. I will let the qdev/QOM/QMP maintainers
> answer that.

I'm reluctant to add QMP features just because we think they might be
useful to someone.  We should talk to users to confirm our hunch before
we act on it.

That said, this one isn't exactly a biggie.

Eduardo Habkost Nov. 4, 2016, 4:04 p.m. UTC | #4

On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <[e-mail]> writes:
> 
> > (CCing libvirt people, as I forgot to CC them)
> >
> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
> >> On Fri, 28 Oct 2016 23:48:06 -0200
> >> Eduardo Habkost <[e-mail]> wrote:
> >> 
> >> > When an abstract class is used on device-list-properties, we can
> >> > simply return the class properties registered for the class.
> >> > 
> >> > This will be useful if management software needs to query for
> >> > supported options that apply to all devices of a given type (e.g.
> >> > options supported by all CPU models, options supported by all PCI
> >> > devices).
> >> Patch looks fine to me but I'm not qmp interface guru
> >> so I'd leave review up to maintainers.
> >> 
> >> One question though,
> >> How would management software discover typename of abstract class?
> >
> > It depends on the use case. On some cases, management may already
> > have bus-specific logic that will know what's the base type it
> > needs to query (e.g. it may query "pci-device" to find out if all
> > PCI devices support a given option). On other cases, it may be
> > discovered using other commands.
> 
> The stated purpose of this feature is to let management software "query
> for supported options that apply to all devices of a given type".  I
> suspect that when management software has a notion of "a given type", it
> knows its name.
> 
> Will management software go fishing for subtype relationships beyond the
> types it knows?  I doubt it.  Of course, management software developers
> are welcome to educate me :)
> 
> > For the CPU case, I will propose adding the base QOM CPU typename
> > in the query-target command.
> 
> Does this type name vary?  If yes, can you give examples?

It does. x86-specific CPU properties are on the x86_64-cpu and
i386-cpu classes. arm-specific CPU properties are on the arm-cpu
class.

> 
> >> Perhaps this patch should be part of some other series.
> >
> > This is a valid point. In this case, it depends on the approach
> > we want to take: do we want to provide a flexible interface for
> > management, let them find ways to make it useful and give us
> > feedback on what is lacking; or do we want to provide the new
> > interface only after we have specified the complete solution for
> > the problem?
> >
> > I don't know the answer. I will let the qdev/QOM/QMP maintainers
> > answer that.
> 
> I'm reluctant to add QMP features just because we think they might be
> useful to someone.  We should talk to users to confirm our hunch before
> we act on it.
> 
> That said, this one isn't exactly a biggie.

Considering we are past soft freeze, I can document a more
concrete usage example when I submit the next version.

Markus Armbruster Nov. 7, 2016, 8:09 a.m. UTC | #5

Eduardo Habkost <[e-mail]> writes:

> On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <[e-mail]> writes:
>> 
>> > (CCing libvirt people, as I forgot to CC them)
>> >
>> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
>> >> On Fri, 28 Oct 2016 23:48:06 -0200
>> >> Eduardo Habkost <[e-mail]> wrote:
>> >> 
>> >> > When an abstract class is used on device-list-properties, we can
>> >> > simply return the class properties registered for the class.
>> >> > 
>> >> > This will be useful if management software needs to query for
>> >> > supported options that apply to all devices of a given type (e.g.
>> >> > options supported by all CPU models, options supported by all PCI
>> >> > devices).
>> >> Patch looks fine to me but I'm not qmp interface guru
>> >> so I'd leave review up to maintainers.
>> >> 
>> >> One question though,
>> >> How would management software discover typename of abstract class?
>> >
>> > It depends on the use case. On some cases, management may already
>> > have bus-specific logic that will know what's the base type it
>> > needs to query (e.g. it may query "pci-device" to find out if all
>> > PCI devices support a given option). On other cases, it may be
>> > discovered using other commands.
>> 
>> The stated purpose of this feature is to let management software "query
>> for supported options that apply to all devices of a given type".  I
>> suspect that when management software has a notion of "a given type", it
>> knows its name.
>> 
>> Will management software go fishing for subtype relationships beyond the
>> types it knows?  I doubt it.  Of course, management software developers
>> are welcome to educate me :)
>> 
>> > For the CPU case, I will propose adding the base QOM CPU typename
>> > in the query-target command.
>> 
>> Does this type name vary?  If yes, can you give examples?
>
> It does. x86-specific CPU properties are on the x86_64-cpu and
> i386-cpu classes. arm-specific CPU properties are on the arm-cpu
> class.

I see we have concrete CPUs (such as "Westmere-x86_64-cpu"), which are
subtypes of an abstract CPU (such as "x86_64-cpu"), which is a subtype
of "cpu", which is a subtype of "device", which is a subtype of
"object".

The chain "cpu" - "device" - "object" is fixed and well-known.

The link from there to the concrete CPU varies.  Whether it could be
considered well-known or not is debatable.

My true question is: should we have a special purpose interface to get
the abstract supertype of concrete CPU types, or should be have general
purpose means to introspect the subtype hierarchy?

Note that we have the latter already, although in a rather cumbersome
form:

    { "execute": "qom-list-types",
      "arguments": { "implements": T, "abstract": true } }

lists all subtypes of T.  You can filter out the concrete subtypes by
subtracting the same query with "abstract": false.  Start with the
type you're interested in, find all its abstract supertypes.  If you
need to know more, repeat for the types you found.

>> >> Perhaps this patch should be part of some other series.
>> >
>> > This is a valid point. In this case, it depends on the approach
>> > we want to take: do we want to provide a flexible interface for
>> > management, let them find ways to make it useful and give us
>> > feedback on what is lacking; or do we want to provide the new
>> > interface only after we have specified the complete solution for
>> > the problem?
>> >
>> > I don't know the answer. I will let the qdev/QOM/QMP maintainers
>> > answer that.
>> 
>> I'm reluctant to add QMP features just because we think they might be
>> useful to someone.  We should talk to users to confirm our hunch before
>> we act on it.
>> 
>> That said, this one isn't exactly a biggie.
>
> Considering we are past soft freeze, I can document a more
> concrete usage example when I submit the next version.

I recommend you get libvirt developers to buy into this feature first.

Eduardo Habkost Nov. 7, 2016, 12:38 p.m. UTC | #6

On Mon, Nov 07, 2016 at 09:09:58AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <[e-mail]> writes:
> 
> > On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <[e-mail]> writes:
> >> 
> >> > (CCing libvirt people, as I forgot to CC them)
> >> >
> >> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
> >> >> On Fri, 28 Oct 2016 23:48:06 -0200
> >> >> Eduardo Habkost <[e-mail]> wrote:
> >> >> 
> >> >> > When an abstract class is used on device-list-properties, we can
> >> >> > simply return the class properties registered for the class.
> >> >> > 
> >> >> > This will be useful if management software needs to query for
> >> >> > supported options that apply to all devices of a given type (e.g.
> >> >> > options supported by all CPU models, options supported by all PCI
> >> >> > devices).
> >> >> Patch looks fine to me but I'm not qmp interface guru
> >> >> so I'd leave review up to maintainers.
> >> >> 
> >> >> One question though,
> >> >> How would management software discover typename of abstract class?
> >> >
> >> > It depends on the use case. On some cases, management may already
> >> > have bus-specific logic that will know what's the base type it
> >> > needs to query (e.g. it may query "pci-device" to find out if all
> >> > PCI devices support a given option). On other cases, it may be
> >> > discovered using other commands.
> >> 
> >> The stated purpose of this feature is to let management software "query
> >> for supported options that apply to all devices of a given type".  I
> >> suspect that when management software has a notion of "a given type", it
> >> knows its name.
> >> 
> >> Will management software go fishing for subtype relationships beyond the
> >> types it knows?  I doubt it.  Of course, management software developers
> >> are welcome to educate me :)
> >> 
> >> > For the CPU case, I will propose adding the base QOM CPU typename
> >> > in the query-target command.
> >> 
> >> Does this type name vary?  If yes, can you give examples?
> >
> > It does. x86-specific CPU properties are on the x86_64-cpu and
> > i386-cpu classes. arm-specific CPU properties are on the arm-cpu
> > class.
> 
> I see we have concrete CPUs (such as "Westmere-x86_64-cpu"), which are
> subtypes of an abstract CPU (such as "x86_64-cpu"), which is a subtype
> of "cpu", which is a subtype of "device", which is a subtype of
> "object".
> 
> The chain "cpu" - "device" - "object" is fixed and well-known.
> 
> The link from there to the concrete CPU varies.  Whether it could be
> considered well-known or not is debatable.
> 
> My true question is: should we have a special purpose interface to get
> the abstract supertype of concrete CPU types, or should be have general
> purpose means to introspect the subtype hierarchy?
> 
> Note that we have the latter already, although in a rather cumbersome
> form:
> 
>     { "execute": "qom-list-types",
>       "arguments": { "implements": T, "abstract": true } }
> 
> lists all subtypes of T.  You can filter out the concrete subtypes by
> subtracting the same query with "abstract": false.  Start with the
> type you're interested in, find all its abstract supertypes.  If you
> need to know more, repeat for the types you found.

Looks cumbersome, because I don't see a way to find all
supertypes of a given type without walking the whole tree
starting from "object" (is there one?). But it could be improved
a bit if we added a "implements" field to ObjectTypeInfo.

But, maybe we should take a step back: my original goal was to
let libvirt know which properties are supported by any CPU model
when using "-cpu". Maybe we should make QEMU do the QOM->option
translation and implement "-cpu" support in
query-command-line-options? We already do something similar with
"-machine".

> 
> >> >> Perhaps this patch should be part of some other series.
> >> >
> >> > This is a valid point. In this case, it depends on the approach
> >> > we want to take: do we want to provide a flexible interface for
> >> > management, let them find ways to make it useful and give us
> >> > feedback on what is lacking; or do we want to provide the new
> >> > interface only after we have specified the complete solution for
> >> > the problem?
> >> >
> >> > I don't know the answer. I will let the qdev/QOM/QMP maintainers
> >> > answer that.
> >> 
> >> I'm reluctant to add QMP features just because we think they might be
> >> useful to someone.  We should talk to users to confirm our hunch before
> >> we act on it.
> >> 
> >> That said, this one isn't exactly a biggie.
> >
> > Considering we are past soft freeze, I can document a more
> > concrete usage example when I submit the next version.
> 
> I recommend you get libvirt developers to buy into this feature first.

Sure. This series originated from a request by Jiri to let him
know which options are supported by "-cpu".

Halil Pasic Nov. 7, 2016, 12:45 p.m. UTC | #7

On 10/29/2016 03:48 AM, Eduardo Habkost wrote:
> When an abstract class is used on device-list-properties, we can
> simply return the class properties registered for the class.
> 
> This will be useful if management software needs to query for
> supported options that apply to all devices of a given type (e.g.
> options supported by all CPU models, options supported by all PCI
> devices).

This will question is slightly off topic (sorry about that) but it does
concern the feature you are trying to establish. 

I stumbled upon this while trying clean up qdev properties in
virtio-ccw. What I dislike is that there is a bunch of properties which
are conceptually inherited from ancestor classes (like devno from
CcwDevice) yet the qdev properties are independent at the moment (that
is each device declares the devno property for itself -- lots of code
duplication).

Now here comes the problem. The ioeventfd property is a property of the
transport (that is logically belonging to VirtioCcwDevice) and tells if
QEMU should use ioeventfd for notifiers. The trouble is this property is
not present for vhost devices since basically the ioeventfd is used by
the in host kernel vhost driver, so the degree of freedom is gone. By
the way, we have the same with virtio-pci and there the ioeventfd
property was simply left out of common virtio-pci properties regardless
of the properties memory backing (where the set writes to and get reads
from) sitting in that is the base class.

Now I think loosing a degree of freedom in the process of specialization
is not something outrageous, but I was unable to figure out how this is
to be modeled with your class based property approach. Can you help me
here?

With the old object anchored properties I was tempted to do something
like simply just removing the property form the object or overriding it
with a read only property but this would now break the semantic of
class properties. Of course having the data backing in the parent and
the property in the child does work but is IMHO kind of ugly.

Regards,
Halil


> 
> Signed-off-by: Eduardo Habkost <[e-mail]>

Eduardo Habkost Nov. 7, 2016, 1:05 p.m. UTC | #8

On Mon, Nov 07, 2016 at 01:45:59PM +0100, Halil Pasic wrote:
> 
> 
> On 10/29/2016 03:48 AM, Eduardo Habkost wrote:
> > When an abstract class is used on device-list-properties, we can
> > simply return the class properties registered for the class.
> > 
> > This will be useful if management software needs to query for
> > supported options that apply to all devices of a given type (e.g.
> > options supported by all CPU models, options supported by all PCI
> > devices).
> 
> This will question is slightly off topic (sorry about that) but it does
> concern the feature you are trying to establish. 
> 
> I stumbled upon this while trying clean up qdev properties in
> virtio-ccw. What I dislike is that there is a bunch of properties which
> are conceptually inherited from ancestor classes (like devno from
> CcwDevice) yet the qdev properties are independent at the moment (that
> is each device declares the devno property for itself -- lots of code
> duplication).
> 
> Now here comes the problem. The ioeventfd property is a property of the
> transport (that is logically belonging to VirtioCcwDevice) and tells if
> QEMU should use ioeventfd for notifiers. The trouble is this property is
> not present for vhost devices since basically the ioeventfd is used by
> the in host kernel vhost driver, so the degree of freedom is gone. By
> the way, we have the same with virtio-pci and there the ioeventfd
> property was simply left out of common virtio-pci properties regardless
> of the properties memory backing (where the set writes to and get reads
> from) sitting in that is the base class.
> 
> Now I think loosing a degree of freedom in the process of specialization
> is not something outrageous, but I was unable to figure out how this is
> to be modeled with your class based property approach. Can you help me
> here?
> 
> With the old object anchored properties I was tempted to do something
> like simply just removing the property form the object or overriding it
> with a read only property but this would now break the semantic of
> class properties. Of course having the data backing in the parent and
> the property in the child does work but is IMHO kind of ugly.

If you want some subclasses to not have the property, then I
recommend not registering it as a class property on the base
class in the first place. I don't expect to see a mechanism to
allow subclasses to remove or override class properties from
parent classes.

You have many alternatives in this case: you could use instance
properties, or register it as class property only on some
subclasses (class_base_init could you help you do it on a single
place, instead of duplicating the property registration code on
subclasses).

Markus Armbruster Nov. 7, 2016, 2:40 p.m. UTC | #9

Eduardo Habkost <[e-mail]> writes:

> On Mon, Nov 07, 2016 at 09:09:58AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <[e-mail]> writes:
>> 
>> > On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <[e-mail]> writes:
>> >> 
>> >> > (CCing libvirt people, as I forgot to CC them)
>> >> >
>> >> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
>> >> >> On Fri, 28 Oct 2016 23:48:06 -0200
>> >> >> Eduardo Habkost <[e-mail]> wrote:
>> >> >> 
>> >> >> > When an abstract class is used on device-list-properties, we can
>> >> >> > simply return the class properties registered for the class.
>> >> >> > 
>> >> >> > This will be useful if management software needs to query for
>> >> >> > supported options that apply to all devices of a given type (e.g.
>> >> >> > options supported by all CPU models, options supported by all PCI
>> >> >> > devices).
>> >> >> Patch looks fine to me but I'm not qmp interface guru
>> >> >> so I'd leave review up to maintainers.
>> >> >> 
>> >> >> One question though,
>> >> >> How would management software discover typename of abstract class?
>> >> >
>> >> > It depends on the use case. On some cases, management may already
>> >> > have bus-specific logic that will know what's the base type it
>> >> > needs to query (e.g. it may query "pci-device" to find out if all
>> >> > PCI devices support a given option). On other cases, it may be
>> >> > discovered using other commands.
>> >> 
>> >> The stated purpose of this feature is to let management software "query
>> >> for supported options that apply to all devices of a given type".  I
>> >> suspect that when management software has a notion of "a given type", it
>> >> knows its name.
>> >> 
>> >> Will management software go fishing for subtype relationships beyond the
>> >> types it knows?  I doubt it.  Of course, management software developers
>> >> are welcome to educate me :)
>> >> 
>> >> > For the CPU case, I will propose adding the base QOM CPU typename
>> >> > in the query-target command.
>> >> 
>> >> Does this type name vary?  If yes, can you give examples?
>> >
>> > It does. x86-specific CPU properties are on the x86_64-cpu and
>> > i386-cpu classes. arm-specific CPU properties are on the arm-cpu
>> > class.
>> 
>> I see we have concrete CPUs (such as "Westmere-x86_64-cpu"), which are
>> subtypes of an abstract CPU (such as "x86_64-cpu"), which is a subtype
>> of "cpu", which is a subtype of "device", which is a subtype of
>> "object".
>> 
>> The chain "cpu" - "device" - "object" is fixed and well-known.
>> 
>> The link from there to the concrete CPU varies.  Whether it could be
>> considered well-known or not is debatable.
>> 
>> My true question is: should we have a special purpose interface to get
>> the abstract supertype of concrete CPU types, or should be have general
>> purpose means to introspect the subtype hierarchy?
>> 
>> Note that we have the latter already, although in a rather cumbersome
>> form:
>> 
>>     { "execute": "qom-list-types",
>>       "arguments": { "implements": T, "abstract": true } }
>> 
>> lists all subtypes of T.  You can filter out the concrete subtypes by
>> subtracting the same query with "abstract": false.  Start with the
>> type you're interested in, find all its abstract supertypes.  If you
>> need to know more, repeat for the types you found.
>
> Looks cumbersome, because I don't see a way to find all
> supertypes of a given type without walking the whole tree
> starting from "object" (is there one?). But it could be improved
> a bit if we added a "implements" field to ObjectTypeInfo.

My point is: we can skip discussing whether we should expose the subtype
relation, because we already do.

> But, maybe we should take a step back: my original goal was to
> let libvirt know which properties are supported by any CPU model
> when using "-cpu".

Why is that useful?

>                    Maybe we should make QEMU do the QOM->option
> translation and implement "-cpu" support in
> query-command-line-options? We already do something similar with
> "-machine".

query-command-line-options is flawed, and the less it's used, the
happier I am.

Real command line introspection would be nice, but getting it will
involve cleaning up the unholy mess our command line has become.  Don't
hold your breath.

My preferred workaround is to find something equivalent to introspect
via QMP.

That said, if you come up with a patch that solves a real problem now, I
won't object to it just because it involves query-command-line-options.

[...]

Halil Pasic Nov. 7, 2016, 2:48 p.m. UTC | #10

On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> If you want some subclasses to not have the property, then I
> recommend not registering it as a class property on the base
> class in the first place. I don't expect to see a mechanism to
> allow subclasses to remove or override class properties from
> parent classes.
> 

Thank you very much for your reply.

I understand, yet I see potential problems. The example with ioeventfd
and vhost in virtio-pci is a good one also because  the first there was
the ioeventfd property with commit 653ced07 and then the vhost case came
along with commit 50787628ee3 (ok ioeventfd is not there for some non
vhost virtio-pci devices for reasons I do not understand).

To rephrase this in generic context a specialization for which a
property does not make sense might come along after the property at the
base class was established.

Now AFAIU properties are external API, so having to make a compatibility
breaking change there might not be fun. Does this mean one should be
very careful to put only use class level properties on abstract classes
where its certain that the property always makes sense including it's
access control?

If yes, how does this relate to patch 5 "Register static properties as
class properties" where the object level properties are converted to
class level properties?

> You have many alternatives in this case: you could use instance
> properties

What are inheritance properties? Sorry I'm not quite that deep
yet to know myself.

> , or register it as class property only on some
> subclasses (class_base_init could you help you do it on a single
> place, instead of duplicating the property registration code on
> subclasses).

Thanks again, and sorry if I'm a bother.

Cheers,
Halil

Daniel P. Berrangé Nov. 7, 2016, 3:01 p.m. UTC | #11

On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> 
> 
> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> > If you want some subclasses to not have the property, then I
> > recommend not registering it as a class property on the base
> > class in the first place. I don't expect to see a mechanism to
> > allow subclasses to remove or override class properties from
> > parent classes.
> > 
> 
> Thank you very much for your reply.
> 
> I understand, yet I see potential problems. The example with ioeventfd
> and vhost in virtio-pci is a good one also because  the first there was
> the ioeventfd property with commit 653ced07 and then the vhost case came
> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> vhost virtio-pci devices for reasons I do not understand).
> 
> To rephrase this in generic context a specialization for which a
> property does not make sense might come along after the property at the
> base class was established.
> 
> Now AFAIU properties are external API, so having to make a compatibility
> breaking change there might not be fun. Does this mean one should be
> very careful to put only use class level properties on abstract classes
> where its certain that the property always makes sense including it's
> access control?

This could be an argument for *NOT* allowing introspectiing of properties
against abstract parent classes. If you only ever allow introspecting against
leaf node non-abstract classes, then QEMU retains the freedom to move props
from a base class down to an leaf class without risk of breaking mgmt apps.

Regards,
Daniel

Markus Armbruster Nov. 7, 2016, 3:51 p.m. UTC | #12

"Daniel P. Berrange" <[e-mail]> writes:

> On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
>> 
>> 
>> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
>> > If you want some subclasses to not have the property, then I
>> > recommend not registering it as a class property on the base
>> > class in the first place. I don't expect to see a mechanism to
>> > allow subclasses to remove or override class properties from
>> > parent classes.
>> > 
>> 
>> Thank you very much for your reply.
>> 
>> I understand, yet I see potential problems. The example with ioeventfd
>> and vhost in virtio-pci is a good one also because  the first there was
>> the ioeventfd property with commit 653ced07 and then the vhost case came
>> along with commit 50787628ee3 (ok ioeventfd is not there for some non
>> vhost virtio-pci devices for reasons I do not understand).
>> 
>> To rephrase this in generic context a specialization for which a
>> property does not make sense might come along after the property at the
>> base class was established.
>> 
>> Now AFAIU properties are external API, so having to make a compatibility
>> breaking change there might not be fun. Does this mean one should be
>> very careful to put only use class level properties on abstract classes
>> where its certain that the property always makes sense including it's
>> access control?
>
> This could be an argument for *NOT* allowing introspectiing of properties
> against abstract parent classes. If you only ever allow introspecting against
> leaf node non-abstract classes, then QEMU retains the freedom to move props
> from a base class down to an leaf class without risk of breaking mgmt apps.

That's a really good point.  To generalize it a bit, introspection of
actual interfaces is fine, but permitting introspection of how they are
made can add artificial constraints.

Introspecting the subtype relation is already problematic in this view.

Eduardo Habkost Nov. 7, 2016, 5:11 p.m. UTC | #13

On Mon, Nov 07, 2016 at 03:40:57PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <[e-mail]> writes:
> 
> > On Mon, Nov 07, 2016 at 09:09:58AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <[e-mail]> writes:
> >> 
> >> > On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
> >> >> Eduardo Habkost <[e-mail]> writes:
> >> >> 
> >> >> > (CCing libvirt people, as I forgot to CC them)
> >> >> >
> >> >> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
> >> >> >> On Fri, 28 Oct 2016 23:48:06 -0200
> >> >> >> Eduardo Habkost <[e-mail]> wrote:
> >> >> >> 
> >> >> >> > When an abstract class is used on device-list-properties, we can
> >> >> >> > simply return the class properties registered for the class.
> >> >> >> > 
> >> >> >> > This will be useful if management software needs to query for
> >> >> >> > supported options that apply to all devices of a given type (e.g.
> >> >> >> > options supported by all CPU models, options supported by all PCI
> >> >> >> > devices).
> >> >> >> Patch looks fine to me but I'm not qmp interface guru
> >> >> >> so I'd leave review up to maintainers.
> >> >> >> 
> >> >> >> One question though,
> >> >> >> How would management software discover typename of abstract class?
> >> >> >
> >> >> > It depends on the use case. On some cases, management may already
> >> >> > have bus-specific logic that will know what's the base type it
> >> >> > needs to query (e.g. it may query "pci-device" to find out if all
> >> >> > PCI devices support a given option). On other cases, it may be
> >> >> > discovered using other commands.
> >> >> 
> >> >> The stated purpose of this feature is to let management software "query
> >> >> for supported options that apply to all devices of a given type".  I
> >> >> suspect that when management software has a notion of "a given type", it
> >> >> knows its name.
> >> >> 
> >> >> Will management software go fishing for subtype relationships beyond the
> >> >> types it knows?  I doubt it.  Of course, management software developers
> >> >> are welcome to educate me :)
> >> >> 
> >> >> > For the CPU case, I will propose adding the base QOM CPU typename
> >> >> > in the query-target command.
> >> >> 
> >> >> Does this type name vary?  If yes, can you give examples?
> >> >
> >> > It does. x86-specific CPU properties are on the x86_64-cpu and
> >> > i386-cpu classes. arm-specific CPU properties are on the arm-cpu
> >> > class.
> >> 
> >> I see we have concrete CPUs (such as "Westmere-x86_64-cpu"), which are
> >> subtypes of an abstract CPU (such as "x86_64-cpu"), which is a subtype
> >> of "cpu", which is a subtype of "device", which is a subtype of
> >> "object".
> >> 
> >> The chain "cpu" - "device" - "object" is fixed and well-known.
> >> 
> >> The link from there to the concrete CPU varies.  Whether it could be
> >> considered well-known or not is debatable.
> >> 
> >> My true question is: should we have a special purpose interface to get
> >> the abstract supertype of concrete CPU types, or should be have general
> >> purpose means to introspect the subtype hierarchy?
> >> 
> >> Note that we have the latter already, although in a rather cumbersome
> >> form:
> >> 
> >>     { "execute": "qom-list-types",
> >>       "arguments": { "implements": T, "abstract": true } }
> >> 
> >> lists all subtypes of T.  You can filter out the concrete subtypes by
> >> subtracting the same query with "abstract": false.  Start with the
> >> type you're interested in, find all its abstract supertypes.  If you
> >> need to know more, repeat for the types you found.
> >
> > Looks cumbersome, because I don't see a way to find all
> > supertypes of a given type without walking the whole tree
> > starting from "object" (is there one?). But it could be improved
> > a bit if we added a "implements" field to ObjectTypeInfo.
> 
> My point is: we can skip discussing whether we should expose the subtype
> relation, because we already do.

Correct. My only problem is that it seems to add extra
assumptions to the code (e.g. that there's only one abstract CPU
type). But if libvirt is careful, it doesn't need to make any
assumptions: it can explore the type hierarchy and confirm that
the assumptions are correct.

> 
> > But, maybe we should take a step back: my original goal was to
> > let libvirt know which properties are supported by any CPU model
> > when using "-cpu".
> 
> Why is that useful?

libvirt wants to know if the QEMU binary supports a given -cpu
option (normally CPU features that can be enabled/disabled using
"+foo"/"-foo").

> 
> >                    Maybe we should make QEMU do the QOM->option
> > translation and implement "-cpu" support in
> > query-command-line-options? We already do something similar with
> > "-machine".
> 
> query-command-line-options is flawed, and the less it's used, the
> happier I am.

No problem, let's just forget my suggestion.  :)

> 
> Real command line introspection would be nice, but getting it will
> involve cleaning up the unholy mess our command line has become.  Don't
> hold your breath.
> 
> My preferred workaround is to find something equivalent to introspect
> via QMP.

Sounds good to me.

> 
> That said, if you come up with a patch that solves a real problem now, I
> won't object to it just because it involves query-command-line-options.

I think I will keep the current approach. Changing
query-command-line-option would probably be more complex and
solve only the -cpu problem. Making QOM introspection better
looks simpler and will probably be useful for other use cases.

I am still tempted to try to make QOM->query-command-line-option
mapping easier and simpler, but it doesn't seem to be the answer
to the current problem.

Eduardo Habkost Nov. 7, 2016, 5:27 p.m. UTC | #14

On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <[e-mail]> writes:
> 
> > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> >> 
> >> 
> >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> >> > If you want some subclasses to not have the property, then I
> >> > recommend not registering it as a class property on the base
> >> > class in the first place. I don't expect to see a mechanism to
> >> > allow subclasses to remove or override class properties from
> >> > parent classes.
> >> > 
> >> 
> >> Thank you very much for your reply.
> >> 
> >> I understand, yet I see potential problems. The example with ioeventfd
> >> and vhost in virtio-pci is a good one also because  the first there was
> >> the ioeventfd property with commit 653ced07 and then the vhost case came
> >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> >> vhost virtio-pci devices for reasons I do not understand).
> >> 
> >> To rephrase this in generic context a specialization for which a
> >> property does not make sense might come along after the property at the
> >> base class was established.
> >> 
> >> Now AFAIU properties are external API, so having to make a compatibility
> >> breaking change there might not be fun. Does this mean one should be
> >> very careful to put only use class level properties on abstract classes
> >> where its certain that the property always makes sense including it's
> >> access control?
> >
> > This could be an argument for *NOT* allowing introspectiing of properties
> > against abstract parent classes. If you only ever allow introspecting against
> > leaf node non-abstract classes, then QEMU retains the freedom to move props
> > from a base class down to an leaf class without risk of breaking mgmt apps.
> 
> That's a really good point.  To generalize it a bit, introspection of
> actual interfaces is fine, but permitting introspection of how they are
> made can add artificial constraints.
> 
> Introspecting the subtype relation is already problematic in this view.

Yes, that's a very good point. But note that that this means
making things more complex for libvirt.

In the case of -cpu, if we don't expose (or allow libvirt to
making assumptions about) subtype relations, the only way libvirt
can conclude that "+foo can be used as -cpu option with any CPU
model", is to query each and every CPU model type, and see if all
of them support the "foo" property.

It's a trade-off between an interface that's more complex to use
and having less freedom to change the class hierarchy.
Personally, I don't mind going either way, if we have a good
reason for that.

Daniel P. Berrangé Nov. 7, 2016, 5:41 p.m. UTC | #15

On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> > "Daniel P. Berrange" <[e-mail]> writes:
> > 
> > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> > >> 
> > >> 
> > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> > >> > If you want some subclasses to not have the property, then I
> > >> > recommend not registering it as a class property on the base
> > >> > class in the first place. I don't expect to see a mechanism to
> > >> > allow subclasses to remove or override class properties from
> > >> > parent classes.
> > >> > 
> > >> 
> > >> Thank you very much for your reply.
> > >> 
> > >> I understand, yet I see potential problems. The example with ioeventfd
> > >> and vhost in virtio-pci is a good one also because  the first there was
> > >> the ioeventfd property with commit 653ced07 and then the vhost case came
> > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> > >> vhost virtio-pci devices for reasons I do not understand).
> > >> 
> > >> To rephrase this in generic context a specialization for which a
> > >> property does not make sense might come along after the property at the
> > >> base class was established.
> > >> 
> > >> Now AFAIU properties are external API, so having to make a compatibility
> > >> breaking change there might not be fun. Does this mean one should be
> > >> very careful to put only use class level properties on abstract classes
> > >> where its certain that the property always makes sense including it's
> > >> access control?
> > >
> > > This could be an argument for *NOT* allowing introspectiing of properties
> > > against abstract parent classes. If you only ever allow introspecting against
> > > leaf node non-abstract classes, then QEMU retains the freedom to move props
> > > from a base class down to an leaf class without risk of breaking mgmt apps.
> > 
> > That's a really good point.  To generalize it a bit, introspection of
> > actual interfaces is fine, but permitting introspection of how they are
> > made can add artificial constraints.
> > 
> > Introspecting the subtype relation is already problematic in this view.
> 
> Yes, that's a very good point. But note that that this means
> making things more complex for libvirt.
> 
> In the case of -cpu, if we don't expose (or allow libvirt to
> making assumptions about) subtype relations, the only way libvirt
> can conclude that "+foo can be used as -cpu option with any CPU
> model", is to query each and every CPU model type, and see if all
> of them support the "foo" property.
>
> It's a trade-off between an interface that's more complex to use
> and having less freedom to change the class hierarchy.
> Personally, I don't mind going either way, if we have a good
> reason for that.

Or could do a tradeoff where we allow introspection of abstract
parent classes, but explicitly document that we reserve the right
to move properties to leaf nodes ?


Regards,
Daniel

Eduardo Habkost Nov. 7, 2016, 6:03 p.m. UTC | #16

On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
> On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> > > "Daniel P. Berrange" <[e-mail]> writes:
> > > 
> > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> > > >> 
> > > >> 
> > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> > > >> > If you want some subclasses to not have the property, then I
> > > >> > recommend not registering it as a class property on the base
> > > >> > class in the first place. I don't expect to see a mechanism to
> > > >> > allow subclasses to remove or override class properties from
> > > >> > parent classes.
> > > >> > 
> > > >> 
> > > >> Thank you very much for your reply.
> > > >> 
> > > >> I understand, yet I see potential problems. The example with ioeventfd
> > > >> and vhost in virtio-pci is a good one also because  the first there was
> > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
> > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> > > >> vhost virtio-pci devices for reasons I do not understand).
> > > >> 
> > > >> To rephrase this in generic context a specialization for which a
> > > >> property does not make sense might come along after the property at the
> > > >> base class was established.
> > > >> 
> > > >> Now AFAIU properties are external API, so having to make a compatibility
> > > >> breaking change there might not be fun. Does this mean one should be
> > > >> very careful to put only use class level properties on abstract classes
> > > >> where its certain that the property always makes sense including it's
> > > >> access control?
> > > >
> > > > This could be an argument for *NOT* allowing introspectiing of properties
> > > > against abstract parent classes. If you only ever allow introspecting against
> > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
> > > > from a base class down to an leaf class without risk of breaking mgmt apps.
> > > 
> > > That's a really good point.  To generalize it a bit, introspection of
> > > actual interfaces is fine, but permitting introspection of how they are
> > > made can add artificial constraints.
> > > 
> > > Introspecting the subtype relation is already problematic in this view.
> > 
> > Yes, that's a very good point. But note that that this means
> > making things more complex for libvirt.
> > 
> > In the case of -cpu, if we don't expose (or allow libvirt to
> > making assumptions about) subtype relations, the only way libvirt
> > can conclude that "+foo can be used as -cpu option with any CPU
> > model", is to query each and every CPU model type, and see if all
> > of them support the "foo" property.
> >
> > It's a trade-off between an interface that's more complex to use
> > and having less freedom to change the class hierarchy.
> > Personally, I don't mind going either way, if we have a good
> > reason for that.
> 
> Or could do a tradeoff where we allow introspection of abstract
> parent classes, but explicitly document that we reserve the right
> to move properties to leaf nodes ?

Reserving the right to move properties to leaf nodes would be
welcome. But it would force libvirt to query all leaf nodes if it
wants to be sure the option is really unsupported by the QEMU
binary, so why would libvirt query the parent class in the first
place?

Daniel P. Berrangé Nov. 7, 2016, 6:08 p.m. UTC | #17

On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
> On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
> > On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> > > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> > > > "Daniel P. Berrange" <[e-mail]> writes:
> > > > 
> > > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> > > > >> 
> > > > >> 
> > > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> > > > >> > If you want some subclasses to not have the property, then I
> > > > >> > recommend not registering it as a class property on the base
> > > > >> > class in the first place. I don't expect to see a mechanism to
> > > > >> > allow subclasses to remove or override class properties from
> > > > >> > parent classes.
> > > > >> > 
> > > > >> 
> > > > >> Thank you very much for your reply.
> > > > >> 
> > > > >> I understand, yet I see potential problems. The example with ioeventfd
> > > > >> and vhost in virtio-pci is a good one also because  the first there was
> > > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
> > > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> > > > >> vhost virtio-pci devices for reasons I do not understand).
> > > > >> 
> > > > >> To rephrase this in generic context a specialization for which a
> > > > >> property does not make sense might come along after the property at the
> > > > >> base class was established.
> > > > >> 
> > > > >> Now AFAIU properties are external API, so having to make a compatibility
> > > > >> breaking change there might not be fun. Does this mean one should be
> > > > >> very careful to put only use class level properties on abstract classes
> > > > >> where its certain that the property always makes sense including it's
> > > > >> access control?
> > > > >
> > > > > This could be an argument for *NOT* allowing introspectiing of properties
> > > > > against abstract parent classes. If you only ever allow introspecting against
> > > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
> > > > > from a base class down to an leaf class without risk of breaking mgmt apps.
> > > > 
> > > > That's a really good point.  To generalize it a bit, introspection of
> > > > actual interfaces is fine, but permitting introspection of how they are
> > > > made can add artificial constraints.
> > > > 
> > > > Introspecting the subtype relation is already problematic in this view.
> > > 
> > > Yes, that's a very good point. But note that that this means
> > > making things more complex for libvirt.
> > > 
> > > In the case of -cpu, if we don't expose (or allow libvirt to
> > > making assumptions about) subtype relations, the only way libvirt
> > > can conclude that "+foo can be used as -cpu option with any CPU
> > > model", is to query each and every CPU model type, and see if all
> > > of them support the "foo" property.
> > >
> > > It's a trade-off between an interface that's more complex to use
> > > and having less freedom to change the class hierarchy.
> > > Personally, I don't mind going either way, if we have a good
> > > reason for that.
> > 
> > Or could do a tradeoff where we allow introspection of abstract
> > parent classes, but explicitly document that we reserve the right
> > to move properties to leaf nodes ?
> 
> Reserving the right to move properties to leaf nodes would be
> welcome. But it would force libvirt to query all leaf nodes if it
> wants to be sure the option is really unsupported by the QEMU
> binary, so why would libvirt query the parent class in the first
> place?

The introspection API is quite general purpose so its semantics have to
be suitable for all types of object, but some types of object may not need
the full degree of flexibility. So what I meant was that while we want
to be able to move props down to leaf classes for objects in general,
we could perhaps assume that this will never happen for CPU model objects.

Regards,
Daniel

Eduardo Habkost Nov. 7, 2016, 6:28 p.m. UTC | #18

On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
> > On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
> > > On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> > > > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> > > > > "Daniel P. Berrange" <[e-mail]> writes:
> > > > > 
> > > > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> > > > > >> 
> > > > > >> 
> > > > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> > > > > >> > If you want some subclasses to not have the property, then I
> > > > > >> > recommend not registering it as a class property on the base
> > > > > >> > class in the first place. I don't expect to see a mechanism to
> > > > > >> > allow subclasses to remove or override class properties from
> > > > > >> > parent classes.
> > > > > >> > 
> > > > > >> 
> > > > > >> Thank you very much for your reply.
> > > > > >> 
> > > > > >> I understand, yet I see potential problems. The example with ioeventfd
> > > > > >> and vhost in virtio-pci is a good one also because  the first there was
> > > > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
> > > > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> > > > > >> vhost virtio-pci devices for reasons I do not understand).
> > > > > >> 
> > > > > >> To rephrase this in generic context a specialization for which a
> > > > > >> property does not make sense might come along after the property at the
> > > > > >> base class was established.
> > > > > >> 
> > > > > >> Now AFAIU properties are external API, so having to make a compatibility
> > > > > >> breaking change there might not be fun. Does this mean one should be
> > > > > >> very careful to put only use class level properties on abstract classes
> > > > > >> where its certain that the property always makes sense including it's
> > > > > >> access control?
> > > > > >
> > > > > > This could be an argument for *NOT* allowing introspectiing of properties
> > > > > > against abstract parent classes. If you only ever allow introspecting against
> > > > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
> > > > > > from a base class down to an leaf class without risk of breaking mgmt apps.
> > > > > 
> > > > > That's a really good point.  To generalize it a bit, introspection of
> > > > > actual interfaces is fine, but permitting introspection of how they are
> > > > > made can add artificial constraints.
> > > > > 
> > > > > Introspecting the subtype relation is already problematic in this view.
> > > > 
> > > > Yes, that's a very good point. But note that that this means
> > > > making things more complex for libvirt.
> > > > 
> > > > In the case of -cpu, if we don't expose (or allow libvirt to
> > > > making assumptions about) subtype relations, the only way libvirt
> > > > can conclude that "+foo can be used as -cpu option with any CPU
> > > > model", is to query each and every CPU model type, and see if all
> > > > of them support the "foo" property.
> > > >
> > > > It's a trade-off between an interface that's more complex to use
> > > > and having less freedom to change the class hierarchy.
> > > > Personally, I don't mind going either way, if we have a good
> > > > reason for that.
> > > 
> > > Or could do a tradeoff where we allow introspection of abstract
> > > parent classes, but explicitly document that we reserve the right
> > > to move properties to leaf nodes ?
> > 
> > Reserving the right to move properties to leaf nodes would be
> > welcome. But it would force libvirt to query all leaf nodes if it
> > wants to be sure the option is really unsupported by the QEMU
> > binary, so why would libvirt query the parent class in the first
> > place?
> 
> The introspection API is quite general purpose so its semantics have to
> be suitable for all types of object, but some types of object may not need
> the full degree of flexibility. So what I meant was that while we want
> to be able to move props down to leaf classes for objects in general,
> we could perhaps assume that this will never happen for CPU model objects.

This would work for me. I only worry that any code that makes the
wrong assumptions (on either QEMU or libvirt) would easily go
unnoticed until we try to change the class hierarchy and it
breaks something.

Markus, what do you think?

Markus Armbruster Nov. 8, 2016, 7:29 a.m. UTC | #19

Eduardo Habkost <[e-mail]> writes:

> On Mon, Nov 07, 2016 at 03:40:57PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <[e-mail]> writes:
>> 
>> > On Mon, Nov 07, 2016 at 09:09:58AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <[e-mail]> writes:
>> >> 
>> >> > On Fri, Nov 04, 2016 at 04:45:17PM +0100, Markus Armbruster wrote:
>> >> >> Eduardo Habkost <[e-mail]> writes:
>> >> >> 
>> >> >> > (CCing libvirt people, as I forgot to CC them)
>> >> >> >
>> >> >> > On Mon, Oct 31, 2016 at 03:07:23PM +0100, Igor Mammedov wrote:
>> >> >> >> On Fri, 28 Oct 2016 23:48:06 -0200
>> >> >> >> Eduardo Habkost <[e-mail]> wrote:
>> >> >> >> 
>> >> >> >> > When an abstract class is used on device-list-properties, we can
>> >> >> >> > simply return the class properties registered for the class.
>> >> >> >> > 
>> >> >> >> > This will be useful if management software needs to query for
>> >> >> >> > supported options that apply to all devices of a given type (e.g.
>> >> >> >> > options supported by all CPU models, options supported by all PCI
>> >> >> >> > devices).
>> >> >> >> Patch looks fine to me but I'm not qmp interface guru
>> >> >> >> so I'd leave review up to maintainers.
>> >> >> >> 
>> >> >> >> One question though,
>> >> >> >> How would management software discover typename of abstract class?
>> >> >> >
>> >> >> > It depends on the use case. On some cases, management may already
>> >> >> > have bus-specific logic that will know what's the base type it
>> >> >> > needs to query (e.g. it may query "pci-device" to find out if all
>> >> >> > PCI devices support a given option). On other cases, it may be
>> >> >> > discovered using other commands.
>> >> >> 
>> >> >> The stated purpose of this feature is to let management software "query
>> >> >> for supported options that apply to all devices of a given type".  I
>> >> >> suspect that when management software has a notion of "a given type", it
>> >> >> knows its name.
>> >> >> 
>> >> >> Will management software go fishing for subtype relationships beyond the
>> >> >> types it knows?  I doubt it.  Of course, management software developers
>> >> >> are welcome to educate me :)
>> >> >> 
>> >> >> > For the CPU case, I will propose adding the base QOM CPU typename
>> >> >> > in the query-target command.
>> >> >> 
>> >> >> Does this type name vary?  If yes, can you give examples?
>> >> >
>> >> > It does. x86-specific CPU properties are on the x86_64-cpu and
>> >> > i386-cpu classes. arm-specific CPU properties are on the arm-cpu
>> >> > class.
>> >> 
>> >> I see we have concrete CPUs (such as "Westmere-x86_64-cpu"), which are
>> >> subtypes of an abstract CPU (such as "x86_64-cpu"), which is a subtype
>> >> of "cpu", which is a subtype of "device", which is a subtype of
>> >> "object".
>> >> 
>> >> The chain "cpu" - "device" - "object" is fixed and well-known.
>> >> 
>> >> The link from there to the concrete CPU varies.  Whether it could be
>> >> considered well-known or not is debatable.
>> >> 
>> >> My true question is: should we have a special purpose interface to get
>> >> the abstract supertype of concrete CPU types, or should be have general
>> >> purpose means to introspect the subtype hierarchy?
>> >> 
>> >> Note that we have the latter already, although in a rather cumbersome
>> >> form:
>> >> 
>> >>     { "execute": "qom-list-types",
>> >>       "arguments": { "implements": T, "abstract": true } }
>> >> 
>> >> lists all subtypes of T.  You can filter out the concrete subtypes by
>> >> subtracting the same query with "abstract": false.  Start with the
>> >> type you're interested in, find all its abstract supertypes.  If you
>> >> need to know more, repeat for the types you found.
>> >
>> > Looks cumbersome, because I don't see a way to find all
>> > supertypes of a given type without walking the whole tree
>> > starting from "object" (is there one?). But it could be improved
>> > a bit if we added a "implements" field to ObjectTypeInfo.
>> 
>> My point is: we can skip discussing whether we should expose the subtype
>> relation, because we already do.
>
> Correct. My only problem is that it seems to add extra
> assumptions to the code (e.g. that there's only one abstract CPU
> type). But if libvirt is careful, it doesn't need to make any
> assumptions: it can explore the type hierarchy and confirm that
> the assumptions are correct.
>
>> 
>> > But, maybe we should take a step back: my original goal was to
>> > let libvirt know which properties are supported by any CPU model
>> > when using "-cpu".
>> 
>> Why is that useful?
>
> libvirt wants to know if the QEMU binary supports a given -cpu
> option (normally CPU features that can be enabled/disabled using
> "+foo"/"-foo").

The obvious way to check whether a specific CPU supports it is to
introspect that CPU.

The obvious way to check whether all CPUs of interest support it
(assuming that is a productive question) is to introspect all CPUs of
interest.  Works regardless of whether the option is inherited, which is
really an implementation detail.

>> >                    Maybe we should make QEMU do the QOM->option
>> > translation and implement "-cpu" support in
>> > query-command-line-options? We already do something similar with
>> > "-machine".
>> 
>> query-command-line-options is flawed, and the less it's used, the
>> happier I am.
>
> No problem, let's just forget my suggestion.  :)
>
>> 
>> Real command line introspection would be nice, but getting it will
>> involve cleaning up the unholy mess our command line has become.  Don't
>> hold your breath.
>> 
>> My preferred workaround is to find something equivalent to introspect
>> via QMP.
>
> Sounds good to me.
>
>> 
>> That said, if you come up with a patch that solves a real problem now, I
>> won't object to it just because it involves query-command-line-options.
>
> I think I will keep the current approach. Changing
> query-command-line-option would probably be more complex and
> solve only the -cpu problem. Making QOM introspection better
> looks simpler and will probably be useful for other use cases.

Makes sense.

> I am still tempted to try to make QOM->query-command-line-option
> mapping easier and simpler, but it doesn't seem to be the answer
> to the current problem.

Markus Armbruster Nov. 8, 2016, 7:37 a.m. UTC | #20

Eduardo Habkost <[e-mail]> writes:

> On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
>> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
>> > On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
>> > > On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
>> > > > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
>> > > > > "Daniel P. Berrange" <[e-mail]> writes:
>> > > > > 
>> > > > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
>> > > > > >> 
>> > > > > >> 
>> > > > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
>> > > > > >> > If you want some subclasses to not have the property, then I
>> > > > > >> > recommend not registering it as a class property on the base
>> > > > > >> > class in the first place. I don't expect to see a mechanism to
>> > > > > >> > allow subclasses to remove or override class properties from
>> > > > > >> > parent classes.
>> > > > > >> > 
>> > > > > >> 
>> > > > > >> Thank you very much for your reply.
>> > > > > >> 
>> > > > > >> I understand, yet I see potential problems. The example with ioeventfd
>> > > > > >> and vhost in virtio-pci is a good one also because  the first there was
>> > > > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
>> > > > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
>> > > > > >> vhost virtio-pci devices for reasons I do not understand).
>> > > > > >> 
>> > > > > >> To rephrase this in generic context a specialization for which a
>> > > > > >> property does not make sense might come along after the property at the
>> > > > > >> base class was established.
>> > > > > >> 
>> > > > > >> Now AFAIU properties are external API, so having to make a compatibility
>> > > > > >> breaking change there might not be fun. Does this mean one should be
>> > > > > >> very careful to put only use class level properties on abstract classes
>> > > > > >> where its certain that the property always makes sense including it's
>> > > > > >> access control?
>> > > > > >
>> > > > > > This could be an argument for *NOT* allowing introspectiing of properties
>> > > > > > against abstract parent classes. If you only ever allow introspecting against
>> > > > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
>> > > > > > from a base class down to an leaf class without risk of breaking mgmt apps.
>> > > > > 
>> > > > > That's a really good point.  To generalize it a bit, introspection of
>> > > > > actual interfaces is fine, but permitting introspection of how they are
>> > > > > made can add artificial constraints.
>> > > > > 
>> > > > > Introspecting the subtype relation is already problematic in this view.
>> > > > 
>> > > > Yes, that's a very good point. But note that that this means
>> > > > making things more complex for libvirt.
>> > > > 
>> > > > In the case of -cpu, if we don't expose (or allow libvirt to
>> > > > making assumptions about) subtype relations, the only way libvirt
>> > > > can conclude that "+foo can be used as -cpu option with any CPU
>> > > > model", is to query each and every CPU model type, and see if all
>> > > > of them support the "foo" property.
>> > > >
>> > > > It's a trade-off between an interface that's more complex to use
>> > > > and having less freedom to change the class hierarchy.
>> > > > Personally, I don't mind going either way, if we have a good
>> > > > reason for that.
>> > > 
>> > > Or could do a tradeoff where we allow introspection of abstract
>> > > parent classes, but explicitly document that we reserve the right
>> > > to move properties to leaf nodes ?
>> > 
>> > Reserving the right to move properties to leaf nodes would be
>> > welcome. But it would force libvirt to query all leaf nodes if it
>> > wants to be sure the option is really unsupported by the QEMU
>> > binary, so why would libvirt query the parent class in the first
>> > place?
>> 
>> The introspection API is quite general purpose so its semantics have to
>> be suitable for all types of object, but some types of object may not need
>> the full degree of flexibility. So what I meant was that while we want
>> to be able to move props down to leaf classes for objects in general,
>> we could perhaps assume that this will never happen for CPU model objects.
>
> This would work for me. I only worry that any code that makes the
> wrong assumptions (on either QEMU or libvirt) would easily go
> unnoticed until we try to change the class hierarchy and it
> breaks something.
>
> Markus, what do you think?

I dislike complexity in interface contracts.

Guidance like "if you want to learn the properties of a type T,
introspect T" is simple.

Guidance like "if you want to learn the properties common to all
subtypes of T, you need to introspect all subtypes of T, except when T
is "cpu", you can take a shortcut and introspect T instead" is not
simple, and the precedent opens the gates to even more complexity.

Is introspecting all CPU types of interest really that bad?

Daniel P. Berrangé Nov. 8, 2016, 10:09 a.m. UTC | #21

On Tue, Nov 08, 2016 at 08:37:20AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <[e-mail]> writes:
> 
> > On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
> >> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
> >> > On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
> >> > > On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> >> > > > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> >> > > > > "Daniel P. Berrange" <[e-mail]> writes:
> >> > > > > 
> >> > > > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> >> > > > > >> 
> >> > > > > >> 
> >> > > > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> >> > > > > >> > If you want some subclasses to not have the property, then I
> >> > > > > >> > recommend not registering it as a class property on the base
> >> > > > > >> > class in the first place. I don't expect to see a mechanism to
> >> > > > > >> > allow subclasses to remove or override class properties from
> >> > > > > >> > parent classes.
> >> > > > > >> > 
> >> > > > > >> 
> >> > > > > >> Thank you very much for your reply.
> >> > > > > >> 
> >> > > > > >> I understand, yet I see potential problems. The example with ioeventfd
> >> > > > > >> and vhost in virtio-pci is a good one also because  the first there was
> >> > > > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
> >> > > > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> >> > > > > >> vhost virtio-pci devices for reasons I do not understand).
> >> > > > > >> 
> >> > > > > >> To rephrase this in generic context a specialization for which a
> >> > > > > >> property does not make sense might come along after the property at the
> >> > > > > >> base class was established.
> >> > > > > >> 
> >> > > > > >> Now AFAIU properties are external API, so having to make a compatibility
> >> > > > > >> breaking change there might not be fun. Does this mean one should be
> >> > > > > >> very careful to put only use class level properties on abstract classes
> >> > > > > >> where its certain that the property always makes sense including it's
> >> > > > > >> access control?
> >> > > > > >
> >> > > > > > This could be an argument for *NOT* allowing introspectiing of properties
> >> > > > > > against abstract parent classes. If you only ever allow introspecting against
> >> > > > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
> >> > > > > > from a base class down to an leaf class without risk of breaking mgmt apps.
> >> > > > > 
> >> > > > > That's a really good point.  To generalize it a bit, introspection of
> >> > > > > actual interfaces is fine, but permitting introspection of how they are
> >> > > > > made can add artificial constraints.
> >> > > > > 
> >> > > > > Introspecting the subtype relation is already problematic in this view.
> >> > > > 
> >> > > > Yes, that's a very good point. But note that that this means
> >> > > > making things more complex for libvirt.
> >> > > > 
> >> > > > In the case of -cpu, if we don't expose (or allow libvirt to
> >> > > > making assumptions about) subtype relations, the only way libvirt
> >> > > > can conclude that "+foo can be used as -cpu option with any CPU
> >> > > > model", is to query each and every CPU model type, and see if all
> >> > > > of them support the "foo" property.
> >> > > >
> >> > > > It's a trade-off between an interface that's more complex to use
> >> > > > and having less freedom to change the class hierarchy.
> >> > > > Personally, I don't mind going either way, if we have a good
> >> > > > reason for that.
> >> > > 
> >> > > Or could do a tradeoff where we allow introspection of abstract
> >> > > parent classes, but explicitly document that we reserve the right
> >> > > to move properties to leaf nodes ?
> >> > 
> >> > Reserving the right to move properties to leaf nodes would be
> >> > welcome. But it would force libvirt to query all leaf nodes if it
> >> > wants to be sure the option is really unsupported by the QEMU
> >> > binary, so why would libvirt query the parent class in the first
> >> > place?
> >> 
> >> The introspection API is quite general purpose so its semantics have to
> >> be suitable for all types of object, but some types of object may not need
> >> the full degree of flexibility. So what I meant was that while we want
> >> to be able to move props down to leaf classes for objects in general,
> >> we could perhaps assume that this will never happen for CPU model objects.
> >
> > This would work for me. I only worry that any code that makes the
> > wrong assumptions (on either QEMU or libvirt) would easily go
> > unnoticed until we try to change the class hierarchy and it
> > breaks something.
> >
> > Markus, what do you think?
> 
> I dislike complexity in interface contracts.
> 
> Guidance like "if you want to learn the properties of a type T,
> introspect T" is simple.
> 
> Guidance like "if you want to learn the properties common to all
> subtypes of T, you need to introspect all subtypes of T, except when T
> is "cpu", you can take a shortcut and introspect T instead" is not
> simple, and the precedent opens the gates to even more complexity.
> 
> Is introspecting all CPU types of interest really that bad?

I'm no sure - adding Jiri who'll ultimately be writing this code ?

If we have to introspect M cpu flags * N cpu models, this will get slow
very quickly as IIRC there's 100+ cpu flags, and 10+ models, so 1000+
combinations

Regards,
Daniel

Halil Pasic Nov. 8, 2016, 10:11 a.m. UTC | #22

On 11/08/2016 08:37 AM, Markus Armbruster wrote:
> Eduardo Habkost <[e-mail]> writes:
> 
>> On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
>>> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
>>>> On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
>>>>> On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
>>>>>> On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
>>>>>>> "Daniel P. Berrange" <[e-mail]> writes:
>>>>>>>
>>>>>>>> On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
>>>>>>>>>> If you want some subclasses to not have the property, then I
>>>>>>>>>> recommend not registering it as a class property on the base
>>>>>>>>>> class in the first place. I don't expect to see a mechanism to
>>>>>>>>>> allow subclasses to remove or override class properties from
>>>>>>>>>> parent classes.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thank you very much for your reply.
>>>>>>>>>
>>>>>>>>> I understand, yet I see potential problems. The example with ioeventfd
>>>>>>>>> and vhost in virtio-pci is a good one also because  the first there was
>>>>>>>>> the ioeventfd property with commit 653ced07 and then the vhost case came
>>>>>>>>> along with commit 50787628ee3 (ok ioeventfd is not there for some non
>>>>>>>>> vhost virtio-pci devices for reasons I do not understand).
>>>>>>>>>
>>>>>>>>> To rephrase this in generic context a specialization for which a
>>>>>>>>> property does not make sense might come along after the property at the
>>>>>>>>> base class was established.
>>>>>>>>>
>>>>>>>>> Now AFAIU properties are external API, so having to make a compatibility
>>>>>>>>> breaking change there might not be fun. Does this mean one should be
>>>>>>>>> very careful to put only use class level properties on abstract classes
>>>>>>>>> where its certain that the property always makes sense including it's
>>>>>>>>> access control?
>>>>>>>>
>>>>>>>> This could be an argument for *NOT* allowing introspectiing of properties
>>>>>>>> against abstract parent classes. If you only ever allow introspecting against
>>>>>>>> leaf node non-abstract classes, then QEMU retains the freedom to move props
>>>>>>>> from a base class down to an leaf class without risk of breaking mgmt apps.
>>>>>>>
>>>>>>> That's a really good point.  To generalize it a bit, introspection of
>>>>>>> actual interfaces is fine, but permitting introspection of how they are
>>>>>>> made can add artificial constraints.
>>>>>>>
>>>>>>> Introspecting the subtype relation is already problematic in this view.
>>>>>>
>>>>>> Yes, that's a very good point. But note that that this means
>>>>>> making things more complex for libvirt.
>>>>>>
>>>>>> In the case of -cpu, if we don't expose (or allow libvirt to
>>>>>> making assumptions about) subtype relations, the only way libvirt
>>>>>> can conclude that "+foo can be used as -cpu option with any CPU
>>>>>> model", is to query each and every CPU model type, and see if all
>>>>>> of them support the "foo" property.
>>>>>>
>>>>>> It's a trade-off between an interface that's more complex to use
>>>>>> and having less freedom to change the class hierarchy.
>>>>>> Personally, I don't mind going either way, if we have a good
>>>>>> reason for that.
>>>>>
>>>>> Or could do a tradeoff where we allow introspection of abstract
>>>>> parent classes, but explicitly document that we reserve the right
>>>>> to move properties to leaf nodes ?
>>>>
>>>> Reserving the right to move properties to leaf nodes would be
>>>> welcome. But it would force libvirt to query all leaf nodes if it
>>>> wants to be sure the option is really unsupported by the QEMU
>>>> binary, so why would libvirt query the parent class in the first
>>>> place?
>>>
>>> The introspection API is quite general purpose so its semantics have to
>>> be suitable for all types of object, but some types of object may not need
>>> the full degree of flexibility. So what I meant was that while we want
>>> to be able to move props down to leaf classes for objects in general,
>>> we could perhaps assume that this will never happen for CPU model objects.
>>
>> This would work for me. I only worry that any code that makes the
>> wrong assumptions (on either QEMU or libvirt) would easily go
>> unnoticed until we try to change the class hierarchy and it
>> breaks something.
>>
>> Markus, what do you think?
> 
> I dislike complexity in interface contracts.
> 
> Guidance like "if you want to learn the properties of a type T,
> introspect T" is simple.
> 
> Guidance like "if you want to learn the properties common to all
> subtypes of T, you need to introspect all subtypes of T, except when T
> is "cpu", you can take a shortcut and introspect T instead" is not
> simple, and the precedent opens the gates to even more complexity.
> 

+1

> Is introspecting all CPU types of interest really that bad?
> 

A simple to use interface could eventually also be achieved by introducing
operations for these things which currently seem cumbersome with 
(e.g. properties common to all subtypes of T) the current set of (primitive)
operations. This would also leave us more degrees of freedom regarding
implementation details and also other stuff. (Sorry if this was
already discussed with previous versions of the patchset).

Regards,
Halil

Markus Armbruster Nov. 8, 2016, 2:35 p.m. UTC | #23

"Daniel P. Berrange" <[e-mail]> writes:

> On Tue, Nov 08, 2016 at 08:37:20AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <[e-mail]> writes:
>> 
>> > On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
>> >> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
>> >> > On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
>> >> > > On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
>> >> > > > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
>> >> > > > > "Daniel P. Berrange" <[e-mail]> writes:
>> >> > > > > 
>> >> > > > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
>> >> > > > > >> 
>> >> > > > > >> 
>> >> > > > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
>> >> > > > > >> > If you want some subclasses to not have the property, then I
>> >> > > > > >> > recommend not registering it as a class property on the base
>> >> > > > > >> > class in the first place. I don't expect to see a mechanism to
>> >> > > > > >> > allow subclasses to remove or override class properties from
>> >> > > > > >> > parent classes.
>> >> > > > > >> > 
>> >> > > > > >> 
>> >> > > > > >> Thank you very much for your reply.
>> >> > > > > >> 
>> >> > > > > >> I understand, yet I see potential problems. The example with ioeventfd
>> >> > > > > >> and vhost in virtio-pci is a good one also because  the first there was
>> >> > > > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
>> >> > > > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
>> >> > > > > >> vhost virtio-pci devices for reasons I do not understand).
>> >> > > > > >> 
>> >> > > > > >> To rephrase this in generic context a specialization for which a
>> >> > > > > >> property does not make sense might come along after the property at the
>> >> > > > > >> base class was established.
>> >> > > > > >> 
>> >> > > > > >> Now AFAIU properties are external API, so having to make a compatibility
>> >> > > > > >> breaking change there might not be fun. Does this mean one should be
>> >> > > > > >> very careful to put only use class level properties on abstract classes
>> >> > > > > >> where its certain that the property always makes sense including it's
>> >> > > > > >> access control?
>> >> > > > > >
>> >> > > > > > This could be an argument for *NOT* allowing introspectiing of properties
>> >> > > > > > against abstract parent classes. If you only ever allow introspecting against
>> >> > > > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
>> >> > > > > > from a base class down to an leaf class without risk of breaking mgmt apps.
>> >> > > > > 
>> >> > > > > That's a really good point.  To generalize it a bit, introspection of
>> >> > > > > actual interfaces is fine, but permitting introspection of how they are
>> >> > > > > made can add artificial constraints.
>> >> > > > > 
>> >> > > > > Introspecting the subtype relation is already problematic in this view.
>> >> > > > 
>> >> > > > Yes, that's a very good point. But note that that this means
>> >> > > > making things more complex for libvirt.
>> >> > > > 
>> >> > > > In the case of -cpu, if we don't expose (or allow libvirt to
>> >> > > > making assumptions about) subtype relations, the only way libvirt
>> >> > > > can conclude that "+foo can be used as -cpu option with any CPU
>> >> > > > model", is to query each and every CPU model type, and see if all
>> >> > > > of them support the "foo" property.
>> >> > > >
>> >> > > > It's a trade-off between an interface that's more complex to use
>> >> > > > and having less freedom to change the class hierarchy.
>> >> > > > Personally, I don't mind going either way, if we have a good
>> >> > > > reason for that.
>> >> > > 
>> >> > > Or could do a tradeoff where we allow introspection of abstract
>> >> > > parent classes, but explicitly document that we reserve the right
>> >> > > to move properties to leaf nodes ?
>> >> > 
>> >> > Reserving the right to move properties to leaf nodes would be
>> >> > welcome. But it would force libvirt to query all leaf nodes if it
>> >> > wants to be sure the option is really unsupported by the QEMU
>> >> > binary, so why would libvirt query the parent class in the first
>> >> > place?
>> >> 
>> >> The introspection API is quite general purpose so its semantics have to
>> >> be suitable for all types of object, but some types of object may not need
>> >> the full degree of flexibility. So what I meant was that while we want
>> >> to be able to move props down to leaf classes for objects in general,
>> >> we could perhaps assume that this will never happen for CPU model objects.
>> >
>> > This would work for me. I only worry that any code that makes the
>> > wrong assumptions (on either QEMU or libvirt) would easily go
>> > unnoticed until we try to change the class hierarchy and it
>> > breaks something.
>> >
>> > Markus, what do you think?
>> 
>> I dislike complexity in interface contracts.
>> 
>> Guidance like "if you want to learn the properties of a type T,
>> introspect T" is simple.
>> 
>> Guidance like "if you want to learn the properties common to all
>> subtypes of T, you need to introspect all subtypes of T, except when T
>> is "cpu", you can take a shortcut and introspect T instead" is not
>> simple, and the precedent opens the gates to even more complexity.
>> 
>> Is introspecting all CPU types of interest really that bad?
>
> I'm no sure - adding Jiri who'll ultimately be writing this code ?
>
> If we have to introspect M cpu flags * N cpu models, this will get slow
> very quickly as IIRC there's 100+ cpu flags, and 10+ models, so 1000+
> combinations

for CPU in CPUs
    if this is the first one
        common_flags = CPU's flags
    else
        common_flags &= CPU's flags

Yes, that's O(M*N), but the I/O is O(N): you query for each CPU just
once.  I'd expect I/O to dominate even with hundreds of CPU flags.

In general, if a management application introspects the same things via
QMP multiple times, it's probably doing it wrong.

Eduardo Habkost Nov. 8, 2016, 4:16 p.m. UTC | #24

On Tue, Nov 08, 2016 at 03:35:08PM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <[e-mail]> writes:
> 
> > On Tue, Nov 08, 2016 at 08:37:20AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <[e-mail]> writes:
> >> 
> >> > On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
> >> >> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
> >> >> > On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
> >> >> > > On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> >> >> > > > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> >> >> > > > > "Daniel P. Berrange" <[e-mail]> writes:
> >> >> > > > > 
> >> >> > > > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> >> >> > > > > >> 
> >> >> > > > > >> 
> >> >> > > > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> >> >> > > > > >> > If you want some subclasses to not have the property, then I
> >> >> > > > > >> > recommend not registering it as a class property on the base
> >> >> > > > > >> > class in the first place. I don't expect to see a mechanism to
> >> >> > > > > >> > allow subclasses to remove or override class properties from
> >> >> > > > > >> > parent classes.
> >> >> > > > > >> > 
> >> >> > > > > >> 
> >> >> > > > > >> Thank you very much for your reply.
> >> >> > > > > >> 
> >> >> > > > > >> I understand, yet I see potential problems. The example with ioeventfd
> >> >> > > > > >> and vhost in virtio-pci is a good one also because  the first there was
> >> >> > > > > >> the ioeventfd property with commit 653ced07 and then the vhost case came
> >> >> > > > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> >> >> > > > > >> vhost virtio-pci devices for reasons I do not understand).
> >> >> > > > > >> 
> >> >> > > > > >> To rephrase this in generic context a specialization for which a
> >> >> > > > > >> property does not make sense might come along after the property at the
> >> >> > > > > >> base class was established.
> >> >> > > > > >> 
> >> >> > > > > >> Now AFAIU properties are external API, so having to make a compatibility
> >> >> > > > > >> breaking change there might not be fun. Does this mean one should be
> >> >> > > > > >> very careful to put only use class level properties on abstract classes
> >> >> > > > > >> where its certain that the property always makes sense including it's
> >> >> > > > > >> access control?
> >> >> > > > > >
> >> >> > > > > > This could be an argument for *NOT* allowing introspectiing of properties
> >> >> > > > > > against abstract parent classes. If you only ever allow introspecting against
> >> >> > > > > > leaf node non-abstract classes, then QEMU retains the freedom to move props
> >> >> > > > > > from a base class down to an leaf class without risk of breaking mgmt apps.
> >> >> > > > > 
> >> >> > > > > That's a really good point.  To generalize it a bit, introspection of
> >> >> > > > > actual interfaces is fine, but permitting introspection of how they are
> >> >> > > > > made can add artificial constraints.
> >> >> > > > > 
> >> >> > > > > Introspecting the subtype relation is already problematic in this view.
> >> >> > > > 
> >> >> > > > Yes, that's a very good point. But note that that this means
> >> >> > > > making things more complex for libvirt.
> >> >> > > > 
> >> >> > > > In the case of -cpu, if we don't expose (or allow libvirt to
> >> >> > > > making assumptions about) subtype relations, the only way libvirt
> >> >> > > > can conclude that "+foo can be used as -cpu option with any CPU
> >> >> > > > model", is to query each and every CPU model type, and see if all
> >> >> > > > of them support the "foo" property.
> >> >> > > >
> >> >> > > > It's a trade-off between an interface that's more complex to use
> >> >> > > > and having less freedom to change the class hierarchy.
> >> >> > > > Personally, I don't mind going either way, if we have a good
> >> >> > > > reason for that.
> >> >> > > 
> >> >> > > Or could do a tradeoff where we allow introspection of abstract
> >> >> > > parent classes, but explicitly document that we reserve the right
> >> >> > > to move properties to leaf nodes ?
> >> >> > 
> >> >> > Reserving the right to move properties to leaf nodes would be
> >> >> > welcome. But it would force libvirt to query all leaf nodes if it
> >> >> > wants to be sure the option is really unsupported by the QEMU
> >> >> > binary, so why would libvirt query the parent class in the first
> >> >> > place?
> >> >> 
> >> >> The introspection API is quite general purpose so its semantics have to
> >> >> be suitable for all types of object, but some types of object may not need
> >> >> the full degree of flexibility. So what I meant was that while we want
> >> >> to be able to move props down to leaf classes for objects in general,
> >> >> we could perhaps assume that this will never happen for CPU model objects.
> >> >
> >> > This would work for me. I only worry that any code that makes the
> >> > wrong assumptions (on either QEMU or libvirt) would easily go
> >> > unnoticed until we try to change the class hierarchy and it
> >> > breaks something.
> >> >
> >> > Markus, what do you think?
> >> 
> >> I dislike complexity in interface contracts.
> >> 
> >> Guidance like "if you want to learn the properties of a type T,
> >> introspect T" is simple.
> >> 
> >> Guidance like "if you want to learn the properties common to all
> >> subtypes of T, you need to introspect all subtypes of T, except when T
> >> is "cpu", you can take a shortcut and introspect T instead" is not
> >> simple, and the precedent opens the gates to even more complexity.
> >> 
> >> Is introspecting all CPU types of interest really that bad?
> >
> > I'm no sure - adding Jiri who'll ultimately be writing this code ?
> >
> > If we have to introspect M cpu flags * N cpu models, this will get slow
> > very quickly as IIRC there's 100+ cpu flags, and 10+ models, so 1000+
> > combinations
> 
> for CPU in CPUs
>     if this is the first one
>         common_flags = CPU's flags
>     else
>         common_flags &= CPU's flags
> 
> Yes, that's O(M*N), but the I/O is O(N): you query for each CPU just
> once.  I'd expect I/O to dominate even with hundreds of CPU flags.
> 
> In general, if a management application introspects the same things via
> QMP multiple times, it's probably doing it wrong.

Or they could query only the CPU model that is being used for a
VM, when validating the VM configuration. But I'm not sure when
exactly this information is going to be used by libvirt.

Jiri Denemark Nov. 11, 2016, 12:17 p.m. UTC | #25

On Tue, Nov 08, 2016 at 08:29:41 +0100, Markus Armbruster wrote:
> Eduardo Habkost <[e-mail]> writes:
> > libvirt wants to know if the QEMU binary supports a given -cpu
> > option (normally CPU features that can be enabled/disabled using
> > "+foo"/"-foo").
> 
> The obvious way to check whether a specific CPU supports it is to
> introspect that CPU.
> 
> The obvious way to check whether all CPUs of interest support it
> (assuming that is a productive question) is to introspect all CPUs of
> interest.  Works regardless of whether the option is inherited, which is
> really an implementation detail.

As Eduardo said, libvirt wants to know whether it can use a given CPU
feature with current QEMU binary. In -cpu help, we can see a list of
models and features QEMU understands, but we need to get similar info
via QMP. CPU models are easy, but how do we get the list of CPU
features? If introspection is the way to get it, I'm fine with that,
just beware that we don't actually know the name of the CPU object
(Westmere-x86_64-cpu), we only know the name of the CPU model
(Westmere). So if the object name is needed, we need to query the
mapping from CPU model names to CPU object names.

Jirka

Eduardo Habkost Nov. 11, 2016, 12:34 p.m. UTC | #26

On Fri, Nov 11, 2016 at 01:17:44PM +0100, Jiri Denemark wrote:
> On Tue, Nov 08, 2016 at 08:29:41 +0100, Markus Armbruster wrote:
> > Eduardo Habkost <[e-mail]> writes:
> > > libvirt wants to know if the QEMU binary supports a given -cpu
> > > option (normally CPU features that can be enabled/disabled using
> > > "+foo"/"-foo").
> > 
> > The obvious way to check whether a specific CPU supports it is to
> > introspect that CPU.
> > 
> > The obvious way to check whether all CPUs of interest support it
> > (assuming that is a productive question) is to introspect all CPUs of
> > interest.  Works regardless of whether the option is inherited, which is
> > really an implementation detail.
> 
> As Eduardo said, libvirt wants to know whether it can use a given CPU
> feature with current QEMU binary. In -cpu help, we can see a list of
> models and features QEMU understands, but we need to get similar info
> via QMP. CPU models are easy, but how do we get the list of CPU
> features? If introspection is the way to get it, I'm fine with that,
> just beware that we don't actually know the name of the CPU object
> (Westmere-x86_64-cpu), we only know the name of the CPU model
> (Westmere). So if the object name is needed, we need to query the
> mapping from CPU model names to CPU object names.

I have patches to add the QOM type name to query-cpu-definitions.
I don't remember if I submitted them to qemu-devel already. I
will check.

Markus Armbruster Nov. 11, 2016, 2:29 p.m. UTC | #27

Jiri Denemark <[e-mail]> writes:

> On Tue, Nov 08, 2016 at 08:29:41 +0100, Markus Armbruster wrote:
>> Eduardo Habkost <[e-mail]> writes:
>> > libvirt wants to know if the QEMU binary supports a given -cpu
>> > option (normally CPU features that can be enabled/disabled using
>> > "+foo"/"-foo").
>> 
>> The obvious way to check whether a specific CPU supports it is to
>> introspect that CPU.
>> 
>> The obvious way to check whether all CPUs of interest support it
>> (assuming that is a productive question) is to introspect all CPUs of
>> interest.  Works regardless of whether the option is inherited, which is
>> really an implementation detail.
>
> As Eduardo said, libvirt wants to know whether it can use a given CPU
> feature with current QEMU binary. In -cpu help, we can see a list of
> models and features QEMU understands, but we need to get similar info
> via QMP. CPU models are easy, but how do we get the list of CPU
> features? If introspection is the way to get it, I'm fine with that,
> just beware that we don't actually know the name of the CPU object
> (Westmere-x86_64-cpu), we only know the name of the CPU model
> (Westmere).

Actually, you do:

    { "execute": "qom-list-types", "arguments": { "implements": "cpu" } }
    {"return": [{"name": "Westmere-x86_64-cpu"}, ...]}

>             So if the object name is needed, we need to query the
> mapping from CPU model names to CPU object names.

Hmm.  The problem is that some interfaces such as -cpu require a "CPU
model name", but introspection gives you the QOM type name.  The mapping
from "model name" to type name even depends on the target: it's CPUClass
method class_by_name().

Can't say I like that, but we got to play the hand we were dealt, and
that means we need to provide a way to introspect the mapping between
CPU model name and QOM type name.  Eduardo's plan to add the QOM type
name to query-cpu-definitions makes more sense to me now.

Patch

diff --git a/qmp.c b/qmp.c
index a06cb7b..1e7e60d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -518,7 +518,7 @@  DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
                                                    Error **errp)
 {
     ObjectClass *klass;
-    Object *obj;
+    Object *obj = NULL;
     ObjectProperty *prop;
     ObjectPropertyIterator iter;
     DevicePropertyInfoList *prop_list = NULL;
@@ -537,19 +537,16 @@  DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
     }
 
     if (object_class_is_abstract(klass)) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name",
-                   "non-abstract device type");
-        return NULL;
-    }
-
-    if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
-        error_setg(errp, "Can't list properties of device '%s'", typename);
-        return NULL;
+        object_class_property_iter_init(&iter, klass);
+    } else {
+        if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
+            error_setg(errp, "Can't list properties of device '%s'", typename);
+            return NULL;
+        }
+        obj = object_new(typename);
+        object_property_iter_init(&iter, obj);
     }
 
-    obj = object_new(typename);
-
-    object_property_iter_init(&iter, obj);
     while ((prop = object_property_iter_next(&iter))) {
         DevicePropertyInfo *info;
         DevicePropertyInfoList *entry;