Re: [Qemu-devel] [PATCH v3 6/7] qmp: print descriptions of object properties

[Eric Blake <eblake@redhat.com>]

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

On 09/26/2014 11:14 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Add a new "description" field to DevicePropertyInfo.
> The descriptions can serve as documentation in the code,
> and they can be used to provide better help. For example:
> 
> $./qemu-system-x86_64 -device virtio-blk-pci,?
> 

> +++ b/qapi-schema.json
> @@ -1615,11 +1615,12 @@
>  #
>  # @name: the name of the property
>  # @type: the typename of the property
> +# @description: the description of the property

Please add a '(since 2.2)' designation.  Will 'description' always be
present (that is, do ALL properties have a description), or is it
optional and only provided when a non-NULL description is available?

[/me reads rest of patch]

Yes, given your code, it looks like you meant for it to be optional.

>  #
>  # Since: 1.2
>  ##
>  { 'type': 'DevicePropertyInfo',
> -  'data': { 'name': 'str', 'type': 'str' } }
> +  'data': { 'name': 'str', 'type': 'str', 'description': 'str' } }

Therefore, this needs to be '*description'...

>  
>  ##
>  # @device-list-properties:
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5ec6606..5ee6bf2 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -213,9 +213,14 @@ int qdev_device_help(QemuOpts *opts)
>      }
>  
>      for (prop = prop_list; prop; prop = prop->next) {
> -        error_printf("%s.%s=%s\n", driver,
> +        error_printf("%s.%s=%s", driver,
>                       prop->value->name,
>                       prop->value->type);
> +        if (prop->value->description) {

...and this needs to be 'prop->value->has_description'.  We don't
support NULL values of non-optional strings in QMP yet.

> @@ -465,7 +466,9 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
>  
>              info = g_malloc0(sizeof(*info));
>              info->name = g_strdup(prop->name);
> -            info->type = g_strdup(prop->info->legacy_name ?: prop->info->name);
> +            info->type = g_strdup(prop->info->name);
> +            info->description = prop->info->description ?
> +                                g_strdup(prop->info->description) : NULL;

This needs to be:

info->has_description = !!prop->info->description;
info->description = g_strdup(prop->info->description);

(it's safe to call g_strdup(NULL), avoiding the need for ?: in the
assignment).

>              return info;
>          }
>          klass = object_class_get_parent(klass);
> @@ -475,6 +478,8 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
>      info = g_malloc0(sizeof(*info));
>      info->name = g_strdup(name);
>      info->type = g_strdup(default_type);
> +    info->description = description ? g_strdup(description) : NULL;

Another place that should avoid the ?:, and set has_description.

-- 
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: 539 bytes --]