On Fri, 1 Feb 2013 18:38:15 +0100
Laszlo Ersek <ler...@redhat.com> wrote:
>
> Signed-off-by: Laszlo Ersek <ler...@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);
> }
> }
