[v2,06/15] set_property(): extend signature with Error

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev-monitor.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

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