qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 06/54] qapi: introduce qapi_enum_lookup()


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2 06/54] qapi: introduce qapi_enum_lookup()
Date: Wed, 23 Aug 2017 10:02:12 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Marc-André Lureau <address@hidden> writes:

> This will help with the introduction of a new structure to handle
> enum lookup.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
[...]
>  45 files changed, 206 insertions(+), 122 deletions(-)

Hmm.

> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 7436ed815c..60733b6a80 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -11,6 +11,8 @@
>  #ifndef QAPI_UTIL_H
>  #define QAPI_UTIL_H
>  
> +const char *qapi_enum_lookup(const char * const lookup[], int val);
> +
>  int qapi_enum_parse(const char * const lookup[], const char *buf,
>                      int max, int def, Error **errp);
>  
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 4606b73849..c4f795475c 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -13,6 +13,7 @@
>  #include "sysemu/hostmem.h"
>  #include "hw/boards.h"
>  #include "qapi/error.h"
> +#include "qapi/util.h"
>  #include "qapi/visitor.h"
>  #include "qapi-types.h"
>  #include "qapi-visit.h"
> @@ -304,7 +305,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>              return;
>          } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
>              error_setg(errp, "host-nodes must be set for policy %s",
> -                       HostMemPolicy_lookup[backend->policy]);
> +                qapi_enum_lookup(HostMemPolicy_lookup, backend->policy));
>              return;
>          }
>  

Lookup becomes even more verbose.

Could we claw back some readability with macros?  Say in addition to

    typedef enum FOO {
        ...
    } FOO;

    extern const char *const FOO_lookup[];

generate

    #define FOO_str(val) qapi_enum_lookup(FOO_lookup, (val))

Needs a matching qapi-code-gen.txt update.

With that, this patch hunk would become

               error_setg(errp, "host-nodes must be set for policy %s",
  -                       HostMemPolicy_lookup[backend->policy]);
  +                       HostMemPolicy_str(backend->policy);

Perhaps we could even throw in some type checking:

    #define FOO_str(val) (type_check(typeof((val)), FOO) \
                          + qapi_enum_lookup(FOO_lookup, (val)))

What do you think?  Want me to explore a fixup patch you could squash
in?

[Skipping lots of mechanical changes...]

I think you missed test-qobject-input-visitor.c and
string-input-visitor.c.

In test-qobject-input-visitor.c's test_visitor_in_enum():

        v = visitor_input_test_init(data, "%s", EnumOne_lookup[i]);

You update it in PATCH 12, but the code only works as long as EnumOne
has no holes.  Mapping the enum to string like we do everywhere else in
this patch would be cleaner.

The loop control also subscripts EnumOne_lookup[i].  You take care of
that one in PATCH 12.  That's okay.

Same for test-string-input-visitor.c's test_visitor_in_enum().

There's one more in test_native_list_integer_helper():

    g_string_append_printf(gstr_union,  "{ 'type': '%s', 'data': [ %s ] }",
                           UserDefNativeListUnionKind_lookup[kind],
                           gstr_list->str);

Same story.

The patch doesn't touch the _lookup[] subscripts you're going to replace
by qapi_enum_parse() in PATCH 07-11.  Understand, but I'd reorder the
patches: first replace by qapi_enum_parse(), because DRY (no need to
explain more at that point).  Then get rid of all the remaining
subscripting into _lookup[], i.e. this patch (explain it helps the next
one), then PATCH 12.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]