Patchwork [7/7] qdev_device_add(): report errors with Error

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

Comments

Laszlo Ersek - Feb. 1, 2013, 5:38 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev-monitor.h |    3 ++-
 hw/qdev-monitor.c |   29 +++++++++++++++--------------
 vl.c              |    9 +++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)
Luiz Capitulino - Feb. 4, 2013, 5:59 p.m.
On Fri,  1 Feb 2013 18:38:19 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.h |    3 ++-
>  hw/qdev-monitor.c |   29 +++++++++++++++--------------
>  vl.c              |    9 +++++++--
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/qdev-monitor.h b/hw/qdev-monitor.h
> index 9ec4850..3760bf5 100644
> --- a/hw/qdev-monitor.h
> +++ b/hw/qdev-monitor.h
> @@ -3,6 +3,7 @@
>  
>  #include "qdev-core.h"
>  #include "monitor/monitor.h"
> +#include "qapi/error.h"
>  
>  /*** monitor commands ***/
>  
> @@ -11,6 +12,6 @@ void do_info_qdm(Monitor *mon, const QDict *qdict);
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int qdev_device_help(QemuOpts *opts);
> -DeviceState *qdev_device_add(QemuOpts *opts);
> +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
>  
>  #endif
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 0c01b04..7e103af 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -417,7 +417,7 @@ static BusState *qbus_find(const char *path, Error **errp)
>      }
>  }
>  
> -DeviceState *qdev_device_add(QemuOpts *opts)
> +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>  {
>      ObjectClass *obj;
>      DeviceClass *k;
> @@ -428,7 +428,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>  
>      driver = qemu_opt_get(opts, "driver");
>      if (!driver) {
> -        qerror_report(QERR_MISSING_PARAMETER, "driver");
> +        error_set(errp, QERR_MISSING_PARAMETER, "driver");
>          return NULL;
>      }
>  
> @@ -444,7 +444,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>  
>      if (!obj) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver", "device type");

This patch looks correct, just one small note: you're free to use error_setg()
and improve the error message for all error classes that are not part of
the @ErrorClass enum.

The example above, for example, could be improved I guess. As the real error
cause seems to be something like "driver name not found" and not
"Parameter 'driver' expects device type".

>          return NULL;
>      }
>  
> @@ -455,13 +455,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      if (path != NULL) {
>          bus = qbus_find(path, &local_err);
>          if (!bus) {
> -            qerror_report_err(local_err);
> -            error_free(local_err);
> +            error_propagate(errp, local_err);
>              return NULL;
>          }
>          if (!object_dynamic_cast(OBJECT(bus), k->bus_type)) {
> -            qerror_report(QERR_BAD_BUS_FOR_DEVICE,
> -                          driver, object_get_typename(OBJECT(bus)));
> +            error_set(errp, QERR_BAD_BUS_FOR_DEVICE,
> +                      driver, object_get_typename(OBJECT(bus)));
>              return NULL;
>          }
>      } else {
> @@ -469,13 +468,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>                                    &local_err);
>          if (!bus) {
>              assert(!error_is_set(&local_err));
> -            qerror_report(QERR_NO_BUS_FOR_DEVICE,
> -                          k->bus_type, driver);
> +            error_set(errp, QERR_NO_BUS_FOR_DEVICE, k->bus_type, driver);
>              return NULL;
>          }
>      }
>      if (qdev_hotplug && !bus->allow_hotplug) {
> -        qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
> +        error_set(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>          return NULL;
>      }
>  
> @@ -496,8 +494,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          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);
> +            error_propagate(errp, local_err);
>              qdev_free(qdev);
>              return NULL;
>          }
> @@ -514,7 +511,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          g_free(name);
>      }
>      if (qdev_init(qdev) < 0) {
> -        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> +        error_set(errp, QERR_DEVICE_INIT_FAILED, driver);
>          return NULL;
>      }
>      qdev->opts = opts;
> @@ -627,7 +624,11 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          qemu_opts_del(opts);
>          return 0;
>      }
> -    if (!qdev_device_add(opts)) {
> +
> +    if (!qdev_device_add(opts, &local_err)) {
> +        assert(error_is_set(&local_err));
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          qemu_opts_del(opts);
>          return -1;
>      }
> diff --git a/vl.c b/vl.c
> index 44656d8..f2f8093 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2232,10 +2232,15 @@ static int device_help_func(QemuOpts *opts, void *opaque)
>  static int device_init_func(QemuOpts *opts, void *opaque)
>  {
>      DeviceState *dev;
> +    Error *local_err = NULL;
>  
> -    dev = qdev_device_add(opts);
> -    if (!dev)
> +    dev = qdev_device_add(opts, &local_err);
> +    assert((dev == NULL) == error_is_set(&local_err));
> +    if (!dev) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          return -1;
> +    }
>      return 0;
>  }
>

