Re: [Qemu-devel] [PATCH 1/4] visitor: Add 'supported_qtypes' parameter to visit_start_alternate()

[Eduardo Habkost]

On Tue, May 02, 2017 at 04:29:32PM -0500, Eric Blake wrote:
> On 05/02/2017 03:31 PM, Eduardo Habkost wrote:
> > This will allow visitors to make decisions based on the supported qtypes
> > of a given alternate type. The new parameter can replace the old
> > 'promote_int' argument, as qobject-input-visitor can simply check if
> > QTYPE_QINT is set in supported_qtypes.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> 
> > @@ -416,7 +417,7 @@ void visit_end_list(Visitor *v, void **list);
> >   */
> >  void visit_start_alternate(Visitor *v, const char *name,
> >                             GenericAlternate **obj, size_t size,
> > -                           bool promote_int, Error **errp);
> > +                           unsigned long supported_qtypes, Error **errp);
> 
> Why unsigned long (which is platform-dependent in size)? At the moment,
> even unsigned char happens to be long enough, although I probably would
> have used uint32_t.
> 
> Oh, I see, it's because you use the BIT() macros from bitops.h, which
> are hardcoded to unsigned long.

Yep. But I don't see a problem with using uint32_t or a simple
int.

> 
> > +++ b/scripts/qapi-visit.py
> > @@ -161,20 +161,21 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> > *name, %(c_name)s *obj, Error
> >  
> >  
> >  def gen_visit_alternate(name, variants):
> > -    promote_int = 'true'
> > +    qtypes = ['BIT(%s)' % (var.type.alternate_qtype())
> > +              for var in variants.variants]
> > +    supported_qtypes = '|'.join(qtypes)
> 
> Do you want ' | '.join(qtypes), so that at least the generated code
> still follows recommended operator spacing? (The line is long no matter
> what, though, and that's not worth worrying about.)

I can do that in v2.

> 
> >      ret = ''
> > -    for var in variants.variants:
> > -        if var.type.alternate_qtype() == 'QTYPE_QINT':
> > -            promote_int = 'false'
> >  
> >      ret += mcgen('''
> >  
> >  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
> > Error **errp)
> >  {
> >      Error *err = NULL;
> > +    unsigned long supported_qtypes = %(supported_qtypes)s;
> >  
> > +    assert(QTYPE__MAX < BITS_PER_LONG);
> 
> Do we really have to generate a separate copy of this assert in every
> generated function?  Especially when we know it is true by construction,
> that seems like a lot.  Having the assertion once in a .c file rather
> than generated in multiple functions might be acceptable, though.

I will probably do this as a single QEMU_BUILD_BUG_ON near
visit_start_alternate().

> 
> > +++ b/qapi/qobject-input-visitor.c
> 
> > @@ -349,7 +351,7 @@ static void qobject_input_start_alternate(Visitor *v, 
> > const char *name,
> >      }
> >      *obj = g_malloc0(size);
> >      (*obj)->type = qobject_type(qobj);
> > -    if (promote_int && (*obj)->type == QTYPE_QINT) {
> > +    if (!(supported_qtypes & BIT(QTYPE_QINT)) && (*obj)->type == 
> > QTYPE_QINT) {
> 
> Experimenting, does this read any better:
> 
> if (!extract32(supported_qtypes, QTYPE_QINT, 1) && ...
> 
> which would be another argument for uint32_t instead of unsigned long in
> the signature.

I am more used to see this written as "if (s & (1UL << n))" than
as "if (extract32(s, n, 1))", so I'm not sure.

I see some extract32(..., ..., 1) cases in the tree, so it's not
as unusual as I thought. I will probably give it a try.

> 
> The idea makes sense, but I'm still not necessarily sold on using a long.

Thanks!

-- 
Eduardo