| Message ID | 20180119050906.18930-1-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,qemu] qmp: Add qom-list-properties to list QOM object properties | expand |
On Fri, Jan 19, 2018 at 04:09:06PM +1100, Alexey Kardashevskiy wrote: > There is already 'device-list-properties' which does most of the job, > however it does not handle everything returned by qom-list-types such > as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. > > This adds a new qom-list-properties command which prints properties > of a specific class and its instance. It is pretty much a simplified copy > of the device-list-properties handler. > > Since it creates an object instance, device properties should appear > in the output as they are copied to QOM properties at the instance_init > hook. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> I think the existing qom-list interface does this already. This won't solve the libvirt problem we were discussing, because it needs an existing instance of the object. libvirt wants to know the machine properties *without* instantiating an instance.
On 19/01/18 16:19, David Gibson wrote: > On Fri, Jan 19, 2018 at 04:09:06PM +1100, Alexey Kardashevskiy wrote: >> There is already 'device-list-properties' which does most of the job, >> however it does not handle everything returned by qom-list-types such >> as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. >> >> This adds a new qom-list-properties command which prints properties >> of a specific class and its instance. It is pretty much a simplified copy >> of the device-list-properties handler. >> >> Since it creates an object instance, device properties should appear >> in the output as they are copied to QOM properties at the instance_init >> hook. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > I think the existing qom-list interface does this already. Nope, it does not. It takes path, not a type, so running with "-machine none" won't help. > This won't solve the libvirt problem we were discussing, because it > needs an existing instance of the object. libvirt wants to know the > machine properties *without* instantiating an instance. My patch works with types, it creates an instance for a short time itself, this is why it does not do a thing for "pseries" as it is not a type and prints properties for the "pseries-2.12-machine" type.
On 19/01/18 17:15, Alexey Kardashevskiy wrote: > On 19/01/18 16:19, David Gibson wrote: >> On Fri, Jan 19, 2018 at 04:09:06PM +1100, Alexey Kardashevskiy wrote: >>> There is already 'device-list-properties' which does most of the job, >>> however it does not handle everything returned by qom-list-types such >>> as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. >>> >>> This adds a new qom-list-properties command which prints properties >>> of a specific class and its instance. It is pretty much a simplified copy >>> of the device-list-properties handler. >>> >>> Since it creates an object instance, device properties should appear >>> in the output as they are copied to QOM properties at the instance_init >>> hook. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> I think the existing qom-list interface does this already. > > Nope, it does not. It takes path, not a type, so running with "-machine > none" won't help. > >> This won't solve the libvirt problem we were discussing, because it >> needs an existing instance of the object. libvirt wants to know the >> machine properties *without* instantiating an instance. > > My patch works with types, it creates an instance for a short time itself, > this is why it does not do a thing for "pseries" as it is not a type and > prints properties for the "pseries-2.12-machine" type. This is what I mean by "none": ./qemu-system-ppc64 -machine none -nodefaults -nographic -chardev socket,id=SOCKET0,server,nowait,host=localhost,port=40000 -mon chardev=SOCKET0,mode=control {"arguments": {"typename": "pseries-2.11-machine"}, "execute": "qom-list-properties"} still returns expected result on the 'none' machine.
On Fri, 2018-01-19 at 17:15 +1100, Alexey Kardashevskiy wrote: > > I think the existing qom-list interface does this already. > > Nope, it does not. It takes path, not a type, so running with "-machine > none" won't help. > > > This won't solve the libvirt problem we were discussing, because it > > needs an existing instance of the object. libvirt wants to know the > > machine properties *without* instantiating an instance. > > My patch works with types, it creates an instance for a short time itself, > this is why it does not do a thing for "pseries" as it is not a type and > prints properties for the "pseries-2.12-machine" type. Yeah, I took this for a spin and can confirm that it's pretty much exactly what I was thinking about. The fact that the QMP command instantiates objects behind the scenes is not an issue, at least from libvirt's point of view: device-list-properties does the same thing and we already use it quite happily; what matters is that we can call this, along with all the other capabilities-collecting QMP commands, in one go and on a single QEMU instance.
On Fri, 2018-01-19 at 15:34 +0100, Andrea Bolognani wrote: > > > This won't solve the libvirt problem we were discussing, because it > > > needs an existing instance of the object. libvirt wants to know the > > > machine properties *without* instantiating an instance. > > > > My patch works with types, it creates an instance for a short time itself, > > this is why it does not do a thing for "pseries" as it is not a type and > > prints properties for the "pseries-2.12-machine" type. > > Yeah, I took this for a spin and can confirm that it's pretty much > exactly what I was thinking about. The fact that the QMP command > instantiates objects behind the scenes is not an issue, at least > from libvirt's point of view: device-list-properties does the same > thing and we already use it quite happily; what matters is that we > can call this, along with all the other capabilities-collecting > QMP commands, in one go and on a single QEMU instance. David, I know you're busy with linux.conf.au, but it would be really helpful if you could carve out five minutes to look over Alexey's proposal again, with my reply above in mind, and let us know whether it looks a reasonable design. Doesn't have to be a review, just a quick feedback on the high-level idea. I'm moving forward with the libvirt implementation of pSeries capabilities and I would have to start implementing support for this new QMP command, well, pretty much... Right now :) But I'd rather not start at all if I'm just going to have to scrap everything later. Thanks.
On Tue, Jan 23, 2018 at 11:08:31AM +0100, Andrea Bolognani wrote: > On Fri, 2018-01-19 at 15:34 +0100, Andrea Bolognani wrote: > > > > This won't solve the libvirt problem we were discussing, because it > > > > needs an existing instance of the object. libvirt wants to know the > > > > machine properties *without* instantiating an instance. > > > > > > My patch works with types, it creates an instance for a short time itself, > > > this is why it does not do a thing for "pseries" as it is not a type and > > > prints properties for the "pseries-2.12-machine" type. > > > > Yeah, I took this for a spin and can confirm that it's pretty much > > exactly what I was thinking about. The fact that the QMP command > > instantiates objects behind the scenes is not an issue, at least > > from libvirt's point of view: device-list-properties does the same > > thing and we already use it quite happily; what matters is that we > > can call this, along with all the other capabilities-collecting > > QMP commands, in one go and on a single QEMU instance. > > David, I know you're busy with linux.conf.au, but it would be > really helpful if you could carve out five minutes to look over > Alexey's proposal again, with my reply above in mind, and let us > know whether it looks a reasonable design. Doesn't have to be a > review, just a quick feedback on the high-level idea. It looks ok, I think, but I don't think I'm really the right person to ask. I do wonder if creating a throwaway instance could cause trouble, especially for something like machine that might well have gotten away with having global side-effects in the past. I think we need to talk with someone who knows more about qom and qapi - Markus seems the obvious choice. > I'm moving forward with the libvirt implementation of pSeries > capabilities and I would have to start implementing support for > this new QMP command, well, pretty much... Right now :) But I'd > rather not start at all if I'm just going to have to scrap > everything later. Yeah, unfortunately because its part of the core infrastructure, not power specific, this isn't something I can make call on.
On Tue, 2018-01-23 at 22:20 +1100, David Gibson wrote: > > David, I know you're busy with linux.conf.au, but it would be > > really helpful if you could carve out five minutes to look over > > Alexey's proposal again, with my reply above in mind, and let us > > know whether it looks a reasonable design. Doesn't have to be a > > review, just a quick feedback on the high-level idea. > > It looks ok, I think, but I don't think I'm really the right person to > ask. I do wonder if creating a throwaway instance could cause > trouble, especially for something like machine that might well have > gotten away with having global side-effects in the past. I think we > need to talk with someone who knows more about qom and qapi - Markus > seems the obvious choice. Good point. CC'ing Markus to try and grab his attention :)
On Tue, Jan 23, 2018 at 01:03:39PM +0100, Andrea Bolognani wrote: > On Tue, 2018-01-23 at 22:20 +1100, David Gibson wrote: > > > David, I know you're busy with linux.conf.au, but it would be > > > really helpful if you could carve out five minutes to look over > > > Alexey's proposal again, with my reply above in mind, and let us > > > know whether it looks a reasonable design. Doesn't have to be a > > > review, just a quick feedback on the high-level idea. > > > > It looks ok, I think, but I don't think I'm really the right person to > > ask. I do wonder if creating a throwaway instance could cause > > trouble, especially for something like machine that might well have > > gotten away with having global side-effects in the past. I think we > > need to talk with someone who knows more about qom and qapi - Markus > > seems the obvious choice. > > Good point. CC'ing Markus to try and grab his attention :) It's also occurred to me that making a spapr specific approach to this might not be quite as horrible as I initially thought. The capabilities table is global (and immutable) so coding up a "get-spapr-caps" qapi entry point which encodes the stuff there into json giving the names and allowed values of each cap would be fairly straightforward. Accurately retreiving default values would be trickier, not sure if that's important or not.
On Tue, 2018-01-23 at 23:49 +1100, David Gibson wrote: > It's also occurred to me that making a spapr specific approach to this > might not be quite as horrible as I initially thought. The > capabilities table is global (and immutable) so coding up a > "get-spapr-caps" qapi entry point which encodes the stuff there into > json giving the names and allowed values of each cap would be fairly > straightforward. OTOH, qom-list-properties is a superset of device-list-properties so it could be used instead of it if supported; plus it would expose properties of machines which are not also capabilities and properties of non-pSeries machine types. There could be value in taking the more generic approach. > Accurately retreiving default values would be trickier, not sure if > that's important or not. Not sure. I think it's okay not to expose that information, since there are other areas where defaults are not exposed and so all libvirt can do is document that not *explicitly* setting a feature will result in the hypervisor default, whatever that might happen to be, being enforced.
On 23/01/18 23:49, David Gibson wrote: > On Tue, Jan 23, 2018 at 01:03:39PM +0100, Andrea Bolognani wrote: >> On Tue, 2018-01-23 at 22:20 +1100, David Gibson wrote: >>>> David, I know you're busy with linux.conf.au, but it would be >>>> really helpful if you could carve out five minutes to look over >>>> Alexey's proposal again, with my reply above in mind, and let us >>>> know whether it looks a reasonable design. Doesn't have to be a >>>> review, just a quick feedback on the high-level idea. >>> >>> It looks ok, I think, but I don't think I'm really the right person to >>> ask. I do wonder if creating a throwaway instance could cause >>> trouble, especially for something like machine that might well have >>> gotten away with having global side-effects in the past. I think we >>> need to talk with someone who knows more about qom and qapi - Markus >>> seems the obvious choice. >> >> Good point. CC'ing Markus to try and grab his attention :) > > It's also occurred to me that making a spapr specific approach to this > might not be quite as horrible as I initially thought. The > capabilities table is global (and immutable) so coding up a > "get-spapr-caps" qapi entry point which encodes the stuff there into > json giving the names and allowed values of each cap would be fairly > straightforward. > > Accurately retreiving default values would be trickier, not sure if > that's important or not. So, do we want to push it further? Markus has not reacted, added Paolo. http://patchwork.ozlabs.org/patch/863325/
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > There is already 'device-list-properties' which does most of the job, > however it does not handle everything returned by qom-list-types such > as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. > > This adds a new qom-list-properties command which prints properties > of a specific class and its instance. It is pretty much a simplified copy > of the device-list-properties handler. > > Since it creates an object instance, device properties should appear > in the output as they are copied to QOM properties at the instance_init > hook. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Related: qom-list, which lists "any properties of a object given a path in the object model." qom-list-properties takes a type name, which qom-list takes the path to an instance. In other words, qom-list-properties is like instantiate with default configuration and without realizing + qom-list + destroy. We need to instantiate because QOM properties are dynamic: they aren't specified by data (which qom-list-properties could simply read), they are created by (instantiation) code (which qom-list-properties has to run). Properties created only after instantiation (by realize, perhaps) aren't visible in qom-list-properties. Do such properties exist? Properties created only in non-default configuration aren't visible either. Such properties have to exist, or else dynamic property creation would be idiotic. Likewise for properties created differently (say with a different type) in non-default configuration. We can hope that no such beasts exist. Since properties get created by code, and code can do anything, we're reduced to hope. Data is so much easier to reason about than code. Three building blocks: instantiate, qom-list, destroy. Do we want the building blocks, or do we want their combination qom-list-properties? > --- > > I am missing the point of make_device_property_info(). > qmp_device_list_properties() creates the instance which copies everything > to QOM properties hashtable and commenting out the do{}while() in > make_device_property_info() does not seem to change a thing, what case > am I missing here? git-blame points to Stefan. Stefan, can you help? > --- > qapi-schema.json | 29 +++++++++++++++++++++++++++++ > qmp.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 5c06745..9d73501 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1455,6 +1455,35 @@ > 'returns': [ 'DevicePropertyInfo' ] } > > ## > +# @QOMPropertyInfo: > +# > +# Information about object properties. > +# > +# @name: the name of the property > +# @type: the typename of the property > +# @description: if specified, the description of the property. > +# > +# Since: 2.12 > +## > +{ 'struct': 'QOMPropertyInfo', > + 'data': { 'name': 'str', 'type': 'str', '*description': 'str' } } > + > +## > +# @qom-list-properties: > +# > +# List properties associated with a QOM object. > +# > +# @typename: the type name of an object > +# > +# Returns: a list of QOMPropertyInfo describing object properties > +# > +# Since: 2.12 > +## > +{ 'command': 'qom-list-properties', > + 'data': { 'typename': 'str'}, > + 'returns': [ 'QOMPropertyInfo' ] } > + > +## > # @xen-set-global-dirty-log: > # > # Enable or disable the global dirty log mode. > diff --git a/qmp.c b/qmp.c > index 52cfd2d..20cb662 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -574,6 +574,58 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, > return prop_list; > } > > +QOMPropertyInfoList *qmp_qom_list_properties(const char *typename, > + Error **errp) > +{ > + ObjectClass *klass; > + Object *obj; > + ObjectProperty *prop; > + ObjectPropertyIterator iter; > + QOMPropertyInfoList *prop_list = NULL; > + > + klass = object_class_by_name(typename); > + if (klass == NULL) { > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Class '%s' not found", typename); > + return NULL; > + } > + > + klass = object_class_dynamic_cast(klass, TYPE_OBJECT); > + if (klass == NULL) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", TYPE_OBJECT); > + return NULL; > + } > + > + if (object_class_is_abstract(klass)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", > + "non-abstract class"); > + return NULL; > + } > + > + obj = object_new(typename); > + > + object_property_iter_init(&iter, obj); > + while ((prop = object_property_iter_next(&iter))) { > + QOMPropertyInfo *info; > + QOMPropertyInfoList *entry; > + > + info = g_malloc0(sizeof(*info)); > + info->name = g_strdup(prop->name); > + info->type = g_strdup(prop->type); > + info->has_description = !!prop->description; > + info->description = g_strdup(prop->description); > + > + entry = g_malloc0(sizeof(*entry)); > + entry->value = info; > + entry->next = prop_list; > + prop_list = entry; > + } > + > + object_unref(obj); > + > + return prop_list; > +} > + > CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) > { > return arch_query_cpu_definitions(errp);
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 23/01/18 23:49, David Gibson wrote: >> On Tue, Jan 23, 2018 at 01:03:39PM +0100, Andrea Bolognani wrote: >>> On Tue, 2018-01-23 at 22:20 +1100, David Gibson wrote: >>>>> David, I know you're busy with linux.conf.au, but it would be >>>>> really helpful if you could carve out five minutes to look over >>>>> Alexey's proposal again, with my reply above in mind, and let us >>>>> know whether it looks a reasonable design. Doesn't have to be a >>>>> review, just a quick feedback on the high-level idea. >>>> >>>> It looks ok, I think, but I don't think I'm really the right person to >>>> ask. I do wonder if creating a throwaway instance could cause >>>> trouble, especially for something like machine that might well have >>>> gotten away with having global side-effects in the past. I think we >>>> need to talk with someone who knows more about qom and qapi - Markus >>>> seems the obvious choice. >>> >>> Good point. CC'ing Markus to try and grab his attention :) >> >> It's also occurred to me that making a spapr specific approach to this >> might not be quite as horrible as I initially thought. The >> capabilities table is global (and immutable) so coding up a >> "get-spapr-caps" qapi entry point which encodes the stuff there into >> json giving the names and allowed values of each cap would be fairly >> straightforward. >> >> Accurately retreiving default values would be trickier, not sure if >> that's important or not. > > > > So, do we want to push it further? Markus has not reacted, added Paolo. > > http://patchwork.ozlabs.org/patch/863325/ I now have. Sorry for the long delay.
On 01/02/18 04:22, Markus Armbruster wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> There is already 'device-list-properties' which does most of the job, >> however it does not handle everything returned by qom-list-types such >> as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. >> >> This adds a new qom-list-properties command which prints properties >> of a specific class and its instance. It is pretty much a simplified copy >> of the device-list-properties handler. >> >> Since it creates an object instance, device properties should appear >> in the output as they are copied to QOM properties at the instance_init >> hook. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Related: qom-list, which lists "any properties of a object given a path > in the object model." qom-list-properties takes a type name, which > qom-list takes the path to an instance. In other words, > qom-list-properties is like instantiate with default configuration and > without realizing + qom-list + destroy. True. Same as device-list-properties. > We need to instantiate because QOM properties are dynamic: they aren't > specified by data (which qom-list-properties could simply read), they > are created by (instantiation) code (which qom-list-properties has to > run). Correct. > Properties created only after instantiation (by realize, perhaps) aren't > visible in qom-list-properties. Do such properties exist? No idea but if they do, then this issue already exists in device-list-properties. > Properties created only in non-default configuration aren't visible > either. Such properties have to exist, or else dynamic property > creation would be idiotic. > > Likewise for properties created differently (say with a different type) > in non-default configuration. We can hope that no such beasts exist. > Since properties get created by code, and code can do anything, we're > reduced to hope. Data is so much easier to reason about than code. > > Three building blocks: instantiate, qom-list, destroy. Do we want the > building blocks, or do we want their combination qom-list-properties? Building blocks as QEMU internal helpers to split my qmp_qom_list_properties() into? These are not going to be huge and "destroy" is literally object_unref(obj) which does not seem very useful. Or I missed the point here? >> --- >> >> I am missing the point of make_device_property_info(). >> qmp_device_list_properties() creates the instance which copies everything >> to QOM properties hashtable and commenting out the do{}while() in >> make_device_property_info() does not seem to change a thing, what case >> am I missing here? > > git-blame points to Stefan. Stefan, can you help? > >> --- >> qapi-schema.json | 29 +++++++++++++++++++++++++++++ >> qmp.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 81 insertions(+) >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 5c06745..9d73501 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -1455,6 +1455,35 @@ >> 'returns': [ 'DevicePropertyInfo' ] } >> >> ## >> +# @QOMPropertyInfo: >> +# >> +# Information about object properties. >> +# >> +# @name: the name of the property >> +# @type: the typename of the property >> +# @description: if specified, the description of the property. >> +# >> +# Since: 2.12 >> +## >> +{ 'struct': 'QOMPropertyInfo', >> + 'data': { 'name': 'str', 'type': 'str', '*description': 'str' } } >> + >> +## >> +# @qom-list-properties: >> +# >> +# List properties associated with a QOM object. >> +# >> +# @typename: the type name of an object >> +# >> +# Returns: a list of QOMPropertyInfo describing object properties >> +# >> +# Since: 2.12 >> +## >> +{ 'command': 'qom-list-properties', >> + 'data': { 'typename': 'str'}, >> + 'returns': [ 'QOMPropertyInfo' ] } >> + >> +## >> # @xen-set-global-dirty-log: >> # >> # Enable or disable the global dirty log mode. >> diff --git a/qmp.c b/qmp.c >> index 52cfd2d..20cb662 100644 >> --- a/qmp.c >> +++ b/qmp.c >> @@ -574,6 +574,58 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, >> return prop_list; >> } >> >> +QOMPropertyInfoList *qmp_qom_list_properties(const char *typename, >> + Error **errp) >> +{ >> + ObjectClass *klass; >> + Object *obj; >> + ObjectProperty *prop; >> + ObjectPropertyIterator iter; >> + QOMPropertyInfoList *prop_list = NULL; >> + >> + klass = object_class_by_name(typename); >> + if (klass == NULL) { >> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> + "Class '%s' not found", typename); >> + return NULL; >> + } >> + >> + klass = object_class_dynamic_cast(klass, TYPE_OBJECT); >> + if (klass == NULL) { >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", TYPE_OBJECT); >> + return NULL; >> + } >> + >> + if (object_class_is_abstract(klass)) { >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", >> + "non-abstract class"); >> + return NULL; >> + } >> + >> + obj = object_new(typename); >> + >> + object_property_iter_init(&iter, obj); >> + while ((prop = object_property_iter_next(&iter))) { >> + QOMPropertyInfo *info; >> + QOMPropertyInfoList *entry; >> + >> + info = g_malloc0(sizeof(*info)); >> + info->name = g_strdup(prop->name); >> + info->type = g_strdup(prop->type); >> + info->has_description = !!prop->description; >> + info->description = g_strdup(prop->description); >> + >> + entry = g_malloc0(sizeof(*entry)); >> + entry->value = info; >> + entry->next = prop_list; >> + prop_list = entry; >> + } >> + >> + object_unref(obj); >> + >> + return prop_list; >> +} >> + >> CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) >> { >> return arch_query_cpu_definitions(errp);
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 01/02/18 04:22, Markus Armbruster wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> There is already 'device-list-properties' which does most of the job, >>> however it does not handle everything returned by qom-list-types such >>> as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. >>> >>> This adds a new qom-list-properties command which prints properties >>> of a specific class and its instance. It is pretty much a simplified copy >>> of the device-list-properties handler. >>> >>> Since it creates an object instance, device properties should appear >>> in the output as they are copied to QOM properties at the instance_init >>> hook. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> Related: qom-list, which lists "any properties of a object given a path >> in the object model." qom-list-properties takes a type name, which >> qom-list takes the path to an instance. In other words, >> qom-list-properties is like instantiate with default configuration and >> without realizing + qom-list + destroy. > > > True. Same as device-list-properties. device-list-properties does a bit more, like skipping "uninteresting" properties, and special magic for qdev properties (that's the make_device_property_info() you asked about below). But that's detail. >> We need to instantiate because QOM properties are dynamic: they aren't >> specified by data (which qom-list-properties could simply read), they >> are created by (instantiation) code (which qom-list-properties has to >> run). > > Correct. > >> Properties created only after instantiation (by realize, perhaps) aren't >> visible in qom-list-properties. Do such properties exist? > > No idea but if they do, then this issue already exists in > device-list-properties. > >> Properties created only in non-default configuration aren't visible >> either. Such properties have to exist, or else dynamic property >> creation would be idiotic. Thus, qom-list-properties design limitation: the result need not reflect properties of instantiated objects. It usually does, as most QOM properties behave as if they were static. But when it doesn't, what then? How are users of qom-list-properties supposed to deal with such inaccurate / incorrect information? Do they just have to know which properties aren't visible in qom-list-properties, and which properties are, but cannot be trusted? I posit that right now *nobody* knows. Would such a command be useful anyway? >> Likewise for properties created differently (say with a different type) >> in non-default configuration. We can hope that no such beasts exist. >> Since properties get created by code, and code can do anything, we're >> reduced to hope. Data is so much easier to reason about than code. >> >> Three building blocks: instantiate, qom-list, destroy. Do we want the >> building blocks, or do we want their combination qom-list-properties? > > > Building blocks as QEMU internal helpers to split my > qmp_qom_list_properties() into? These are not going to be huge and > "destroy" is literally object_unref(obj) which does not seem very useful. > Or I missed the point here? My question is whether the QMP interface should provide the building blocks, or only compositions. >>> --- >>> >>> I am missing the point of make_device_property_info(). >>> qmp_device_list_properties() creates the instance which copies everything >>> to QOM properties hashtable and commenting out the do{}while() in >>> make_device_property_info() does not seem to change a thing, what case >>> am I missing here? >> >> git-blame points to Stefan. Stefan, can you help?
On 02/02/18 18:37, Markus Armbruster wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 01/02/18 04:22, Markus Armbruster wrote: >>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>> >>>> There is already 'device-list-properties' which does most of the job, >>>> however it does not handle everything returned by qom-list-types such >>>> as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. >>>> >>>> This adds a new qom-list-properties command which prints properties >>>> of a specific class and its instance. It is pretty much a simplified copy >>>> of the device-list-properties handler. >>>> >>>> Since it creates an object instance, device properties should appear >>>> in the output as they are copied to QOM properties at the instance_init >>>> hook. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> >>> Related: qom-list, which lists "any properties of a object given a path >>> in the object model." qom-list-properties takes a type name, which >>> qom-list takes the path to an instance. In other words, >>> qom-list-properties is like instantiate with default configuration and >>> without realizing + qom-list + destroy. >> >> >> True. Same as device-list-properties. > > device-list-properties does a bit more, like skipping "uninteresting" > properties, and special magic for qdev properties (that's the > make_device_property_info() you asked about below). But that's detail. It is not clear at all why would we want to skip some properties (yes, question to Stefan, not you) but this does not explain make_device_property_info() - this thing walks through the device properties even though they are already copied to the Object properties and we could enumerate those, all of them, I compared device-list-properties and qom-list-properties: -*** Sending {"execute": "device-list-properties", "arguments": {"typename": "vfio-pci"}} +*** Sending {"arguments": {"typename": "vfio-pci"}, "execute": "qom-list-properties"} >>>>>>>>>>>>>>>> { 'return': [ { 'description': 'on/off', 'name': 'x-igd-opregion', 'type': 'bool'}, + {'name': 'legacy-addr', 'type': 'str'}, {'name': 'x-pci-vendor-id', 'type': 'uint32'}, + {'name': 'type', 'type': 'string'}, {'name': 'x-pci-sub-device-id', 'type': 'uint32'}, {'name': 'rombar', 'type': 'uint32'}, { 'description': 'on/off', @@ -19,10 +21,13 @@ 'host device, example: 04:10.0', 'name': 'host', 'type': 'str'}, + {'name': 'realized', 'type': 'bool'}, {'name': 'x-no-kvm-msix', 'type': 'bool'}, { 'description': 'on/off', 'name': 'command_serr_enable', 'type': 'bool'}, + {'name': 'parent_bus', 'type': 'link<bus>'}, + {'name': 'hotplugged', 'type': 'bool'}, {'name': 'x-pci-sub-vendor-id', 'type': 'uint32'}, {'name': 'x-pci-device-id', 'type': 'uint32'}, { 'description': 'on/off', @@ -45,6 +50,7 @@ 'type': 'bool'}, {'name': 'x-no-mmap', 'type': 'bool'}, {'name': 'bootindex', 'type': 'int32'}, + {'name': 'hotpluggable', 'type': 'bool'}, {'name': 'sysfsdev', 'type': 'str'}, {'description': 'on/off', 'name': 'x-vga', 'type': 'bool'}, {'name': 'romfile', 'type': 'str'}]} The difference is "uninteresting" props and one suspicious "legacy-addr" (which I have no idea what is that but it is offtopic here). > >>> We need to instantiate because QOM properties are dynamic: they aren't >>> specified by data (which qom-list-properties could simply read), they >>> are created by (instantiation) code (which qom-list-properties has to >>> run). >> >> Correct. >> >>> Properties created only after instantiation (by realize, perhaps) aren't >>> visible in qom-list-properties. Do such properties exist? >> >> No idea but if they do, then this issue already exists in >> device-list-properties. >> >>> Properties created only in non-default configuration aren't visible >>> either. Such properties have to exist, or else dynamic property >>> creation would be idiotic. > > Thus, qom-list-properties design limitation: the result need not reflect > properties of instantiated objects. It usually does, as most QOM > properties behave as if they were static. But when it doesn't, what > then? How are users of qom-list-properties supposed to deal with such > inaccurate / incorrect information? Do they just have to know which > properties aren't visible in qom-list-properties, and which properties > are, but cannot be trusted? > > I posit that right now *nobody* knows. > > Would such a command be useful anyway? qom-list-properties? yes, we want it to let libvirt a way to know if a machine supports certain properties, if a machine was derived from a device, then we would not need it. > >>> Likewise for properties created differently (say with a different type) >>> in non-default configuration. We can hope that no such beasts exist. >>> Since properties get created by code, and code can do anything, we're >>> reduced to hope. Data is so much easier to reason about than code. >>> >>> Three building blocks: instantiate, qom-list, destroy. Do we want the >>> building blocks, or do we want their combination qom-list-properties? >> >> >> Building blocks as QEMU internal helpers to split my >> qmp_qom_list_properties() into? These are not going to be huge and >> "destroy" is literally object_unref(obj) which does not seem very useful. >> Or I missed the point here? > > My question is whether the QMP interface should provide the building > blocks, or only compositions. instantiate but not realize? Not sure we have users for that. > >>>> --- >>>> >>>> I am missing the point of make_device_property_info(). >>>> qmp_device_list_properties() creates the instance which copies everything >>>> to QOM properties hashtable and commenting out the do{}while() in >>>> make_device_property_info() does not seem to change a thing, what case >>>> am I missing here? >>> >>> git-blame points to Stefan. Stefan, can you help?
On Mon, 2018-02-05 at 14:30 +1100, Alexey Kardashevskiy wrote: > On 02/02/18 18:37, Markus Armbruster wrote: > > > > Likewise for properties created differently (say with a different type) > > > > in non-default configuration. We can hope that no such beasts exist. > > > > Since properties get created by code, and code can do anything, we're > > > > reduced to hope. Data is so much easier to reason about than code. > > > > > > > > Three building blocks: instantiate, qom-list, destroy. Do we want the > > > > building blocks, or do we want their combination qom-list-properties? > > > > > > Building blocks as QEMU internal helpers to split my > > > qmp_qom_list_properties() into? These are not going to be huge and > > > "destroy" is literally object_unref(obj) which does not seem very useful. > > > Or I missed the point here? > > > > My question is whether the QMP interface should provide the building > > blocks, or only compositions. > > instantiate but not realize? Not sure we have users for that. We already have scaffolding for dealing with device-list-properties in libvirt, so the implementation of qom-list-properties proposed by Alexey would probably be able to reuse a lot of code and could be integrated very quickly. So there's that. Would there be any other known use case for providing the building blocks?
On 19/01/18 16:09, Alexey Kardashevskiy wrote: > There is already 'device-list-properties' which does most of the job, > however it does not handle everything returned by qom-list-types such > as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. > > This adds a new qom-list-properties command which prints properties > of a specific class and its instance. It is pretty much a simplified copy > of the device-list-properties handler. > > Since it creates an object instance, device properties should appear > in the output as they are copied to QOM properties at the instance_init > hook. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> So is it ack or nack for the patch? Whose area is this? Thanks, > --- > > I am missing the point of make_device_property_info(). > qmp_device_list_properties() creates the instance which copies everything > to QOM properties hashtable and commenting out the do{}while() in > make_device_property_info() does not seem to change a thing, what case > am I missing here? > > > --- > qapi-schema.json | 29 +++++++++++++++++++++++++++++ > qmp.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 5c06745..9d73501 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1455,6 +1455,35 @@ > 'returns': [ 'DevicePropertyInfo' ] } > > ## > +# @QOMPropertyInfo: > +# > +# Information about object properties. > +# > +# @name: the name of the property > +# @type: the typename of the property > +# @description: if specified, the description of the property. > +# > +# Since: 2.12 > +## > +{ 'struct': 'QOMPropertyInfo', > + 'data': { 'name': 'str', 'type': 'str', '*description': 'str' } } > + > +## > +# @qom-list-properties: > +# > +# List properties associated with a QOM object. > +# > +# @typename: the type name of an object > +# > +# Returns: a list of QOMPropertyInfo describing object properties > +# > +# Since: 2.12 > +## > +{ 'command': 'qom-list-properties', > + 'data': { 'typename': 'str'}, > + 'returns': [ 'QOMPropertyInfo' ] } > + > +## > # @xen-set-global-dirty-log: > # > # Enable or disable the global dirty log mode. > diff --git a/qmp.c b/qmp.c > index 52cfd2d..20cb662 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -574,6 +574,58 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, > return prop_list; > } > > +QOMPropertyInfoList *qmp_qom_list_properties(const char *typename, > + Error **errp) > +{ > + ObjectClass *klass; > + Object *obj; > + ObjectProperty *prop; > + ObjectPropertyIterator iter; > + QOMPropertyInfoList *prop_list = NULL; > + > + klass = object_class_by_name(typename); > + if (klass == NULL) { > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Class '%s' not found", typename); > + return NULL; > + } > + > + klass = object_class_dynamic_cast(klass, TYPE_OBJECT); > + if (klass == NULL) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", TYPE_OBJECT); > + return NULL; > + } > + > + if (object_class_is_abstract(klass)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", > + "non-abstract class"); > + return NULL; > + } > + > + obj = object_new(typename); > + > + object_property_iter_init(&iter, obj); > + while ((prop = object_property_iter_next(&iter))) { > + QOMPropertyInfo *info; > + QOMPropertyInfoList *entry; > + > + info = g_malloc0(sizeof(*info)); > + info->name = g_strdup(prop->name); > + info->type = g_strdup(prop->type); > + info->has_description = !!prop->description; > + info->description = g_strdup(prop->description); > + > + entry = g_malloc0(sizeof(*entry)); > + entry->value = info; > + entry->next = prop_list; > + prop_list = entry; > + } > + > + object_unref(obj); > + > + return prop_list; > +} > + > CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) > { > return arch_query_cpu_definitions(errp); >
On 21/02/2018 04:36, Alexey Kardashevskiy wrote: > On 19/01/18 16:09, Alexey Kardashevskiy wrote: >> There is already 'device-list-properties' which does most of the job, >> however it does not handle everything returned by qom-list-types such >> as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. >> >> This adds a new qom-list-properties command which prints properties >> of a specific class and its instance. It is pretty much a simplified copy >> of the device-list-properties handler. >> >> Since it creates an object instance, device properties should appear >> in the output as they are copied to QOM properties at the instance_init >> hook. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > So is it ack or nack for the patch? Whose area is this? Thanks, I think Markus, but I can queue it too because he's on leave. Can you please resubmit it? Paolo
diff --git a/qapi-schema.json b/qapi-schema.json index 5c06745..9d73501 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1455,6 +1455,35 @@ 'returns': [ 'DevicePropertyInfo' ] } ## +# @QOMPropertyInfo: +# +# Information about object properties. +# +# @name: the name of the property +# @type: the typename of the property +# @description: if specified, the description of the property. +# +# Since: 2.12 +## +{ 'struct': 'QOMPropertyInfo', + 'data': { 'name': 'str', 'type': 'str', '*description': 'str' } } + +## +# @qom-list-properties: +# +# List properties associated with a QOM object. +# +# @typename: the type name of an object +# +# Returns: a list of QOMPropertyInfo describing object properties +# +# Since: 2.12 +## +{ 'command': 'qom-list-properties', + 'data': { 'typename': 'str'}, + 'returns': [ 'QOMPropertyInfo' ] } + +## # @xen-set-global-dirty-log: # # Enable or disable the global dirty log mode. diff --git a/qmp.c b/qmp.c index 52cfd2d..20cb662 100644 --- a/qmp.c +++ b/qmp.c @@ -574,6 +574,58 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, return prop_list; } +QOMPropertyInfoList *qmp_qom_list_properties(const char *typename, + Error **errp) +{ + ObjectClass *klass; + Object *obj; + ObjectProperty *prop; + ObjectPropertyIterator iter; + QOMPropertyInfoList *prop_list = NULL; + + klass = object_class_by_name(typename); + if (klass == NULL) { + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Class '%s' not found", typename); + return NULL; + } + + klass = object_class_dynamic_cast(klass, TYPE_OBJECT); + if (klass == NULL) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", TYPE_OBJECT); + return NULL; + } + + if (object_class_is_abstract(klass)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", + "non-abstract class"); + return NULL; + } + + obj = object_new(typename); + + object_property_iter_init(&iter, obj); + while ((prop = object_property_iter_next(&iter))) { + QOMPropertyInfo *info; + QOMPropertyInfoList *entry; + + info = g_malloc0(sizeof(*info)); + info->name = g_strdup(prop->name); + info->type = g_strdup(prop->type); + info->has_description = !!prop->description; + info->description = g_strdup(prop->description); + + entry = g_malloc0(sizeof(*entry)); + entry->value = info; + entry->next = prop_list; + prop_list = entry; + } + + object_unref(obj); + + return prop_list; +} + CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) { return arch_query_cpu_definitions(errp);
There is already 'device-list-properties' which does most of the job, however it does not handle everything returned by qom-list-types such as machines as they inherit directly from TYPE_OBJECT and not TYPE_DEVICE. This adds a new qom-list-properties command which prints properties of a specific class and its instance. It is pretty much a simplified copy of the device-list-properties handler. Since it creates an object instance, device properties should appear in the output as they are copied to QOM properties at the instance_init hook. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- I am missing the point of make_device_property_info(). qmp_device_list_properties() creates the instance which copies everything to QOM properties hashtable and commenting out the do{}while() in make_device_property_info() does not seem to change a thing, what case am I missing here? --- qapi-schema.json | 29 +++++++++++++++++++++++++++++ qmp.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+)