Patch

diff --git a/hw/qdev-monitor.h b/hw/qdev-monitor.h
index 9ec4850..3760bf5 100644
--- a/hw/qdev-monitor.h
+++ b/hw/qdev-monitor.h
@@ -3,6 +3,7 @@ 
 
 #include "qdev-core.h"
 #include "monitor/monitor.h"
+#include "qapi/error.h"
 
 /*** monitor commands ***/
 
@@ -11,6 +12,6 @@  void do_info_qdm(Monitor *mon, const QDict *qdict);
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int qdev_device_help(QemuOpts *opts);
-DeviceState *qdev_device_add(QemuOpts *opts);
+DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
 
 #endif
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 0c01b04..7e103af 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -417,7 +417,7 @@  static BusState *qbus_find(const char *path, Error **errp)
     }
 }
 
-DeviceState *qdev_device_add(QemuOpts *opts)
+DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
     ObjectClass *obj;
     DeviceClass *k;
@@ -428,7 +428,7 @@  DeviceState *qdev_device_add(QemuOpts *opts)
 
     driver = qemu_opt_get(opts, "driver");
     if (!driver) {
-        qerror_report(QERR_MISSING_PARAMETER, "driver");
+        error_set(errp, QERR_MISSING_PARAMETER, "driver");
         return NULL;
     }
 
@@ -444,7 +444,7 @@  DeviceState *qdev_device_add(QemuOpts *opts)
     }
 
     if (!obj) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
         return NULL;
     }
 
@@ -455,13 +455,12 @@  DeviceState *qdev_device_add(QemuOpts *opts)
     if (path != NULL) {
         bus = qbus_find(path, &local_err);
         if (!bus) {
-            qerror_report_err(local_err);
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return NULL;
         }
         if (!object_dynamic_cast(OBJECT(bus), k->bus_type)) {
-            qerror_report(QERR_BAD_BUS_FOR_DEVICE,
-                          driver, object_get_typename(OBJECT(bus)));
+            error_set(errp, QERR_BAD_BUS_FOR_DEVICE,
+                      driver, object_get_typename(OBJECT(bus)));
             return NULL;
         }
     } else {
@@ -469,13 +468,12 @@  DeviceState *qdev_device_add(QemuOpts *opts)
                                   &local_err);
         if (!bus) {
             assert(!error_is_set(&local_err));
-            qerror_report(QERR_NO_BUS_FOR_DEVICE,
-                          k->bus_type, driver);
+            error_set(errp, QERR_NO_BUS_FOR_DEVICE, k->bus_type, driver);
             return NULL;
         }
     }
     if (qdev_hotplug && !bus->allow_hotplug) {
-        qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
+        error_set(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
 
@@ -496,8 +494,7 @@  DeviceState *qdev_device_add(QemuOpts *opts)
         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);
+            error_propagate(errp, local_err);
             qdev_free(qdev);
             return NULL;
         }
@@ -514,7 +511,7 @@  DeviceState *qdev_device_add(QemuOpts *opts)
         g_free(name);
     }
     if (qdev_init(qdev) < 0) {
-        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
+        error_set(errp, QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
     qdev->opts = opts;
@@ -627,7 +624,11 @@  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
         qemu_opts_del(opts);
         return 0;
     }
-    if (!qdev_device_add(opts)) {
+
+    if (!qdev_device_add(opts, &local_err)) {
+        assert(error_is_set(&local_err));
+        qerror_report_err(local_err);
+        error_free(local_err);
         qemu_opts_del(opts);
         return -1;
     }
diff --git a/vl.c b/vl.c
index 44656d8..f2f8093 100644
--- a/vl.c
+++ b/vl.c
@@ -2232,10 +2232,15 @@  static int device_help_func(QemuOpts *opts, void *opaque)
 static int device_init_func(QemuOpts *opts, void *opaque)
 {
     DeviceState *dev;
+    Error *local_err = NULL;
 
-    dev = qdev_device_add(opts);
-    if (!dev)
+    dev = qdev_device_add(opts, &local_err);
+    assert((dev == NULL) == error_is_set(&local_err));
+    if (!dev) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
+    }
     return 0;
 }