From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44076) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TgiON-0003Lv-Dl for qemu-devel@nongnu.org; Thu, 06 Dec 2012 15:50:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TgiOM-0007fC-5o for qemu-devel@nongnu.org; Thu, 06 Dec 2012 15:49:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:11112) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TgiOL-0007f5-Ua for qemu-devel@nongnu.org; Thu, 06 Dec 2012 15:49:58 -0500 Date: Thu, 6 Dec 2012 21:49:49 +0100 From: Igor Mammedov Message-ID: <20121206214949.7e65dc9f@thinkpad.mammed.net> In-Reply-To: <20121205210058.GA8940@vm> References: <1354649683-9078-1-git-send-email-ehabkost@redhat.com> <1354649683-9078-6-git-send-email-ehabkost@redhat.com> <20121205174520.GA9133@vm> <20121205192150.GS4255@otherpad.lan.raisama.net> <20121205210058.GA8940@vm> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] add visitor for parsing hz[KMG] input string List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mdroth Cc: Don Slutz , Eduardo Habkost , Andreas =?UTF-8?B?RsOkcmJlcg==?= , qemu-devel@nongnu.org On Wed, 5 Dec 2012 15:00:59 -0600 mdroth wrote: > On Wed, Dec 05, 2012 at 05:21:50PM -0200, Eduardo Habkost wrote: > > On Wed, Dec 05, 2012 at 11:52:29AM -0600, mdroth wrote: > > > On Tue, Dec 04, 2012 at 05:34:42PM -0200, Eduardo Habkost wrote: > > [...] > > > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > > > > index 497eb9a..74fe395 100644 > > > > --- a/qapi/string-input-visitor.c > > > > +++ b/qapi/string-input-visitor.c > > > > @@ -110,6 +110,27 @@ static void parse_start_optional(Visitor *v, bool *present, > > > > *present = true; > > > > } > > > > > > > > +static void parse_type_freq(Visitor *v, int64_t *obj, const char *name, > > > > + Error **errp) > > > > +{ > > > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > > > + char *endp = (char *) siv->string; > > > > + long long val = 0; > > > > + > > > > + errno = 0; > > > > > > If this is for strtosz_suffix_unit(), it looks like this is already > > > handled internally and can be dropped. Relic from a previous version > > > that called strtod() directly maybe? > > > > > > If that's not the case, I think it should be fixed in the called function[s] > > > rather than for each caller. > > > > > > > + if (siv->string) { > > > > + val = strtosz_suffix_unit(siv->string, &endp, > > > > + STRTOSZ_DEFSUFFIX_B, 1000); > > > > > > Specifying 1000 as the unit size will make "1M" == 1000^2 as opposed to > > > 1024^2. Is this intentional? > > > > I don't know if this is a good idea for a generalx-use visitor, but this is the > > current behavior of "-cpu ...,tsc_freq=1M", that we need to keep for > > compatibility, somehow. > > > > > > > > > + } > > > > + if (!siv->string || val == -1 || *endp) { > > > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > > > + "a value representable as a non-negative int64"); > > > > + return; > > > > + } > > > > + > > > > + *obj = val; > > > > +} > > > > + > > > > Visitor *string_input_get_visitor(StringInputVisitor *v) > > > > { > > > > return &v->visitor; > > > > @@ -132,6 +153,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) > > > > v->visitor.type_str = parse_type_str; > > > > v->visitor.type_number = parse_type_number; > > > > v->visitor.start_optional = parse_start_optional; > > > > + v->visitor.type_freq = parse_type_freq; > > > > > > This seems applicable for stuff like -m 1G and potentionally some other > > > properties. We can make it generic later, but if we do end up re-spinning > > > consider something like ->type_unit_suffixed_int(). But I'm not against > > > leaving as is for now since I can't think of a better name for it atm :) > > > > I thought the visitor was going to support things like "1GHz", but if it's just > > a "suffixed int" with no unit, the name could be changed, I guess. > > > > But we still have the 1000 vs 1024 problem. On the one hand, it would be > > interesting to make make it consistent and use the same base everywhere. > > On the other hand, I assume we have different command-line options using > > different bases and we'll need to keep compatibility. > > If we were to map it to a QAPI schema definition at some point, I'd > imagine it looking something like: > > { 'field_name': { 'type': 'suffixed_int', 'unit': 1000 } } > > with 'unit' defaulting to 1024 if unspecified. (I have some patches > floating around as part of the QIDL series for doing more descriptive > QAPI definitions) > > > > > Must all visitor functions have the > > "function(Visitor *v, obj, const char *name, Error **errp)" signature, > > or can we add additional type-specific arguments? (so we could tell > > the visitor if the default base should be 1000 or 1024) > > Visitor functions don't necessarilly have to follow the same basic > signature, so it would be okay to declare it with an extra 'unit' param > and pass that in. We could still hide this behind a visit_type_freq() > wrapper in open-coded visitors as well, I'd just prefer to make the bits > we add to qapi-visit-core.c more general where possible to keep unit > tests and code generation stuff somewhat sane for the core api. Lets try to do it this way and if people don't like idea fall back to fixed type_freq. I'll post patches in a momment > > > > > -- > > Eduardo > > -- Regards, Igor