From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5w2N-0000Jx-UM for qemu-devel@nongnu.org; Fri, 19 Jun 2015 09:08:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5w2K-000336-Hj for qemu-devel@nongnu.org; Fri, 19 Jun 2015 09:08:51 -0400 Received: from cantor2.suse.de ([195.135.220.15]:44069 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5w2K-00032r-8J for qemu-devel@nongnu.org; Fri, 19 Jun 2015 09:08:48 -0400 Message-ID: <5584145F.6040802@suse.de> Date: Fri, 19 Jun 2015 15:08:47 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1434194302-26589-1-git-send-email-armbru@redhat.com> <1434194302-26589-8-git-send-email-armbru@redhat.com> In-Reply-To: <1434194302-26589-8-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=iso-8859-15 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: Markus Armbruster , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, kraxel@redhat.com 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 > #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 > -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 > 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 > /* 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 > /* 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->bu= s_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 > @@ -592,7 +586,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > =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; > } > 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, QemuO= pts *opts, Error **errp) > =20 > static int device_init_func(void *opaque, QemuOpts *opts, Error **errp= ) > { > + Error *err =3D NULL; > DeviceState *dev; > =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=E4rber Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=FCrnberg)