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