Re: [PATCH] qapi: give available enum values in error string

[Markus Armbruster <armbru@redhat.com>]

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?