| Message ID | 1359740299-27968-4-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 1 Feb 2013 18:38:15 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> 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); > } > }
On 02/04/13 18:27, Luiz Capitulino wrote: > On Fri, 1 Feb 2013 18:38:15 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > 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
On Tue, 05 Feb 2013 01:31:14 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/04/13 18:27, Luiz Capitulino wrote: > > On Fri, 1 Feb 2013 18:38:15 +0100 > > Laszlo Ersek <lersek@redhat.com> wrote: > > > >> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > > > 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 >
On 02/05/13 13:20, Luiz Capitulino wrote: > On Tue, 05 Feb 2013 01:31:14 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 02/04/13 18:27, Luiz Capitulino wrote: >>> >>> 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. OK, let me post v2 soon. I don't expect it to be final, but hopefully it'll enable us to discuss it in more targeted details. Thanks much! Laszlo
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; } 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; } 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); } }
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/qdev-properties.h | 4 +++- hw/qdev-monitor.c | 26 +++++++++++++++++++++----- hw/qdev-properties.c | 12 ++++++++---- 3 files changed, 32 insertions(+), 10 deletions(-)