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

[Eric Blake <eblake@redhat.com>]

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

On 01/20/2016 02:42 AM, Valentin Rakush wrote:
> This patch adds support for qom-type-list command that supposed to list object class properties. Will use this functionality further to implement x86_64 cpu properties.

In addition to Dan's comments,

s/that supposed//
s/Will use/A later patch will use/

> 
> Additionally object_class_property_iterator_init function is implemented for consistency.

Starting a paragraph with "Also" or "Additionally" can be a sign that
the patch may be worth splitting into two smaller pieces.  It's not
always the case, but worth thinking about.

> 
> 
> Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hmp.c                | 26 ++++++++++++++++++++++++++
>  hmp.h                |  1 +
>  include/qom/object.h | 31 +++++++++++++++++++++++++++++++
>  qapi-schema.json     | 19 +++++++++++++++++++
>  qmp-commands.hx      |  6 ++++++
>  qmp.c                | 29 +++++++++++++++++++++++++++++
>  qom/object.c         |  8 ++++++++
>  7 files changed, 120 insertions(+)
> 
> diff --git a/hmp.c b/hmp.c
> index 54f2620..64aa991 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2052,6 +2052,32 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_qom_type_list(Monitor *mon, const QDict *qdict)
> +{
> +    const char *path = qdict_get_try_str(qdict, "path");
> +    ObjectPropertyInfoList *list;
> +    Error *err = NULL;
> +
> +    if (path == NULL) {

I'd write '(!path)', but that's cosmetic.

> +        monitor_printf(mon, "/\n");
> +        return;
> +    }
> +
> +    list = qmp_qom_type_list(path, &err);
> +    if (err == NULL) {
> +        ObjectPropertyInfoList *start = list;
> +        while (list != NULL) {

Again, this could be 'if (!err)' and 'while (list)'.

> +++ b/include/qom/object.h
> @@ -1013,6 +1013,37 @@ void object_property_iter_init(ObjectPropertyIterator *iter,
>   */
>  ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter);
>  
> +/**
> + * object_class_property_iter_init:
> + * @klass: the klass to iter properties for

Reads awkwardly; maybe:

@klass: the class owning the properties to be iterated over

> +++ b/qapi-schema.json
> @@ -4081,3 +4081,22 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @qom-type-list:
> +#
> +# This command will list any properties of a object class

s/a object/an object/

> +# given a path in the object class model.

If you change the parameter name to 'typename' as suggested by Dan, then
this sentence needs tweaking as well.

> +#
> +# @path: the path within the object class model. See @qom-list-types
> +#        to check available object class names.
> +#


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]