| Message ID | 1411794841-9672-7-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
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.
> > 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. 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
diff --git a/qapi-schema.json b/qapi-schema.json index 4bfaf20..04b7e62 100644 --- a/qapi-schema.json +++ 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 # # Since: 1.2 ## { 'type': 'DevicePropertyInfo', - 'data': { 'name': 'str', 'type': 'str' } } + 'data': { 'name': 'str', 'type': 'str', 'description': 'str' } } ## # @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) { + error_printf(" (%s)\n", prop->value->description); + } else { + error_printf("\n"); + } } qapi_free_DevicePropertyInfoList(prop_list); diff --git a/qmp.c b/qmp.c index c6767c4..7e17d5a 100644 --- a/qmp.c +++ b/qmp.c @@ -442,7 +442,8 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements, */ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass, const char *name, - const char *default_type) + const char *default_type, + const char *description) { DevicePropertyInfo *info; Property *prop; @@ -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; 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; + return info; } @@ -521,7 +526,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, continue; } - info = make_device_property_info(klass, prop->name, prop->type); + info = make_device_property_info(klass, prop->name, prop->type, + prop->description); if (!info) { continue; }