| Message ID | 1355175187-11470-2-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 '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(+) > [...]
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 >
On Fri, Dec 14, 2012 at 02:20:48PM +0100, Igor Mammedov wrote: > 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. 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 <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
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 75214e7..09c3b01 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -143,6 +143,13 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[], { } +static void qapi_dealloc_type_suffixed_int(Visitor *v, int64_t *obj, + const char *name, + const int suffix_factor, + Error **errp) +{ +} + Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) { return &v->visitor; @@ -170,6 +177,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.type_str = qapi_dealloc_type_str; v->visitor.type_number = qapi_dealloc_type_number; v->visitor.type_size = qapi_dealloc_type_size; + v->visitor.type_suffixed_int = qapi_dealloc_type_suffixed_int; QTAILQ_INIT(&v->stack); diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 7a82b63..9f6b8ae 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -311,3 +311,38 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[], g_free(enum_str); *obj = value; } + +/** + * visit_type_suffixed_int: + * @v: visitor used for accesing external representation of value + * @obj: object that stores internal value + * @name: name of the option + * @suffix_factor: multiplication factor of suffix 'K' + * @errp: object to store error code + * + * Converts option value represented by @v taking in account multiplication + * factor of a suffix character that might follow number. + * i.e. resulted *@obj = number * suffix. + * + * Where following suffixes are recognised: + * no suffix = 1 + * K = @suffix_factor + * M = K * @suffix_factor + * G = M * @suffix_factor + * T = G * @suffix_factor + * + * In case @v doesn't provide type_suffixed_int() parser, convertion + * fall-backs to suffix-less type_int64() parser. + */ +void visit_type_suffixed_int(Visitor *v, int64_t *obj, const char *name, + const int suffix_factor, Error **errp) +{ + if (error_is_set(errp)) { + return; + } + if (v->type_suffixed_int) { + v->type_suffixed_int(v, obj, name, suffix_factor, errp); + } else { + visit_type_int64(v, obj, name, errp); + } +} diff --git a/qapi/qapi-visit-core.h b/qapi/qapi-visit-core.h index 60aceda..05e9940 100644 --- a/qapi/qapi-visit-core.h +++ b/qapi/qapi-visit-core.h @@ -62,6 +62,8 @@ struct Visitor void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp); /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */ void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp); + void (*type_suffixed_int)(Visitor *v, int64_t *obj, const char *name, + const int uffix_factor, Error **errp); }; void visit_start_handle(Visitor *v, void **obj, const char *kind, @@ -91,5 +93,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp); void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp); void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp); void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp); +void visit_type_suffixed_int(Visitor *v, int64_t *obj, const char *name, + const int suffix_factor, Error **errp); #endif diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 497eb9a..9641bce 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -110,6 +110,30 @@ static void parse_start_optional(Visitor *v, bool *present, *present = true; } +static void parse_type_suffixed_int(Visitor *v, int64_t *obj, const char *name, + const int suffix_factor, Error **errp) +{ + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); + char *endp = (char *) siv->string; + long long val = 0; + + if (siv->string) { + /* TODO: presently strtosz_suffix_unit() is limited to [0..INT64_MAX] + * it should be fixed to handle [INT64_MIN..INT64_MAX], + * to cover whole int64_t range + */ + val = strtosz_suffix_unit(siv->string, &endp, + STRTOSZ_DEFSUFFIX_B, suffix_factor); + } + 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 +156,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_suffixed_int = parse_type_suffixed_int; v->string = str; return v;
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> --- 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(+)