qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Gonglei (Arei)
Subject: Re: [Qemu-devel] [PATCH v3 6/7] qmp: print descriptions of object properties
Date: Tue, 30 Sep 2014 01:49:27 +0000

> 
> On 09/26/2014 11:14 PM, address@hidden wrote:
> > From: Gonglei <address@hidden>
> >
> > 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. 

OK.

> 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.
> 

Yes.

> >  #
> >  # Since: 1.2
> >  ##
> >  { 'type': 'DevicePropertyInfo',
> > -  'data': { 'name': 'str', 'type': 'str' } }
> > +  'data': { 'name': 'str', 'type': 'str', 'description': 'str' } }
> 
> Therefore, this needs to be '*description'...
> 

OK.

> >
> >  ##
> >  # @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.
>
OK.
 
> > @@ -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).
> 
OK.

> >              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.
>
OK. Thanks a lot, Eric!

Will fix them in v4.

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]