* [PATCH] qapi: give available enum values in error string @ 2023-03-07 11:22 marcandre.lureau 2023-03-07 12:39 ` Markus Armbruster 0 siblings, 1 reply; 4+ messages in thread From: marcandre.lureau @ 2023-03-07 11:22 UTC (permalink / raw) To: qemu-devel Cc: Marc-André Lureau, Markus Armbruster, Michael Roth, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost From: Marc-André Lureau <marcandre.lureau@redhat.com> This allows for a more pleasant user experience. Before: $ ./qemu-system-x86_64 -display egl-headless,gl= qemu-system-x86_64: -display egl-headless,gl=: Parameter 'gl' does not accept value '' After: $ ./qemu-system-x86_64 -display egl-headless,gl= qemu-system-x86_64: -display egl-headless,gl=help: Parameter 'gl' does not accept value '' (available: 'off', 'on', 'core', 'es') Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- include/qapi/util.h | 1 + qapi/qapi-util.c | 15 +++++++++++++++ qapi/qapi-visit-core.c | 7 +++++-- tests/unit/check-qom-proplist.c | 4 ++-- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/include/qapi/util.h b/include/qapi/util.h index 81a2b13a33..a5363e595d 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -23,6 +23,7 @@ typedef struct QEnumLookup { } QEnumLookup; const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); +char *qapi_enum_available(const QEnumLookup *lookup); int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, int def, Error **errp); bool qapi_bool_parse(const char *name, const char *value, bool *obj, diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c index 63596e11c5..19ba4b695a 100644 --- a/qapi/qapi-util.c +++ b/qapi/qapi-util.c @@ -84,6 +84,21 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, return def; } +char *qapi_enum_available(const QEnumLookup *lookup) +{ + int i; + GString *str = g_string_new(NULL); + + for (i = 0; i < lookup->size; i++) { + g_string_append_printf(str, "'%s'", lookup->array[i]); + if (i + 1 < lookup->size) { + g_string_append(str, ", "); + } + } + + return g_string_free(str, FALSE); +} + bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error **errp) { if (g_str_equal(value, "on") || diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 6c13510a2b..b887cbf3fd 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -404,8 +404,11 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, value = qapi_enum_parse(lookup, enum_str, -1, NULL); if (value < 0) { - error_setg(errp, "Parameter '%s' does not accept value '%s'", - name ? name : "null", enum_str); + g_autofree char *avail = NULL; + + avail = qapi_enum_available(lookup); + error_setg(errp, "Parameter '%s' does not accept value '%s' (available: %s)", + name ? name : "null", enum_str, avail); return false; } diff --git a/tests/unit/check-qom-proplist.c b/tests/unit/check-qom-proplist.c index 79d4a8b89d..eb0b215abb 100644 --- a/tests/unit/check-qom-proplist.c +++ b/tests/unit/check-qom-proplist.c @@ -488,8 +488,8 @@ static void test_dummy_badenum(void) g_assert(dobj == NULL); g_assert(err != NULL); - g_assert_cmpstr(error_get_pretty(err), ==, - "Parameter 'av' does not accept value 'yeti'"); + g_assert_nonnull(strstr(error_get_pretty(err), + "Parameter 'av' does not accept value 'yeti'")); g_assert(object_resolve_path_component(parent, "dummy0") == NULL); -- 2.39.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] qapi: give available enum values in error string 2023-03-07 11:22 [PATCH] qapi: give available enum values in error string marcandre.lureau @ 2023-03-07 12:39 ` Markus Armbruster 2023-03-07 13:11 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: Markus Armbruster @ 2023-03-07 12:39 UTC (permalink / raw) To: marcandre.lureau Cc: qemu-devel, Michael Roth, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > This allows for a more pleasant user experience. > > Before: > $ ./qemu-system-x86_64 -display egl-headless,gl= > qemu-system-x86_64: -display egl-headless,gl=: Parameter 'gl' does not accept value '' > > After: > $ ./qemu-system-x86_64 -display egl-headless,gl= > qemu-system-x86_64: -display egl-headless,gl=help: Parameter 'gl' does > not accept value '' (available: 'off', 'on', 'core', 'es') error.h recommends to user error_append_hint() for this: * Create an error and add additional explanation: * error_setg(errp, "invalid quark"); * error_append_hint(errp, "Valid quarks are up, down, strange, " * "charm, top, bottom.\n"); > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > include/qapi/util.h | 1 + > qapi/qapi-util.c | 15 +++++++++++++++ > qapi/qapi-visit-core.c | 7 +++++-- > tests/unit/check-qom-proplist.c | 4 ++-- > 4 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/include/qapi/util.h b/include/qapi/util.h > index 81a2b13a33..a5363e595d 100644 > --- a/include/qapi/util.h > +++ b/include/qapi/util.h > @@ -23,6 +23,7 @@ typedef struct QEnumLookup { > } QEnumLookup; > > const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); > +char *qapi_enum_available(const QEnumLookup *lookup); > int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, > int def, Error **errp); > bool qapi_bool_parse(const char *name, const char *value, bool *obj, > diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c > index 63596e11c5..19ba4b695a 100644 > --- a/qapi/qapi-util.c > +++ b/qapi/qapi-util.c > @@ -84,6 +84,21 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, > return def; > } > > +char *qapi_enum_available(const QEnumLookup *lookup) > +{ > + int i; > + GString *str = g_string_new(NULL); > + > + for (i = 0; i < lookup->size; i++) { > + g_string_append_printf(str, "'%s'", lookup->array[i]); > + if (i + 1 < lookup->size) { > + g_string_append(str, ", "); > + } > + } > + > + return g_string_free(str, FALSE); > +} > + If we use error_append_hint(), we can fold it into the reusable helper, and name the result error_append_qapi_enum_hint(). Easier to use than qapi_enum_available(), because the caller doesn't have to free anything. > bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error **errp) > { > if (g_str_equal(value, "on") || > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 6c13510a2b..b887cbf3fd 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -404,8 +404,11 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, > > value = qapi_enum_parse(lookup, enum_str, -1, NULL); > if (value < 0) { > - error_setg(errp, "Parameter '%s' does not accept value '%s'", > - name ? name : "null", enum_str); > + g_autofree char *avail = NULL; > + > + avail = qapi_enum_available(lookup); > + error_setg(errp, "Parameter '%s' does not accept value '%s' (available: %s)", > + name ? name : "null", enum_str, avail); > return false; > } > > diff --git a/tests/unit/check-qom-proplist.c b/tests/unit/check-qom-proplist.c > index 79d4a8b89d..eb0b215abb 100644 > --- a/tests/unit/check-qom-proplist.c > +++ b/tests/unit/check-qom-proplist.c > @@ -488,8 +488,8 @@ static void test_dummy_badenum(void) > > g_assert(dobj == NULL); > g_assert(err != NULL); > - g_assert_cmpstr(error_get_pretty(err), ==, > - "Parameter 'av' does not accept value 'yeti'"); > + g_assert_nonnull(strstr(error_get_pretty(err), > + "Parameter 'av' does not accept value 'yeti'")); > > g_assert(object_resolve_path_component(parent, "dummy0") > == NULL); No need to mess with the test then. What do you think? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qapi: give available enum values in error string 2023-03-07 12:39 ` Markus Armbruster @ 2023-03-07 13:11 ` Marc-André Lureau 2023-03-07 14:28 ` Markus Armbruster 0 siblings, 1 reply; 4+ messages in thread From: Marc-André Lureau @ 2023-03-07 13:11 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-devel, Michael Roth, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost [-- Attachment #1: Type: text/plain, Size: 4862 bytes --] Hi On Tue, Mar 7, 2023 at 4:39 PM Markus Armbruster <armbru@redhat.com> wrote: > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > This allows for a more pleasant user experience. > > > > Before: > > $ ./qemu-system-x86_64 -display egl-headless,gl= > > qemu-system-x86_64: -display egl-headless,gl=: Parameter 'gl' does not > accept value '' > > > > After: > > $ ./qemu-system-x86_64 -display egl-headless,gl= > > qemu-system-x86_64: -display egl-headless,gl=help: Parameter 'gl' does > > not accept value '' (available: 'off', 'on', 'core', 'es') > > error.h recommends to user error_append_hint() for this: > > * Create an error and add additional explanation: > * error_setg(errp, "invalid quark"); > * error_append_hint(errp, "Valid quarks are up, down, strange, " > * "charm, top, bottom.\n"); > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > include/qapi/util.h | 1 + > > qapi/qapi-util.c | 15 +++++++++++++++ > > qapi/qapi-visit-core.c | 7 +++++-- > > tests/unit/check-qom-proplist.c | 4 ++-- > > 4 files changed, 23 insertions(+), 4 deletions(-) > > > > diff --git a/include/qapi/util.h b/include/qapi/util.h > > index 81a2b13a33..a5363e595d 100644 > > --- a/include/qapi/util.h > > +++ b/include/qapi/util.h > > @@ -23,6 +23,7 @@ typedef struct QEnumLookup { > > } QEnumLookup; > > > > const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); > > +char *qapi_enum_available(const QEnumLookup *lookup); > > int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, > > int def, Error **errp); > > bool qapi_bool_parse(const char *name, const char *value, bool *obj, > > diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c > > index 63596e11c5..19ba4b695a 100644 > > --- a/qapi/qapi-util.c > > +++ b/qapi/qapi-util.c > > @@ -84,6 +84,21 @@ int qapi_enum_parse(const QEnumLookup *lookup, const > char *buf, > > return def; > > } > > > > +char *qapi_enum_available(const QEnumLookup *lookup) > > +{ > > + int i; > > + GString *str = g_string_new(NULL); > > + > > + for (i = 0; i < lookup->size; i++) { > > + g_string_append_printf(str, "'%s'", lookup->array[i]); > > + if (i + 1 < lookup->size) { > > + g_string_append(str, ", "); > > + } > > + } > > + > > + return g_string_free(str, FALSE); > > +} > > + > > If we use error_append_hint(), we can fold it into the reusable helper, > and name the result error_append_qapi_enum_hint(). Easier to use than > qapi_enum_available(), because the caller doesn't have to free anything. > Why not. That was not my first approach as it makes enumlookup->string specific to Error, but if they are no other users, that's fair. > > > bool qapi_bool_parse(const char *name, const char *value, bool *obj, > Error **errp) > > { > > if (g_str_equal(value, "on") || > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > > index 6c13510a2b..b887cbf3fd 100644 > > --- a/qapi/qapi-visit-core.c > > +++ b/qapi/qapi-visit-core.c > > @@ -404,8 +404,11 @@ static bool input_type_enum(Visitor *v, const char > *name, int *obj, > > > > value = qapi_enum_parse(lookup, enum_str, -1, NULL); > > if (value < 0) { > > - error_setg(errp, "Parameter '%s' does not accept value '%s'", > > - name ? name : "null", enum_str); > > + g_autofree char *avail = NULL; > > + > > + avail = qapi_enum_available(lookup); > > + error_setg(errp, "Parameter '%s' does not accept value '%s' > (available: %s)", > > + name ? name : "null", enum_str, avail); > > return false; > > } > > > > diff --git a/tests/unit/check-qom-proplist.c > b/tests/unit/check-qom-proplist.c > > index 79d4a8b89d..eb0b215abb 100644 > > --- a/tests/unit/check-qom-proplist.c > > +++ b/tests/unit/check-qom-proplist.c > > @@ -488,8 +488,8 @@ static void test_dummy_badenum(void) > > > > g_assert(dobj == NULL); > > g_assert(err != NULL); > > - g_assert_cmpstr(error_get_pretty(err), ==, > > - "Parameter 'av' does not accept value 'yeti'"); > > + g_assert_nonnull(strstr(error_get_pretty(err), > > + "Parameter 'av' does not accept value > 'yeti'")); > > > > g_assert(object_resolve_path_component(parent, "dummy0") > > == NULL); > > No need to mess with the test then. > > What do you think? > We hit an interesting limit of the API though, because error_setg() with &error_fatal will fail immediately before append_hint(). But we can work around it by using a ERRP_GUARD() ! sending v2 [-- Attachment #2: Type: text/html, Size: 6593 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qapi: give available enum values in error string 2023-03-07 13:11 ` Marc-André Lureau @ 2023-03-07 14:28 ` Markus Armbruster 0 siblings, 0 replies; 4+ messages in thread From: Markus Armbruster @ 2023-03-07 14:28 UTC (permalink / raw) To: Marc-André Lureau Cc: qemu-devel, Michael Roth, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost Marc-André Lureau <marcandre.lureau@redhat.com> writes: [...] > We hit an interesting limit of the API though, because error_setg() with > &error_fatal will fail immediately before append_hint(). > > But we can work around it by using a ERRP_GUARD() ! error.h again: * Create an error and add additional explanation: * error_setg(errp, "invalid quark"); * error_append_hint(errp, "Valid quarks are up, down, strange, " * "charm, top, bottom.\n"); ---> * This may require use of ERRP_GUARD(); more on that below. ;) > sending v2 Thanks! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-07 14:28 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-07 11:22 [PATCH] qapi: give available enum values in error string marcandre.lureau 2023-03-07 12:39 ` Markus Armbruster 2023-03-07 13:11 ` Marc-André Lureau 2023-03-07 14:28 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.