Message ID | 1441901956-21991-1-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > 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> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > > v4: add R-b, rebase to latest qemu.git > 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 426d5ea..d7878c3 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -155,6 +155,13 @@ void error_setg_win32_internal(Error **errp, > */ > void error_propagate(Error **dst_errp, Error *local_err); > > +/** > + * 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); > + > /* > * Convenience function to report open() failure. > */ > 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 cdb726c..9dd474f 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -20,6 +20,7 @@ struct Error > ErrorClass err_class; > const char *src, *func; > int line; > + GString *hint; > }; > > Error *error_abort; > @@ -115,6 +116,28 @@ void error_setg_file_open_internal(Error **errp, > "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_setg_win32_internal(Error **errp, > @@ -151,6 +174,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; > } > @@ -168,6 +194,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); > } > > @@ -175,6 +204,9 @@ void error_free(Error *err) > { > if (err) { > g_free(err->msg); > + if (err->hint) { > + g_string_free(err->hint, true); > + } > g_free(err); > } > } Once again, GLib demonstrates its lack of taste. > diff --git a/util/qemu-option.c b/util/qemu-option.c > index efd6f02..6c76ed2 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -205,10 +205,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 { > @@ -648,9 +646,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); I'll take this through my tree. Thanks!
diff --git a/include/qapi/error.h b/include/qapi/error.h index 426d5ea..d7878c3 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -155,6 +155,13 @@ void error_setg_win32_internal(Error **errp, */ void error_propagate(Error **dst_errp, Error *local_err); +/** + * 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); + /* * Convenience function to report open() failure. */ 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 cdb726c..9dd474f 100644 --- a/util/error.c +++ b/util/error.c @@ -20,6 +20,7 @@ struct Error ErrorClass err_class; const char *src, *func; int line; + GString *hint; }; Error *error_abort; @@ -115,6 +116,28 @@ void error_setg_file_open_internal(Error **errp, "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_setg_win32_internal(Error **errp, @@ -151,6 +174,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; } @@ -168,6 +194,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); } @@ -175,6 +204,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 efd6f02..6c76ed2 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -205,10 +205,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 { @@ -648,9 +646,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);