Andreas Färber <afaer...@suse.de> writes:
> 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;
>> }
After my next patch series, this will be
dev = qdev_device_add(opts, &local_err);
if (!dev) {
error_propagate(errp, local_err);
qemu_opts_del(opts);
return;
}
We got similar logic all over the tree.
Whether to check the returned pointer or the error variable is a matter
of taste. I prefer to check the pointer, not least because it's less
likely to confuse Coverity in my experience.
>> 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>
Thanks!
