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

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Hi
> 
> - Original Message -
> > Marc-André Lureau  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 
> > > ---
> > [...]
> > >  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 / dgilb...@redhat.com / Manchester, UK

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

[Marc-André Lureau]

Hi

- Original Message -
> Marc-André Lureau  writes:
> 
> > This will help with the introduction of a new structure to handle
> > enum lookup.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> [...]
> >  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

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

[Markus Armbruster]

Marc-André Lureau  writes:

> This will help with the introduction of a new structure to handle
> enum lookup.
>
> Signed-off-by: Marc-André Lureau 
> ---
[...]
>  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.

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

[John Snow]

On 08/22/2017 09:22 AM, Marc-André Lureau wrote:
> This will help with the introduction of a new structure to handle
> enum lookup.
> 

Procedurally for the sake of review, it's a little odd to introduce the
function, deploy it, and then change it and update all callers.

content-wise, I'm really glad to have a function like this that we can
use universally.

ACK (6, 12)