[RFC,v4,06/30] qapi: make visit_type_size fallback to type_int

Currently visit_type_size checks if the visitor's type_size function pointer is
NULL. If not, it calls it, otherwise it calls v->type_uint64(). But neither of
these pointers are ever set. Fallback to calling v->type_int() in this third
(default) case.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 qapi/qapi-visit-core.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

Am 18.12.2012 13:41, schrieb Vasilis Liaskovitis:
> Currently visit_type_size checks if the visitor's type_size function pointer is
> NULL. If not, it calls it, otherwise it calls v->type_uint64(). But neither of
> these pointers are ever set. Fallback to calling v->type_int() in this third
> (default) case.
> 
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>

Is this patch still needed? I thought size -> int fallback was fixed
differently for the deallocation visitor...

Anyway, someone (Anthony?) recently suggested to drop the size visitor
completely in favor of string type (cf. frequency visitor discussion).

Regards,
Andreas

> ---
>  qapi/qapi-visit-core.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 7a82b63..497e693 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -236,8 +236,17 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
>  
>  void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>  {
> +    int64_t value;
>      if (!error_is_set(errp)) {
> -        (v->type_size ? v->type_size : v->type_uint64)(v, obj, name, errp);
> +        if (v->type_size) {
> +            v->type_size(v, obj, name, errp);
> +        } else if (v->type_uint64) {
> +            v->type_uint64(v, obj, name, errp);
> +        } else {
> +            value = *obj;
> +            v->type_int(v, &value, name, errp);
> +            *obj = value;
> +        }
>      }
>  }
>

On Wed, Jan 09, 2013 at 01:18:04AM +0100, Andreas Färber wrote:
> Am 18.12.2012 13:41, schrieb Vasilis Liaskovitis:
> > Currently visit_type_size checks if the visitor's type_size function pointer is
> > NULL. If not, it calls it, otherwise it calls v->type_uint64(). But neither of
> > these pointers are ever set. Fallback to calling v->type_int() in this third
> > (default) case.
> > 
> > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> 
> Is this patch still needed? I thought size -> int fallback was fixed
> differently for the deallocation visitor...

It was only fixed for the dealloc visitor since that was the only path
hit in 1.3. If we add a non-OptsVisitor user of visit_type_size() then
we'll trigger the bug this patch fixes.

I do have some comments on this patch though (below)

> 
> Anyway, someone (Anthony?) recently suggested to drop the size visitor
> completely in favor of string type (cf. frequency visitor discussion).

Hmm, in terms of generalizing size/freq visitors without adding code for
theoretical use cases (which I think was the deadlock) it seems to serve
that purpose. And it does make sense to handle special suffixes at the
end-points (qemu options and option users) rather than bake them into the
visitor interfaces (which get too numerous if we add them on a
case-by-case, and unwieldly if we try to generalize too much)

Though for qapi-generated visitors (as opposed to the open-coded ones like
the freq visitor) it does have the downside of requiring us to deserialize
into a string field before parsing/using it later, so we add some
unecessary fields. But I don't think that's a major downside.

At least, I would consider implementing the frequency visitor as a
string visitor should we decide to leave visit_type_size() as is.

> 
> Regards,
> Andreas
> 
> > ---
> >  qapi/qapi-visit-core.c |   11 ++++++++++-
> >  1 files changed, 10 insertions(+), 1 deletions(-)
> > 
> > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> > index 7a82b63..497e693 100644
> > --- a/qapi/qapi-visit-core.c
> > +++ b/qapi/qapi-visit-core.c
> > @@ -236,8 +236,17 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
> >  
> >  void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
> >  {
> > +    int64_t value;
> >      if (!error_is_set(errp)) {
> > -        (v->type_size ? v->type_size : v->type_uint64)(v, obj, name, errp);
> > +        if (v->type_size) {
> > +            v->type_size(v, obj, name, errp);
> > +        } else if (v->type_uint64) {
> > +            v->type_uint64(v, obj, name, errp);
> > +        } else {
> > +            value = *obj;
> > +            v->type_int(v, &value, name, errp);
> > +            *obj = value;
> > +        }

I'd recommend just doing:

  if (v->type_size) {
      v->type_size(v, obj, name, errp);
  } else {
      visit_type_uint64(v, obj, name, errp);
  }

visit_type_uint64() already handles the fallback to visit_type_int() so no
need to duplicate.

> >      }
> >  }
> >  
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>