Re: [Qemu-devel] [RFC PATCH v4 06/30] qapi: make visit_type_size fallback to type_int

[mdroth <mdroth@linux.vnet.ibm.com>]

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
>