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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 12 Dec 2012 16:16:42 -0200
Eduardo Habkost <ehabkost@redhat.com> 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 <imammedo@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> 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.
  
> '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 <afaerber@suse.de>
> >   - 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