* [Qemu-devel] [PATCH v3] hmp: Allow for error message hints on HMP
@ 2015-08-19 16:34 Eric Blake
2015-09-07 11:15 ` Paolo Bonzini
0 siblings, 1 reply; 2+ messages in thread
From: Eric Blake @ 2015-08-19 16:34 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru, stefanha, afaerber
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.
For example, we are now back to the pleasant:
$ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
qemu-system-x86_64: --chardev null,id=,: Parameter 'id' expects an identifier
Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v3: don't crash when freeing error without hint [Eric]
v2: use GString instead of rolling our own O(n^2) string concast [Paolo]
include/qapi/error.h | 7 +++++++
qdev-monitor.c | 42 ++++++++++++++----------------------------
util/error.c | 32 ++++++++++++++++++++++++++++++++
util/qemu-option.c | 11 ++++-------
4 files changed, 57 insertions(+), 35 deletions(-)
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, ErrorClass err_class,
*/
void error_setg_file_open(Error **errp, int os_errno, const char *filename);
+/**
+ * Append a printf-style human-readable explanation to an existing error.
+ * 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;
}
-#if 0 /* conversion from qerror_report() to error_set() broke their use */
-static void qbus_list_bus(DeviceState *dev)
+static void qbus_list_bus(DeviceState *dev, Error **errp)
{
BusState *child;
const char *sep = " ";
- 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(dev)));
QLIST_FOREACH(child, &dev->child_bus, sibling) {
- error_printf("%s\"%s\"", sep, child->name);
+ error_append_hint(errp, "%s\"%s\"", sep, child->name);
sep = ", ";
}
- error_printf("\n");
}
-static void qbus_list_dev(BusState *bus)
+static void qbus_list_dev(BusState *bus, Error **errp)
{
BusChild *kid;
const char *sep = " ";
- error_printf("devices at \"%s\":", bus->name);
+ error_append_hint(errp, "devices at \"%s\":", bus->name);
QTAILQ_FOREACH(kid, &bus->children, sibling) {
DeviceState *dev = 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 = ", ";
}
- error_printf("\n");
}
-#endif
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;
}
@@ -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 = 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..df5d640 100644
--- a/util/error.c
+++ b/util/error.c
@@ -18,6 +18,7 @@ struct Error
{
char *msg;
ErrorClass err_class;
+ GString *hint;
};
Error *error_abort;
@@ -91,6 +92,28 @@ void error_setg_file_open(Error **errp, int os_errno, const char *filename)
error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
}
+void error_append_hint(Error **errp, const char *fmt, ...)
+{
+ va_list ap;
+ int saved_errno = errno;
+ Error *err;
+
+ if (!errp) {
+ return;
+ }
+ err = *errp;
+ assert(err && errp != &error_abort);
+
+ if (!err->hint) {
+ err->hint = g_string_new(NULL);
+ }
+ va_start(ap, fmt);
+ g_string_append_vprintf(err->hint, fmt, ap);
+ va_end(ap);
+
+ errno = saved_errno;
+}
+
#ifdef _WIN32
void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
@@ -138,6 +161,9 @@ Error *error_copy(const Error *err)
err_new = g_malloc0(sizeof(*err));
err_new->msg = g_strdup(err->msg);
err_new->err_class = err->err_class;
+ if (err->hint) {
+ err_new->hint = g_string_new(err->hint->str);
+ }
return err_new;
}
@@ -155,6 +181,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->str);
+ }
error_free(err);
}
@@ -162,6 +191,9 @@ void error_free(Error *err)
{
if (err) {
g_free(err->msg);
+ if (err->hint) {
+ g_string_free(err->hint, true);
+ }
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 char *value,
break;
default:
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
-#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, const 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, digits, '-', '.', '_', starting with a letter.\n");
-#endif
+ error_append_hint(errp, "Identifiers consist of letters, digits, "
+ "'-', '.', '_', starting with a letter.");
return NULL;
}
opts = qemu_opts_find(list, id);
--
2.4.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hmp: Allow for error message hints on HMP
2015-08-19 16:34 [Qemu-devel] [PATCH v3] hmp: Allow for error message hints on HMP Eric Blake
@ 2015-09-07 11:15 ` Paolo Bonzini
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Bonzini @ 2015-09-07 11:15 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: armbru, stefanha, afaerber
On 19/08/2015 18:34, 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.
>
> For example, we are now back to the pleasant:
>
> $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
> qemu-system-x86_64: --chardev null,id=,: Parameter 'id' expects an identifier
> Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> v3: don't crash when freeing error without hint [Eric]
> v2: use GString instead of rolling our own O(n^2) string concast [Paolo]
>
> include/qapi/error.h | 7 +++++++
> qdev-monitor.c | 42 ++++++++++++++----------------------------
> util/error.c | 32 ++++++++++++++++++++++++++++++++
> util/qemu-option.c | 11 ++++-------
> 4 files changed, 57 insertions(+), 35 deletions(-)
>
> 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, ErrorClass err_class,
> */
> void error_setg_file_open(Error **errp, int os_errno, const char *filename);
>
> +/**
> + * Append a printf-style human-readable explanation to an existing error.
> + * 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;
> }
>
> -#if 0 /* conversion from qerror_report() to error_set() broke their use */
> -static void qbus_list_bus(DeviceState *dev)
> +static void qbus_list_bus(DeviceState *dev, Error **errp)
> {
> BusState *child;
> const char *sep = " ";
>
> - 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(dev)));
> QLIST_FOREACH(child, &dev->child_bus, sibling) {
> - error_printf("%s\"%s\"", sep, child->name);
> + error_append_hint(errp, "%s\"%s\"", sep, child->name);
> sep = ", ";
> }
> - error_printf("\n");
> }
>
> -static void qbus_list_dev(BusState *bus)
> +static void qbus_list_dev(BusState *bus, Error **errp)
> {
> BusChild *kid;
> const char *sep = " ";
>
> - error_printf("devices at \"%s\":", bus->name);
> + error_append_hint(errp, "devices at \"%s\":", bus->name);
> QTAILQ_FOREACH(kid, &bus->children, sibling) {
> DeviceState *dev = 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 = ", ";
> }
> - error_printf("\n");
> }
> -#endif
>
> 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;
> }
>
> @@ -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 = 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..df5d640 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -18,6 +18,7 @@ struct Error
> {
> char *msg;
> ErrorClass err_class;
> + GString *hint;
> };
>
> Error *error_abort;
> @@ -91,6 +92,28 @@ void error_setg_file_open(Error **errp, int os_errno, const char *filename)
> error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
> }
>
> +void error_append_hint(Error **errp, const char *fmt, ...)
> +{
> + va_list ap;
> + int saved_errno = errno;
> + Error *err;
> +
> + if (!errp) {
> + return;
> + }
> + err = *errp;
> + assert(err && errp != &error_abort);
> +
> + if (!err->hint) {
> + err->hint = g_string_new(NULL);
> + }
> + va_start(ap, fmt);
> + g_string_append_vprintf(err->hint, fmt, ap);
> + va_end(ap);
> +
> + errno = saved_errno;
> +}
> +
> #ifdef _WIN32
>
> void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
> @@ -138,6 +161,9 @@ Error *error_copy(const Error *err)
> err_new = g_malloc0(sizeof(*err));
> err_new->msg = g_strdup(err->msg);
> err_new->err_class = err->err_class;
> + if (err->hint) {
> + err_new->hint = g_string_new(err->hint->str);
> + }
>
> return err_new;
> }
> @@ -155,6 +181,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->str);
> + }
> error_free(err);
> }
>
> @@ -162,6 +191,9 @@ void error_free(Error *err)
> {
> if (err) {
> g_free(err->msg);
> + if (err->hint) {
> + g_string_free(err->hint, true);
> + }
> 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 char *value,
> break;
> default:
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
> -#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, const 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, digits, '-', '.', '_', starting with a letter.\n");
> -#endif
> + error_append_hint(errp, "Identifiers consist of letters, digits, "
> + "'-', '.', '_', starting with a letter.");
> return NULL;
> }
> opts = qemu_opts_find(list, id);
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-09-07 11:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-19 16:34 [Qemu-devel] [PATCH v3] hmp: Allow for error message hints on HMP Eric Blake
2015-09-07 11:15 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).