Re: [Qemu-devel] [PATCH 3/7] qdev_prop_parse(): report errors via Error

[Luiz Capitulino]

On Fri,  1 Feb 2013 18:38:15 +0100
Laszlo Ersek <address@hidden> wrote:

> 
> Signed-off-by: Laszlo Ersek <address@hidden>

I usually split this kind of patch the following way:

 1. add an Error ** argument to the function reporting the error. Callers
    are changed to pass NULL for the new argument

 2. Handling of the new error is added for each caller in a different
    patch (it's OK to group callers when the error handling is the same)

 3. Drop error code return value from the function taking an Error **
    argument

More comments below.

> ---
>  hw/qdev-properties.h |    4 +++-
>  hw/qdev-monitor.c    |   26 +++++++++++++++++++++-----
>  hw/qdev-properties.c |   12 ++++++++----
>  3 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
> index ddcf774..e33f31b 100644
> --- a/hw/qdev-properties.h
> +++ b/hw/qdev-properties.h
> @@ -2,6 +2,7 @@
>  #define QEMU_QDEV_PROPERTIES_H
>  
>  #include "qdev-core.h"
> +#include "qapi/error.h"
>  
>  /*** qdev-properties.c ***/
>  
> @@ -99,7 +100,8 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
>  
>  /* Set properties between creation and init.  */
>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
> -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
> +int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> +                    Error **errp);
>  void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
>  void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value);
>  void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t 
> value);
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 32be5a2..691bab5 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -100,16 +100,23 @@ static void qdev_print_devinfo(ObjectClass *klass, void 
> *opaque)
>      error_printf("\n");
>  }
>  
> +typedef struct {
> +    DeviceState *dev;
> +    Error **errp;
> +} PropertyContext;
> +
>  static int set_property(const char *name, const char *value, void *opaque)
>  {
> -    DeviceState *dev = opaque;
> +    PropertyContext *ctx = opaque;
> +    Error *err = NULL;
>  
>      if (strcmp(name, "driver") == 0)
>          return 0;
>      if (strcmp(name, "bus") == 0)
>          return 0;
>  
> -    if (qdev_prop_parse(dev, name, value) == -1) {
> +    if (qdev_prop_parse(ctx->dev, name, value, &err) == -1) {
> +        error_propagate(ctx->errp, err);
>          return -1;

The return error can be dropped if it's only used to signal success/failure.

>      }
>      return 0;
> @@ -415,6 +422,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      const char *driver, *path, *id;
>      DeviceState *qdev;
>      BusState *bus;
> +    Error *local_err = NULL;
>  
>      driver = qemu_opt_get(opts, "driver");
>      if (!driver) {
> @@ -477,10 +485,18 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      if (id) {
>          qdev->id = id;
>      }
> -    if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
> -        qdev_free(qdev);
> -        return NULL;
> +
> +    {
> +        PropertyContext ctx = { .dev = qdev, .errp = &local_err };
> +
> +        if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) {
> +            qerror_report_err(local_err);
> +            error_free(local_err);
> +            qdev_free(qdev);
> +            return NULL;
> +        }
>      }
> +
>      if (qdev->id) {
>          object_property_add_child(qdev_get_peripheral(), qdev->id,
>                                    OBJECT(qdev), NULL);
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index a8a31f5..8e3d014 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -835,7 +835,8 @@ 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)
> +int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> +                    Error **errp)
>  {
>      char *legacy_name;
>      Error *err = NULL;
> @@ -849,8 +850,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, 
> const char *value)
>      g_free(legacy_name);
>  
>      if (err) {
> -        qerror_report_err(err);
> -        error_free(err);
> +        error_propagate(errp, err);
>          return -1;
>      }

Same here.

>      return 0;
> @@ -962,10 +962,14 @@ void qdev_prop_set_globals(DeviceState *dev)
>      do {
>          GlobalProperty *prop;
>          QTAILQ_FOREACH(prop, &global_props, next) {
> +            Error *err = NULL;
> +
>              if (strcmp(object_class_get_name(class), prop->driver) != 0) {
>                  continue;
>              }
> -            if (qdev_prop_parse(dev, prop->property, prop->value) != 0) {
> +            if (qdev_prop_parse(dev, prop->property, prop->value, &err) != 
> 0) {
> +                qerror_report_err(err);
> +                error_free(err);
>                  exit(1);
>              }
>          }