Re: [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties

[Anthony Liguori]

On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<address@hidden>
> ---
>   hw/qdev-properties.c |   39 ++++++++++++++++++++++++---------------
>   hw/qdev.c            |    3 +--
>   hw/qdev.h            |    2 ++
>   3 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index f0b811c..76ecb38 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -614,6 +614,26 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
>       return qdev_prop_find(dev, name) ? true : false;
>   }
>
> +void qdev_prop_error(Error **errp, int ret,
> +                     DeviceState *dev, Property *prop, const char *value)
> +{
> +    switch (ret) {
> +    case -EEXIST:
> +        error_set(errp, QERR_PROPERTY_VALUE_IN_USE,
> +                  dev->info->name, prop->name, value);
> +        break;
> +    default:
> +    case -EINVAL:
> +        error_set(errp, QERR_PROPERTY_VALUE_BAD,
> +                  dev->info->name, prop->name, value);
> +        break;
> +    case -ENOENT:
> +        error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
> +                  dev->info->name, prop->name, value);
> +        break;
> +    }
> +}
> +
>   int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
>   {
>       Property *prop;
> @@ -632,21 +652,10 @@ int qdev_prop_parse(DeviceState *dev, const char *name, 
> const char *value)
>       }
>       ret = prop->info->parse(dev, prop, value);
>       if (ret<  0) {
> -        switch (ret) {
> -        case -EEXIST:
> -            qerror_report(QERR_PROPERTY_VALUE_IN_USE,
> -                          dev->info->name, name, value);
> -            break;
> -        default:
> -        case -EINVAL:
> -            qerror_report(QERR_PROPERTY_VALUE_BAD,
> -                          dev->info->name, name, value);
> -            break;
> -        case -ENOENT:
> -            qerror_report(QERR_PROPERTY_VALUE_NOT_FOUND,
> -                          dev->info->name, name, value);
> -            break;
> -        }
> +        Error *err;
> +        qdev_prop_error(&err, ret, dev, prop, value);
> +        qerror_report_err(err);
> +        error_free(err);
>           return -1;
>       }
>       return 0;
> diff --git a/hw/qdev.c b/hw/qdev.c
> index c020a6f..c8ab7b7 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -1163,8 +1163,7 @@ static void qdev_set_legacy_property(DeviceState *dev, 
> Visitor *v, void *opaque,
>
>       ret = prop->info->parse(dev, prop, ptr);
>       if (ret != 0) {
> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> -                  name, prop->info->name);
> +        qdev_prop_error(errp, ret, dev, prop, ptr);
>       }
>       g_free(ptr);
>   }
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 6e18427..828d811 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -370,6 +370,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property 
> *props);
>
>   void qdev_prop_register_global_list(GlobalProperty *props);
>   void qdev_prop_set_globals(DeviceState *dev);
> +void qdev_prop_error(Error **errp, int ret, DeviceState *name,
> +                     Property *prop, const char *value);

s/name/dev

And perhaps it would make more sense to return Error * and make the function 
name be a constructor:

Error *error_from_qdev_prop_err(int ret, DeviceState *dev,
                                Property *prop, const char *value);

I was fairly confused about what was going on here at first.

Reards,

Anthony Liguori

>   static inline const char *qdev_fw_name(DeviceState *dev)
>   {