Re: [Qemu-devel] [PATCH 1/2] add visitor for parsing int[KMGT] input string

[Eduardo Habkost]

On Fri, Dec 14, 2012 at 02:20:48PM +0100, Igor Mammedov wrote:
> On Wed, 12 Dec 2012 16:16:42 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Mon, Dec 10, 2012 at 10:33:06PM +0100, Igor Mammedov wrote:
> > > Caller of visit_type_suffixed_int() have to specify
> > > value of 'K' suffix using suffix_factor argument.
> > > Example of selecting suffix_factor value:
> > >  * Kbytes: 1024
> > >  * Khz: 1000
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > 
> > Reviewed-by: Eduardo Habkost <address@hidden>
> > 
> > 
> > I wonder if we could later introduce a visit_type_frequency() function
> > that simply calls visit_type_suffixed_int(). This would allow us to use
> > a 'frequency' type on QAPI, like the existing 'size' type we already
> > have.
> > 
> > I suggest having explicitly distinct types on QAPI because the 'size'
> > type probably won't abort (and maybe it _can't_ abort, to keep
> > compatibility) in case it finds a "100MB" string. Likewise, the
> It won't accept MB with current code, but we could probably pass
> something like custom suffix table {KHz => 1000, MHz=>1000000, ...}  instead
> of unit for variables that accept frequency, and a corresponding table for
> sizes and whatever else if needed. Than we could use only
> visit_type_suffixed_int() and avoid creating an extra boiler code for every
> kind of units that might be needed in future.

The above make sense, but I wasn't worrying about how it's done
internally when calling/configuring the parsing code. My only point is
about the type definitions in QAPI. I believe that having this in QAPI:
  'type':'frequency'
  'type':'size'
looks better than:
  'type': 'suffixed_int', 'suffix_factor': 1000, 'unit': 'Hz'
  'type': 'suffixed_int', 'suffix_factor': 1024, 'unit': 'B'
or, even worse:
  'type': 'suffixed_int', 'suffix_table': { 'K': 1000, 'KHz': 1000, 'M': 
1000000, 'MHz': 10000000, ... }
  'type': 'suffixed_int', 'suffix_table': { 'K': 1024, 'KB': 1024, 'M': 
1048576, 'MB': 1048576, ... }
or:
  'type': 'suffixed_int', 'suffix_table': { 'K': 1000, 'M': 1000000, ... }, 
'unit': 'Hz'
  'type': 'suffixed_int', 'suffix_table': { 'K': 1024, 'M': 1048576, ... }, 
'unit': 'B'


>   
> > 'frequency' type wouldn't abort in case it finds a "100MHz" string.
> > 
> > With separate types, we could also make the 'frequency' type _not_
> > accept "100B" as a valid string (strtosz_suffix_unit() accepts "B" as a
> > valid suffix, today).
> > 
> > 
> > > ---
> > >  v3:
> > >   - Fix errp check. Spotted-By: Andreas Färber <address@hidden>
> > >   - s/type_unit_suffixed_int/type_suffixed_int/
> > >   - use 'suffix_factor' instead of 'unit'
> > >   - document visit_type_suffixed_int()
> > >   - add comment on current impl. limitation
> > >  v2:
> > >   - convert type_freq to type_unit_suffixed_int.
> > >   - provide qapi_dealloc_type_unit_suffixed_int() impl.
> > > ---
> > >  qapi/qapi-dealloc-visitor.c |  8 ++++++++
> > >  qapi/qapi-visit-core.c      | 35 +++++++++++++++++++++++++++++++++++
> > >  qapi/qapi-visit-core.h      |  4 ++++
> > >  qapi/string-input-visitor.c | 25 +++++++++++++++++++++++++
> > >  4 files changed, 72 insertions(+)
> > > 
> > [...]
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo