[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] qapi: give available enum values as error hint
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2] qapi: give available enum values as error hint |
Date: |
Tue, 07 Mar 2023 15:40:32 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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=: Parameter 'gl' does not accept
> value ''
> Available: 'off', 'on', 'core', 'es'
I think "Acceptable values are ..." would go more nicely with "does not
accept". What do you think?
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/qapi-visit-core.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6c13510a2b..45e39ff533 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -392,9 +392,25 @@ static bool output_type_enum(Visitor *v, const char
> *name, int *obj,
> return visit_type_str(v, name, &enum_str, errp);
> }
>
> +static void error_append_qapi_enum_hint(Error **errp, const QEnumLookup
> *lookup)
error_append_hint() has Error *const *errp to make it obvious[*] that
the function does *not* set an error (by assigning to *errp). We need
to do the same here.
See also commit 49fbc7236dd (error: make Error **errp const where it is
appropriate).
> +{
> + int i;
> +
> + error_append_hint(errp, "Available: ");
> + for (i = 0; i < lookup->size; i++) {
> + error_append_hint(errp, "'%s'", lookup->array[i]);
> + if (i + 1 < lookup->size) {
> + error_append_hint(errp, ", ");
> + }
> + }
> + error_append_hint(errp, "\n");
> +}
> +
> +
> static bool input_type_enum(Visitor *v, const char *name, int *obj,
> const QEnumLookup *lookup, Error **errp)
> {
> + ERRP_GUARD();
> int64_t value;
> g_autofree char *enum_str = NULL;
>
> @@ -406,6 +422,7 @@ static bool input_type_enum(Visitor *v, const char *name,
> int *obj,
> if (value < 0) {
> error_setg(errp, "Parameter '%s' does not accept value '%s'",
> name ? name : "null", enum_str);
> + error_append_qapi_enum_hint(errp, lookup);
> return false;
> }
Both of my review comments could be addressed without a respin. Up to
you!
[*] Obvious to C language lawyers, I guess. And also to Coccinelle
scripts.