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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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?
> 

Yes, I had similar thoughts, but didn't spent too much time finding the best 
proposal, that wasn't my main goal in the series. 

Indeed, I would prefer that further improvements be done as follow-up. Or if 
you have a ready solution, I can squash it there. I can picture the conflicts 
with the next patch though... 

> [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.
> 

ok

> 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.
> 

ok