From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36843) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5wg6-0003Mx-LG for qemu-devel@nongnu.org; Fri, 19 Jun 2015 09:49:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5wg1-0002hF-N1 for qemu-devel@nongnu.org; Fri, 19 Jun 2015 09:49:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33544) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5wg1-0002gt-GN for qemu-devel@nongnu.org; Fri, 19 Jun 2015 09:49:49 -0400 From: Markus Armbruster References: <1434194302-26589-1-git-send-email-armbru@redhat.com> <1434194302-26589-8-git-send-email-armbru@redhat.com> <5584145F.6040802@suse.de> Date: Fri, 19 Jun 2015 15:49:46 +0200 In-Reply-To: <5584145F.6040802@suse.de> ("Andreas =?utf-8?Q?F=C3=A4rber=22?= =?utf-8?Q?'s?= message of "Fri, 19 Jun 2015 15:08:47 +0200") Message-ID: <87pp4rlsed.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 7/7] qdev-monitor: Propagate errors through qdev_device_add() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com Andreas F=C3=A4rber writes: > Am 13.06.2015 um 13:18 schrieb Markus Armbruster: >> Also polish an error message while I'm touching the line anyway, >>=20 >> Signed-off-by: Markus Armbruster >> Reviewed-by: Eric Blake >> --- >> include/monitor/qdev.h | 2 +- >> qdev-monitor.c | 36 +++++++++++++++--------------------- >> vl.c | 7 +++++-- >> 3 files changed, 21 insertions(+), 24 deletions(-) >>=20 >> 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); >>=20=20 >> #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; >> } >>=20=20 >> -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) >>=20=20 >> driver =3D qemu_opt_get(opts, "driver"); >> if (!driver) { >> - qerror_report(QERR_MISSING_PARAMETER, "driver"); >> + error_set(errp, QERR_MISSING_PARAMETER, "driver"); >> return NULL; >> } >>=20=20 >> /* find driver */ >> - dc =3D qdev_get_device_class(&driver, &err); >> - if (err) { >> - qerror_report_err(err); >> - error_free(err); >> + dc =3D qdev_get_device_class(&driver, errp); >> + if (!dc) { > > Here ... > >> return NULL; >> } >>=20=20 >> /* find bus */ >> path =3D qemu_opt_get(opts, "bus"); >> if (path !=3D NULL) { >> - bus =3D qbus_find(path, &err); >> + bus =3D 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 !=3D NULL) { >> bus =3D 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; >> } >>=20=20 >> @@ -592,7 +586,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >>=20=20 >> /* 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 =3D opts; >> object_property_set_bool(OBJECT(dev), true, "realized", &err); >> if (err !=3D NULL) { >> - qerror_report_err(err); >> - error_free(err); >> + error_propagate(errp, err); >> dev->opts =3D 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 =3D qdev_device_add(opts); >> + dev =3D 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 =3D 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, QemuOp= ts *opts, Error **errp) >>=20=20 >> static int device_init_func(void *opaque, QemuOpts *opts, Error **errp) >> { >> + Error *err =3D NULL; >> DeviceState *dev; >>=20=20 >> - dev =3D qdev_device_add(opts); >> - if (!dev) >> + dev =3D qdev_device_add(opts, &err); >> + if (!dev) { >> + error_report_err(err); >> return -1; >> + } >> object_unref(OBJECT(dev)); >> return 0; >> } > > Anyway, > > Reviewed-by: Andreas F=C3=A4rber Thanks!