On Fri, Dec 14, 2012 at 02:20:48PM +0100, Igor Mammedov wrote:
> On Wed, 12 Dec 2012 16:16:42 -0200
> Eduardo Habkost <ehabk...@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 <imamm...@redhat.com>
> >
> > Reviewed-by: Eduardo Habkost <ehabk...@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.
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 <afaer...@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
--
Eduardo
