Patchwork [v2,07/15] set_property(): push error handling to callers

login
register
mail settings
Submitter Laszlo Ersek
Date Feb. 5, 2013, 8:39 p.m.
Message ID <1360096768-8863-8-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/218346/
State New
Headers show

Comments

Laszlo Ersek - Feb. 5, 2013, 8:39 p.m.
The return type can't be changed to void, because "set_property" must have
type "qemu_opt_loopfunc".

Conversion status (call chains covered or substituted by error propagation
marked with square brackets):

do_device_add -> [qemu_find_opts -> error_report]
do_device_add -> qdev_device_add -> qerror_report
do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive
  -> qerror_report
do_device_add -> qdev_device_add -> qbus_find -> qerror_report
do_device_add -> qdev_device_add -> [set_property -> qdev_prop_parse
  -> qerror_report_err]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev-monitor.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
Luiz Capitulino - Feb. 7, 2013, 6:55 p.m.
On Tue,  5 Feb 2013 21:39:20 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> The return type can't be changed to void, because "set_property" must have
> type "qemu_opt_loopfunc".
> 
> Conversion status (call chains covered or substituted by error propagation
> marked with square brackets):

This is fine to be squashed with the previous patch because only
set_property() and its caller are touched. Hope I didn't say the
contrary for this patch in my last review :)

One more comment below.

> 
> do_device_add -> [qemu_find_opts -> error_report]
> do_device_add -> qdev_device_add -> qerror_report
> do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive
>   -> qerror_report
> do_device_add -> qdev_device_add -> qbus_find -> qerror_report
> do_device_add -> qdev_device_add -> [set_property -> qdev_prop_parse
>   -> qerror_report_err]
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 545e66c..da32763 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -117,8 +117,7 @@ static int set_property(const char *name, const char *value, void *opaque)
>  
>      qdev_prop_parse(ctx->dev, name, value, &err);
>      if (error_is_set(&err)) {
> -        qerror_report_err(err);
> -        error_free(err);
> +        error_propagate(ctx->errp, err);
>          return -1;
>      }
>      return 0;
> @@ -424,6 +423,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) {
> @@ -488,9 +488,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>  
>      {
> -        PropertyContext ctx = { .dev = qdev, .errp = NULL };
> +        PropertyContext ctx = { .dev = qdev, .errp = &local_err };
>  
>          if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) {

It's better to use local_err to check for error, as that's what
you use below.

> +            qerror_report_err(local_err);
> +            error_free(local_err);
>              qdev_free(qdev);
>              return NULL;
>          }
Markus Armbruster - March 20, 2013, 2:19 p.m.
Laszlo Ersek <lersek@redhat.com> writes:

> The return type can't be changed to void, because "set_property" must have
> type "qemu_opt_loopfunc".

Further evidence of qemu_opt_foreach()'s awkwardness.

[...]

Patch

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 545e66c..da32763 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -117,8 +117,7 @@  static int set_property(const char *name, const char *value, void *opaque)
 
     qdev_prop_parse(ctx->dev, name, value, &err);
     if (error_is_set(&err)) {
-        qerror_report_err(err);
-        error_free(err);
+        error_propagate(ctx->errp, err);
         return -1;
     }
     return 0;
@@ -424,6 +423,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) {
@@ -488,9 +488,11 @@  DeviceState *qdev_device_add(QemuOpts *opts)
     }
 
     {
-        PropertyContext ctx = { .dev = qdev, .errp = 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;
         }