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

[Laszlo Ersek]

On 02/04/13 18:27, Luiz Capitulino wrote:
> 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

If the called function already switches to error_set() /
error_propagate() at this point, then standing at this patch in a
possible bisection, the error message is lost. (The caller doesn't print
it *yet*, the callee doesn't print it *any longer*.)

If, OTOH, the called function still prints the error message here, and
doesn't pass it up (only its signature has changed), then:

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

at some point there will be callers that need the callee to pass up the
error, and other callers (the ones not yet converted) that want the
callee not to.

I can
- switch callee and all callers at once (including prototype and error
handling), or
- put up with losing some messages inside the series, or
- put up with duplicate messages inside the series, or
- switch callee and callers at once (only prototype, error handling
stays unchanged), then switch them again all at once (error handling).

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

set_property() is called from qemu_opt_foreach() and must match the
qemu_opt_loopfunc prototype.

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

Yes.

Thanks
Laszlo