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

[Dr. David Alan Gilbert]

* Marc-André Lureau (address@hidden) wrote:
> Hi
> 
> ----- Original Message -----
> > Marc-André Lureau <address@hidden> writes:
> > 
> > > This will help with the introduction of a new structure to handle
> > > enum lookup.

It would be good to make that comment explain why it's necessary.

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

If you did it as a separate patch it would have to be another
huge patch changing lots of areas.
I think the use of the macro to keep it simple should be in this one;
you can add the type check change later.

Dave

> > [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
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK