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(-)
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
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?
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
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!
© 2016 - 2024 Red Hat, Inc.