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

[Luiz Capitulino]

On Tue, 05 Feb 2013 01:31:14 +0100
Laszlo Ersek <address@hidden> wrote:

> 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 I got what you meant, the called function shouldn't be using error_set()
at this point, as it doesn't even take an Error ** argument.

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

Yes, but it's case-by-case. For example, if the called function prints to
the monitor or uses fprintf(), then it's OK to keep them until all callers
are converted.

Now, if the called function reports error with qerror_report() then
we have different options. You could call qerror_report_err() at some
point in the call stack, for example.

But, honestly speaking, I think I went too general on my feedback and
lost sight of the details concerning this patch and I don't my feedback
is good enough at this point, sorry for that. I'll try to be more
specific when you respin.

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

The bottom line of the feedback I probably failed to provide is that I
usually separate adding Error ** handling from everything else so that
each patch does one thing only.

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

Fair enough.

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