Re: [Qemu-devel] [PATCH v2 13/27] qdev: remove direct calls to print/parse

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/04/2012 02:02 AM, Paolo Bonzini wrote:
> There's no need to call into ->parse and ->print manually.  The
> QOM legacy properties do that for us.
>
> Furthermore, in some cases legacy and static properties have exactly
> the same behavior, and we could drop the legacy properties right away.
> Add an appropriate fallback to prepare for this.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Nice cleanup.  Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

I wonder how far we are from moving most of qdev-monitor.c to hmp.c and qmp.c 
respectively.  I think that given this patch, we can actually implement info 
qtree/qdm via qom QMP operations.

That would be a fairly nice win.

Regards,

Anthony Liguori

> ---
>   hw/qdev-monitor.c    |   30 +++++++++++++++++-------------
>   hw/qdev-properties.c |   26 ++++++++++----------------
>   hw/qdev.c            |    9 +++++++++
>   3 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 135c2bf..49f13ca 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -485,22 +485,26 @@ static void qbus_print(Monitor *mon, BusState *bus, int indent);
>   static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>                                const char *prefix, int indent)
>   {
> -    char buf[64];
> -
>       if (!props)
>           return;
> -    while (props->name) {
> -        /*
> -         * TODO Properties without a print method are just for dirty
> -         * hacks.  qdev_prop_ptr is the only such PropertyInfo.  It's
> -         * marked for removal.  The test props->info->print should be
> -         * removed along with it.
> -         */
> -        if (props->info->print) {
> -            props->info->print(dev, props, buf, sizeof(buf));
> -            qdev_printf("%s-prop: %s = %s\n", prefix, props->name, buf);
> +    for (; props->name; props++) {
> +        Error *err = NULL;
> +        char *value;
> +        char *legacy_name = g_strdup_printf("legacy-%s", props->name);
> +        if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> +            value = object_property_get_str(OBJECT(dev), legacy_name,&err);
> +        } else {
> +            value = object_property_get_str(OBJECT(dev), props->name,&err);
> +        }
> +        g_free(legacy_name);
> +
> +        if (err) {
> +            error_free(err);
> +            continue;
>           }
> -        props++;
> +        qdev_printf("%s-prop: %s = %s\n", prefix, props->name,
> +                    value&&  *value ? value : "<null>");
> +        g_free(value);
>       }
>   }
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index c4583a1..5e19ec8 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -1073,24 +1073,18 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>
>   int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
>   {
> -    Property *prop;
> -    int ret;
> +    char *legacy_name;
> +    Error *err = NULL;
>
> -    prop = qdev_prop_find(dev, name);
> -    /*
> -     * TODO Properties without a parse method are just for dirty
> -     * hacks.  qdev_prop_ptr is the only such PropertyInfo.  It's
> -     * marked for removal.  The test !prop->info->parse should be
> -     * removed along with it.
> -     */
> -    if (!prop || !prop->info->parse) {
> -        qerror_report(QERR_PROPERTY_NOT_FOUND, object_get_typename(OBJECT(dev)), name);
> -        return -1;
> +    legacy_name = g_strdup_printf("legacy-%s", name);
> +    if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> +        object_property_set_str(OBJECT(dev), value, legacy_name,&err);
> +    } else {
> +        object_property_set_str(OBJECT(dev), value, name,&err);
>       }
> -    ret = prop->info->parse(dev, prop, value);
> -    if (ret<  0) {
> -        Error *err;
> -        error_set_from_qdev_prop_error(&err, ret, dev, prop, value);
> +    g_free(legacy_name);
> +
> +    if (err) {
>           qerror_report_err(err);
>           error_free(err);
>           return -1;
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e3b53b7..a731e41 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -581,6 +581,15 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>   void qdev_property_add_static(DeviceState *dev, Property *prop,
>                                 Error **errp)
>   {
> +    /*
> +     * TODO qdev_prop_ptr does not have getters or setters.  It must
> +     * go now that it can be replaced with links.  The test should be
> +     * removed along with it: all static properties are read/write.
> +     */
> +    if (!prop->info->get&&  !prop->info->set) {
> +        return;
> +    }
> +
>       object_property_add(OBJECT(dev), prop->name, prop->info->name,
>                           prop->info->get, prop->info->set,
>                           NULL,