Message ID | 1360096768-8863-7-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Feb 05, 2013 at 09:39:19PM +0100, Laszlo Ersek wrote: > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/qdev-monitor.c | 21 ++++++++++++++++----- > 1 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index cf96046..545e66c 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -100,9 +100,14 @@ 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) > @@ -110,7 +115,7 @@ static int set_property(const char *name, const char *value, void *opaque) > if (strcmp(name, "bus") == 0) > return 0; > > - qdev_prop_parse(dev, name, value, &err); > + qdev_prop_parse(ctx->dev, name, value, &err); > if (error_is_set(&err)) { > qerror_report_err(err); > error_free(err); > @@ -481,10 +486,16 @@ 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 = NULL }; > + > + if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) { What about redefining qemu_opt_loopfunc to accept an Error parameter? qemu_opt_foreach() is used in only 4 places in the whole QEMU code, and it already has code to abort on error (but using the function return value instead of an Error** paramater). > + qdev_free(qdev); > + return NULL; > + } > } > + > if (qdev->id) { > object_property_add_child(qdev_get_peripheral(), qdev->id, > OBJECT(qdev), NULL); > -- > 1.7.1 > >
On Thu, 7 Feb 2013 15:18:41 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 05, 2013 at 09:39:19PM +0100, Laszlo Ersek wrote: > > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > hw/qdev-monitor.c | 21 ++++++++++++++++----- > > 1 files changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > > index cf96046..545e66c 100644 > > --- a/hw/qdev-monitor.c > > +++ b/hw/qdev-monitor.c > > @@ -100,9 +100,14 @@ 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) > > @@ -110,7 +115,7 @@ static int set_property(const char *name, const char *value, void *opaque) > > if (strcmp(name, "bus") == 0) > > return 0; > > > > - qdev_prop_parse(dev, name, value, &err); > > + qdev_prop_parse(ctx->dev, name, value, &err); > > if (error_is_set(&err)) { > > qerror_report_err(err); > > error_free(err); > > @@ -481,10 +486,16 @@ 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 = NULL }; > > + > > + if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) { > > What about redefining qemu_opt_loopfunc to accept an Error parameter? > > qemu_opt_foreach() is used in only 4 places in the whole QEMU code, and > it already has code to abort on error (but using the function return > value instead of an Error** paramater). He would have to convert the users too, which would be very welcome, but it's out of the scope of this series IMO (but would be fine as a first series to this work). > > > > > + qdev_free(qdev); > > + return NULL; > > + } > > } > > + > > if (qdev->id) { > > object_property_add_child(qdev_get_peripheral(), qdev->id, > > OBJECT(qdev), NULL); > > -- > > 1.7.1 > > > > >
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Thu, 7 Feb 2013 15:18:41 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Tue, Feb 05, 2013 at 09:39:19PM +0100, Laszlo Ersek wrote: >> > >> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> > --- >> > hw/qdev-monitor.c | 21 ++++++++++++++++----- >> > 1 files changed, 16 insertions(+), 5 deletions(-) >> > >> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c >> > index cf96046..545e66c 100644 >> > --- a/hw/qdev-monitor.c >> > +++ b/hw/qdev-monitor.c >> > @@ -100,9 +100,14 @@ 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) >> > @@ -110,7 +115,7 @@ static int set_property(const char *name, const char *value, void *opaque) >> > if (strcmp(name, "bus") == 0) >> > return 0; >> > >> > - qdev_prop_parse(dev, name, value, &err); >> > + qdev_prop_parse(ctx->dev, name, value, &err); >> > if (error_is_set(&err)) { >> > qerror_report_err(err); >> > error_free(err); >> > @@ -481,10 +486,16 @@ 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 = NULL }; >> > + >> > + if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) { >> >> What about redefining qemu_opt_loopfunc to accept an Error parameter? >> >> qemu_opt_foreach() is used in only 4 places in the whole QEMU code, and >> it already has code to abort on error (but using the function return >> value instead of an Error** paramater). > > He would have to convert the users too, which would be very welcome, > but it's out of the scope of this series IMO (but would be fine as > a first series to this work). I'm fine with this patch as is. I wouldn't add Error support to qemu_opt_foreach(), I'd replace it by a more C-ish qemu_opt_next(): for (opt = qemu_opt_next(NULL); opt; opt = qemu_opt_next(opt)) { set_property(qdev, qemu_opt_name(opt), qemu_opt_value(opt), &err); if (err) { ... break; } } Higher-order functions are great in Lisp, but awkward in C. > >> >> >> >> > + qdev_free(qdev); >> > + return NULL; >> > + } >> > } >> > + >> > if (qdev->id) { >> > object_property_add_child(qdev_get_peripheral(), qdev->id, >> > OBJECT(qdev), NULL); >> > -- >> > 1.7.1 >> > >> > >>
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index cf96046..545e66c 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -100,9 +100,14 @@ 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) @@ -110,7 +115,7 @@ static int set_property(const char *name, const char *value, void *opaque) if (strcmp(name, "bus") == 0) return 0; - qdev_prop_parse(dev, name, value, &err); + qdev_prop_parse(ctx->dev, name, value, &err); if (error_is_set(&err)) { qerror_report_err(err); error_free(err); @@ -481,10 +486,16 @@ 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 = NULL }; + + if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) { + qdev_free(qdev); + return NULL; + } } + if (qdev->id) { object_property_add_child(qdev_get_peripheral(), qdev->id, OBJECT(qdev), NULL);
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/qdev-monitor.c | 21 ++++++++++++++++----- 1 files changed, 16 insertions(+), 5 deletions(-)