From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38733) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tsy9L-0006YA-Qn for qemu-devel@nongnu.org; Wed, 09 Jan 2013 11:05:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tsy9K-0001Ox-CZ for qemu-devel@nongnu.org; Wed, 09 Jan 2013 11:05:07 -0500 Received: from mail-ie0-f181.google.com ([209.85.223.181]:46885) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tsy9K-0001Nq-8o for qemu-devel@nongnu.org; Wed, 09 Jan 2013 11:05:06 -0500 Received: by mail-ie0-f181.google.com with SMTP id 16so2236635iea.40 for ; Wed, 09 Jan 2013 08:05:05 -0800 (PST) Sender: fluxion Date: Wed, 9 Jan 2013 10:00:25 -0600 From: mdroth Message-ID: <20130109160025.GB1543@vm> References: <1355834518-17989-1-git-send-email-vasilis.liaskovitis@profitbricks.com> <1355834518-17989-7-git-send-email-vasilis.liaskovitis@profitbricks.com> <50ECB73C.9090605@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <50ECB73C.9090605@suse.de> Subject: Re: [Qemu-devel] [RFC PATCH v4 06/30] qapi: make visit_type_size fallback to type_int List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: blauwirbel@gmail.com, pingfank@linux.vnet.ibm.com, gleb@redhat.com, stefanha@gmail.com, jbaron@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org, Vasilis Liaskovitis , kevin@koconnor.net, kraxel@redhat.com, Anthony Liguori 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 > > 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 >