From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56382) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRlgO-0004OR-1C for qemu-devel@nongnu.org; Tue, 18 Aug 2015 14:32:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRlgL-0001cl-9W for qemu-devel@nongnu.org; Tue, 18 Aug 2015 14:32:23 -0400 Received: from resqmta-po-03v.sys.comcast.net ([96.114.154.162]:44490) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRlgL-0001cO-24 for qemu-devel@nongnu.org; Tue, 18 Aug 2015 14:32:21 -0400 References: <1439921996-6425-1-git-send-email-eblake@redhat.com> From: Eric Blake Message-ID: <55D37A2A.9010407@redhat.com> Date: Tue, 18 Aug 2015 11:32:10 -0700 MIME-Version: 1.0 In-Reply-To: <1439921996-6425-1-git-send-email-eblake@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="h1NUoa8g89cQCu0P4iMiXH2d2D2Buk3S8" Subject: Re: [Qemu-devel] [PATCH] hmp: Allow for error message hints on HMP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Markus Armbruster , Stefan Hajnoczi , =?UTF-8?Q?Andreas_F=c3=a4rber?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --h1NUoa8g89cQCu0P4iMiXH2d2D2Buk3S8 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable [adding some potential maintainers in cc; ./scripts/get_maintainer.pl struck out in spite of the patch touching multiple files. Maybe that means we need a MAINTAINERS update?] On 08/18/2015 11:19 AM, Eric Blake wrote: > Commits 7216ae3d and d2828429 disabled some error message hints, > all because a change to use modern error reporting meant that the > hint would be output prior to the actual error. Fix this by making > hints a first-class member of Error. >=20 > For example, we are now back to the pleasant: >=20 > $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=3D, > qemu-system-x86_64: --chardev null,id=3D,: Parameter 'id' expects an i= dentifier > Identifiers consist of letters, digits, '-', '.', '_', starting with a= letter. >=20 > Signed-off-by: Eric Blake > --- > include/qapi/error.h | 7 +++++++ > qdev-monitor.c | 42 ++++++++++++++---------------------------- > util/error.c | 31 +++++++++++++++++++++++++++++++ > util/qemu-option.c | 11 ++++------- > 4 files changed, 56 insertions(+), 35 deletions(-) >=20 > diff --git a/include/qapi/error.h b/include/qapi/error.h > index f44c451..00daea3 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -67,6 +67,13 @@ void error_set_win32(Error **errp, int win32_err, Er= rorClass err_class, > */ > void error_setg_file_open(Error **errp, int os_errno, const char *file= name); >=20 > +/** > + * Append a printf-style human-readable explanation to an existing err= or. > + * May be called multiple times, and safe if @errp is NULL. > + */ > +void error_append_hint(Error **errp, const char *fmt, ...) > + GCC_FMT_ATTR(2, 3); > + > /* > * Get the error class of an error object. > */ > diff --git a/qdev-monitor.c b/qdev-monitor.c > index f9e2d62..0bf7f83 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -289,37 +289,35 @@ static Object *qdev_get_peripheral_anon(void) > return dev; > } >=20 > -#if 0 /* conversion from qerror_report() to error_set() broke their us= e */ > -static void qbus_list_bus(DeviceState *dev) > +static void qbus_list_bus(DeviceState *dev, Error **errp) > { > BusState *child; > const char *sep =3D " "; >=20 > - error_printf("child buses at \"%s\":", > - dev->id ? dev->id : object_get_typename(OBJECT(dev)))= ; > + error_append_hint(errp, "child buses at \"%s\":", > + dev->id ? dev->id : object_get_typename(OBJECT(d= ev))); > QLIST_FOREACH(child, &dev->child_bus, sibling) { > - error_printf("%s\"%s\"", sep, child->name); > + error_append_hint(errp, "%s\"%s\"", sep, child->name); > sep =3D ", "; > } > - error_printf("\n"); > } >=20 > -static void qbus_list_dev(BusState *bus) > +static void qbus_list_dev(BusState *bus, Error **errp) > { > BusChild *kid; > const char *sep =3D " "; >=20 > - error_printf("devices at \"%s\":", bus->name); > + error_append_hint(errp, "devices at \"%s\":", bus->name); > QTAILQ_FOREACH(kid, &bus->children, sibling) { > DeviceState *dev =3D kid->child; > - error_printf("%s\"%s\"", sep, object_get_typename(OBJECT(dev))= ); > - if (dev->id) > - error_printf("/\"%s\"", dev->id); > + error_append_hint(errp, "%s\"%s\"", sep, > + object_get_typename(OBJECT(dev))); > + if (dev->id) { > + error_append_hint(errp, "/\"%s\"", dev->id); > + } > sep =3D ", "; > } > - error_printf("\n"); > } > -#endif >=20 > static BusState *qbus_find_bus(DeviceState *dev, char *elem) > { > @@ -461,11 +459,7 @@ static BusState *qbus_find(const char *path, Error= **errp) > if (!dev) { > error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > "Device '%s' not found", elem); > -#if 0 /* conversion from qerror_report() to error_set() broke this: */= > - if (!monitor_cur_is_qmp()) { > - qbus_list_dev(bus); > - } > -#endif > + qbus_list_dev(bus, errp); > return NULL; > } >=20 > @@ -483,11 +477,7 @@ static BusState *qbus_find(const char *path, Error= **errp) > if (dev->num_child_bus) { > error_setg(errp, "Device '%s' has multiple child buses= ", > elem); > -#if 0 /* conversion from qerror_report() to error_set() broke this: */= > - if (!monitor_cur_is_qmp()) { > - qbus_list_bus(dev); > - } > -#endif > + qbus_list_bus(dev, errp); > } else { > error_setg(errp, "Device '%s' has no child bus", elem)= ; > } > @@ -503,11 +493,7 @@ static BusState *qbus_find(const char *path, Error= **errp) > bus =3D qbus_find_bus(dev, elem); > if (!bus) { > error_setg(errp, "Bus '%s' not found", elem); > -#if 0 /* conversion from qerror_report() to error_set() broke this: */= > - if (!monitor_cur_is_qmp()) { > - qbus_list_bus(dev); > - } > -#endif > + qbus_list_bus(dev, errp); > return NULL; > } > } > diff --git a/util/error.c b/util/error.c > index 14f4351..70d7ed0 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -18,6 +18,7 @@ struct Error > { > char *msg; > ErrorClass err_class; > + char *hint; > }; >=20 > Error *error_abort; > @@ -91,6 +92,31 @@ void error_setg_file_open(Error **errp, int os_errno= , const char *filename) > error_setg_errno(errp, os_errno, "Could not open '%s'", filename);= > } >=20 > +void error_append_hint(Error **errp, const char *fmt, ...) > +{ > + char *msg1; > + char *tmp; > + va_list ap; > + int saved_errno =3D errno; > + Error *err; > + > + if (!errp) { > + return; > + } > + err =3D *errp; > + assert(err && errp !=3D &error_abort); > + > + va_start(ap, fmt); > + msg1 =3D g_strdup_vprintf(fmt, ap); > + tmp =3D err->hint; > + err->hint =3D g_strdup_printf("%s%s", tmp ? tmp : "", msg1); > + g_free(tmp); > + g_free(msg1); > + va_end(ap); > + > + errno =3D saved_errno; > +} > + > #ifdef _WIN32 >=20 > void error_set_win32(Error **errp, int win32_err, ErrorClass err_class= , > @@ -138,6 +164,7 @@ Error *error_copy(const Error *err) > err_new =3D g_malloc0(sizeof(*err)); > err_new->msg =3D g_strdup(err->msg); > err_new->err_class =3D err->err_class; > + err_new->hint =3D g_strdup(err->hint); >=20 > return err_new; > } > @@ -155,6 +182,9 @@ const char *error_get_pretty(Error *err) > void error_report_err(Error *err) > { > error_report("%s", error_get_pretty(err)); > + if (err->hint) { > + error_printf_unless_qmp("%s\n", err->hint); > + } > error_free(err); > } >=20 > @@ -162,6 +192,7 @@ void error_free(Error *err) > { > if (err) { > g_free(err->msg); > + g_free(err->hint); > g_free(err); > } > } > diff --git a/util/qemu-option.c b/util/qemu-option.c > index efe9d27..03201ad 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -200,10 +200,8 @@ void parse_option_size(const char *name, const cha= r *value, > break; > default: > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a si= ze"); > -#if 0 /* conversion from qerror_report() to error_set() broke this: */= > - error_printf_unless_qmp("You may use k, M, G or T suffixes= for " > - "kilobytes, megabytes, gigabytes and terabytes.\n"= ); > -#endif > + error_append_hint(errp, "You may use k, M, G or T suffixes= for " > + "kilobytes, megabytes, gigabytes and terabytes.");= > return; > } > } else { > @@ -643,9 +641,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, cons= t char *id, > if (!id_wellformed(id)) { > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id", > "an identifier"); > -#if 0 /* conversion from qerror_report() to error_set() broke this: */= > - error_printf_unless_qmp("Identifiers consist of letters, d= igits, '-', '.', '_', starting with a letter.\n"); > -#endif > + error_append_hint(errp, "Identifiers consist of letters, d= igits, " > + "'-', '.', '_', starting with a letter."= ); > return NULL; > } > opts =3D qemu_opts_find(list, id); >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --h1NUoa8g89cQCu0P4iMiXH2d2D2Buk3S8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV03oqAAoJEKeha0olJ0NqXtkH/3p1Gaomb1DdpnZ4czPdT+f9 H2chn6XB5cEj1b38J/MYNTFkZiatHK6B0luSZVfp3qCRn3/xAGJv2IqyL9YsxHBE nN82qYaca5gGtzFx3AkxHyYRHUQX8KuodYnNx5ntXTQ8Hibg3EJ8SsbbmSK7d2d3 PtoelsVap6jKb+SxN7amB1p2/UGtX7WYVTuLP8+JbIYbEhUk8xDXldaUMRBVgOMC nucIsi25ty/83cLudye4swLHfd2Z25A1fjNkxrE/7Io6zRTQO6CrL7OzWdxcgkGf kuLbPK+1pZP69Qs0oHLeSdpFH4v9Ilwk9sPYN0ucPZ5LPgiUjta2jqdYfA4Xm5s= =iQyL -----END PGP SIGNATURE----- --h1NUoa8g89cQCu0P4iMiXH2d2D2Buk3S8--