Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
> Also polish an error message while I'm touching the line anyway,
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
> ---
> include/monitor/qdev.h | 2 +-
> qdev-monitor.c | 36 +++++++++++++++---------------------
> vl.c | 7 +++++--
> 3 files changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 7190752..2ce8578 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -11,6 +11,6 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict);
> void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
> int do_device_add(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/qdev-monitor.c b/qdev-monitor.c
> index e7e9a50..686c9df 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -516,7 +516,7 @@ static BusState *qbus_find(const char *path, Error **errp)
> return bus;
> }
>
> -DeviceState *qdev_device_add(QemuOpts *opts)
> +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> {
> DeviceClass *dc;
> const char *driver, *path, *id;
> @@ -526,44 +526,38 @@ 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;
> }
>
> /* find driver */
> - dc = qdev_get_device_class(&driver, &err);
> - if (err) {
> - qerror_report_err(err);
> - error_free(err);
> + dc = qdev_get_device_class(&driver, errp);
> + if (!dc) {
Here ...
> return NULL;
> }
>
> /* find bus */
> path = qemu_opt_get(opts, "bus");
> if (path != NULL) {
> - bus = qbus_find(path, &err);
> + bus = qbus_find(path, errp);
> if (!bus) {
> - qerror_report_err(err);
> - error_free(err);
... and here you've elegantly eliminated assumptions about return value
and errp relations. Before, we were accessing err without checking that
it is non-NULL - error_free() shouldn't care but qerror_report_err()
might and just out of design principle.
> return NULL;
> }
> if (!object_dynamic_cast(OBJECT(bus), dc->bus_type)) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR,
> - "Device '%s' can't go on a %s bus",
> - driver, object_get_typename(OBJECT(bus)));
> + error_setg(errp, "Device '%s' can't go on %s bus",
> + driver, object_get_typename(OBJECT(bus)));
> return NULL;
> }
> } else if (dc->bus_type != NULL) {
> bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
> if (!bus || qbus_is_full(bus)) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR,
> - "No '%s' bus found for device '%s'",
> - dc->bus_type, driver);
> + error_setg(errp, "No '%s' bus found for device '%s'",
> + dc->bus_type, driver);
> return NULL;
> }
> }
> if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
> - qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
> + error_set(errp, QERR_BUS_NO_HOTPLUG, bus->name);
> return NULL;
> }
>
> @@ -592,7 +586,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>
> /* set properties */
> if (qemu_opt_foreach(opts, set_property, dev, &err)) {
> - qerror_report_err(err);
> + error_propagate(errp, err);
> object_unparent(OBJECT(dev));
> object_unref(OBJECT(dev));
> return NULL;
> @@ -601,12 +595,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> dev->opts = opts;
> object_property_set_bool(OBJECT(dev), true, "realized", &err);
> if (err != NULL) {
> - qerror_report_err(err);
> - error_free(err);
> + error_propagate(errp, err);
> dev->opts = NULL;
> object_unparent(OBJECT(dev));
> object_unref(OBJECT(dev));
> - qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> return NULL;
> }
> return dev;
> @@ -779,8 +771,10 @@ int do_device_add(Monitor *mon, const QDict *qdict,
> QObject **ret_data)
> qemu_opts_del(opts);
> return 0;
> }
> - dev = qdev_device_add(opts);
> + dev = qdev_device_add(opts, &local_err);
> if (!dev) {
Would seem safer to change this to "if (local_err) {" now that it is
dealing with 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 9542095..f0dcb6b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2185,11 +2185,14 @@ static int device_help_func(void *opaque, QemuOpts
> *opts, Error **errp)
>
> static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
> {
> + Error *err = NULL;
> DeviceState *dev;
>
> - dev = qdev_device_add(opts);
> - if (!dev)
> + dev = qdev_device_add(opts, &err);
> + if (!dev) {
> + error_report_err(err);
> return -1;
> + }
> object_unref(OBJECT(dev));
> return 0;
> }
Anyway,
Reviewed-by: Andreas Färber <afaer...@suse.de>
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
