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

[Eric Blake]

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.

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

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

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

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature