Re: [Qemu-devel] [PATCH 5/6] add visitor for parsing hz[KMG] input string

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 5 Dec 2012 15:00:59 -0600
mdroth <mdroth@linux.vnet.ibm.com> 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