| Message ID | 1354828325-16568-2-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 06, 2012 at 10:12:04PM +0100, Igor Mammedov wrote: > Caller of visit_type_unit_suffixed_int() will have to specify > value of 'K' suffix via unit argument. > For Kbytes it's 1024, for Khz it's 1000. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > v2: > - convert type_freq to type_unit_suffixed_int. > - provide qapi_dealloc_type_unit_suffixed_int() impl. > --- > qapi/qapi-dealloc-visitor.c | 7 +++++++ > qapi/qapi-visit-core.c | 13 +++++++++++++ > qapi/qapi-visit-core.h | 8 ++++++++ > qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ > 4 files changed, 50 insertions(+) > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 75214e7..57e662c 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[], > { > } > > +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, > + const char *name, > + const int unit, Error **errp) > +{ > +} > + > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > { > return &v->visitor; > @@ -170,6 +176,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_unit_suffixed_int = qapi_dealloc_type_unit_suffixed_int; > > QTAILQ_INIT(&v->stack); > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 7a82b63..dcbc1a9 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[], > g_free(enum_str); > *obj = value; > } > + > +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp) > +{ > + if (!error_is_set(errp)) { > + return; > + } > + if (v->type_unit_suffixed_int) { > + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 > --- a/qapi/qapi-visit-core.h > +++ b/qapi/qapi-visit-core.h > @@ -62,6 +62,12 @@ 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); > + /* > + * visit_unit_suffixed_int() falls back to (*type_int64)() > + * if type_unit_suffixed_int is unset > + */ > + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp); > }; > > void visit_start_handle(Visitor *v, void **obj, const char *kind, > @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp); > > #endif > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, > + const char *name, const int unit, > + Error **errp) > +{ > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > + char *endp = (char *) siv->string; > + long long val = 0; > + > + if (siv->string) { > + val = strtosz_suffix_unit(siv->string, &endp, > + STRTOSZ_DEFSUFFIX_B, unit); > + } > + 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_unit_suffixed_int = parse_type_unit_suffixed_int; > > v->string = str; > return v; > -- > 1.7.11.7 >
On Thu, Dec 06, 2012 at 10:12:04PM +0100, Igor Mammedov wrote: > Caller of visit_type_unit_suffixed_int() will have to specify > value of 'K' suffix via unit argument. > For Kbytes it's 1024, for Khz it's 1000. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > - convert type_freq to type_unit_suffixed_int. > - provide qapi_dealloc_type_unit_suffixed_int() impl. > --- > qapi/qapi-dealloc-visitor.c | 7 +++++++ > qapi/qapi-visit-core.c | 13 +++++++++++++ > qapi/qapi-visit-core.h | 8 ++++++++ > qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ > 4 files changed, 50 insertions(+) > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 75214e7..57e662c 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[], > { > } > > +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, > + const char *name, > + const int unit, Error **errp) > +{ > +} > + > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > { > return &v->visitor; > @@ -170,6 +176,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_unit_suffixed_int = qapi_dealloc_type_unit_suffixed_int; > > QTAILQ_INIT(&v->stack); > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 7a82b63..dcbc1a9 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[], > g_free(enum_str); > *obj = value; > } > + > +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp) > +{ > + if (!error_is_set(errp)) { > + return; > + } > + if (v->type_unit_suffixed_int) { > + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 > --- a/qapi/qapi-visit-core.h > +++ b/qapi/qapi-visit-core.h > @@ -62,6 +62,12 @@ 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); > + /* > + * visit_unit_suffixed_int() falls back to (*type_int64)() > + * if type_unit_suffixed_int is unset > + */ > + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp); > }; > > void visit_start_handle(Visitor *v, void **obj, const char *kind, > @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp); > > #endif > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, > + const char *name, const int unit, > + Error **errp) > +{ > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > + char *endp = (char *) siv->string; > + long long val = 0; > + > + if (siv->string) { > + val = strtosz_suffix_unit(siv->string, &endp, > + STRTOSZ_DEFSUFFIX_B, unit); > + } I just noticed that strtosz_suffix_unit() is not as generic as I expected: it interprets "100B" as "100", but the value we're reading may be completely unrelated to byte counts. It would be nice to fix this later, but the current interface/behavior doesn't seem to break anything (as the k/M/G/T cases seem to work properly), and it even matches the current frequency parsing code on target-i386/cpu.c, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + 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_unit_suffixed_int = parse_type_unit_suffixed_int; > > v->string = str; > return v; > -- > 1.7.11.7 >
Am 06.12.2012 22:12, schrieb Igor Mammedov: > Caller of visit_type_unit_suffixed_int() will have to specify > value of 'K' suffix via unit argument. > For Kbytes it's 1024, for Khz it's 1000. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > - convert type_freq to type_unit_suffixed_int. > - provide qapi_dealloc_type_unit_suffixed_int() impl. > --- > qapi/qapi-dealloc-visitor.c | 7 +++++++ > qapi/qapi-visit-core.c | 13 +++++++++++++ > qapi/qapi-visit-core.h | 8 ++++++++ > qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ > 4 files changed, 50 insertions(+) > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 75214e7..57e662c 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[], > { > } > > +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, > + const char *name, > + const int unit, Error **errp) > +{ > +} > + > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > { > return &v->visitor; > @@ -170,6 +176,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_unit_suffixed_int = qapi_dealloc_type_unit_suffixed_int; > > QTAILQ_INIT(&v->stack); > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 7a82b63..dcbc1a9 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[], > g_free(enum_str); > *obj = value; > } > + > +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp) > +{ > + if (!error_is_set(errp)) { if (error_is_set(errp)) { > + return; > + } > + if (v->type_unit_suffixed_int) { > + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 > --- a/qapi/qapi-visit-core.h > +++ b/qapi/qapi-visit-core.h > @@ -62,6 +62,12 @@ 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); > + /* > + * visit_unit_suffixed_int() falls back to (*type_int64)() > + * if type_unit_suffixed_int is unset > + */ Indentation is one off. > + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp); Are we expecting differently suffixed ints? Otherwise we could optionally shorten to type_suffixed_int (but that probably still doesn't fit within one comment line ;)). > }; > > void visit_start_handle(Visitor *v, void **obj, const char *kind, > @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > + const int unit, Error **errp); > > #endif > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, > + const char *name, const int unit, > + Error **errp) > +{ > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > + char *endp = (char *) siv->string; > + long long val = 0; > + > + if (siv->string) { > + val = strtosz_suffix_unit(siv->string, &endp, > + STRTOSZ_DEFSUFFIX_B, unit); > + } > + if (!siv->string || val == -1 || *endp) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > + "a value representable as a non-negative int64"); Weird indentation remaining, looks as if we could align with errp within 80 chars. However, I wonder if "unit" is the (physically etc.) correct term here? Isn't the "unit" Hz / byte / ... and 1000 more of a conversion factor or something? At least that's the way I've seen unit used in the API of another project, passing an enum of Hertz, gram, meter/second, etc. Andreas > + 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_unit_suffixed_int = parse_type_unit_suffixed_int; > > v->string = str; > return v; >
On Fri, Dec 07, 2012 at 07:57:35PM +0100, Andreas Färber wrote: [...] > > +static void parse_type_unit_suffixed_int(Visitor *v, int64_t *obj, > > + const char *name, const int unit, > > + Error **errp) > > +{ > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > + char *endp = (char *) siv->string; > > + long long val = 0; > > + > > + if (siv->string) { > > + val = strtosz_suffix_unit(siv->string, &endp, > > + STRTOSZ_DEFSUFFIX_B, unit); > > + } > > + if (!siv->string || val == -1 || *endp) { > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > + "a value representable as a non-negative int64"); > > Weird indentation remaining, looks as if we could align with errp within > 80 chars. > > However, I wonder if "unit" is the (physically etc.) correct term here? > Isn't the "unit" Hz / byte / ... and 1000 more of a conversion factor or > something? At least that's the way I've seen unit used in the API of > another project, passing an enum of Hertz, gram, meter/second, etc. I also think that calling it "unit" is confusing. Strictly speaking, the "unit" we're dealing with is Hz (and the visitor simply don't care about unit, it only looks for "unit prefixes"). Can't we call the 1000/1024 parameter "base" or "factor", instead?
On Fri, 07 Dec 2012 19:57:35 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 06.12.2012 22:12, schrieb Igor Mammedov: > > Caller of visit_type_unit_suffixed_int() will have to specify > > value of 'K' suffix via unit argument. > > For Kbytes it's 1024, for Khz it's 1000. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v2: > > - convert type_freq to type_unit_suffixed_int. > > - provide qapi_dealloc_type_unit_suffixed_int() impl. > > --- > > qapi/qapi-dealloc-visitor.c | 7 +++++++ > > qapi/qapi-visit-core.c | 13 +++++++++++++ > > qapi/qapi-visit-core.h | 8 ++++++++ > > qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ > > 4 files changed, 50 insertions(+) > > > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > > index 75214e7..57e662c 100644 > > --- a/qapi/qapi-dealloc-visitor.c > > +++ b/qapi/qapi-dealloc-visitor.c > > @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int > > *obj, const char *strings[], { > > } > > > > +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, > > + const char *name, > > + const int unit, Error > > **errp) +{ > > +} > > + > > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > > { > > return &v->visitor; > > @@ -170,6 +176,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_unit_suffixed_int = > > qapi_dealloc_type_unit_suffixed_int; > > QTAILQ_INIT(&v->stack); > > > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > > index 7a82b63..dcbc1a9 100644 > > --- a/qapi/qapi-visit-core.c > > +++ b/qapi/qapi-visit-core.c > > @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const > > char *strings[], g_free(enum_str); > > *obj = value; > > } > > + > > +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char > > *name, > > + const int unit, Error **errp) > > +{ > > + if (!error_is_set(errp)) { > > if (error_is_set(errp)) { Thanks, I'll fix it. > > + return; > > + } > > + if (v->type_unit_suffixed_int) { > > + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 > > --- a/qapi/qapi-visit-core.h > > +++ b/qapi/qapi-visit-core.h > > @@ -62,6 +62,12 @@ 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); > > + /* > > + * visit_unit_suffixed_int() falls back to (*type_int64)() > > + * if type_unit_suffixed_int is unset > > + */ > > Indentation is one off. ditto > > > + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char > > *name, > > + const int unit, Error **errp); > > Are we expecting differently suffixed ints? Otherwise we could > optionally shorten to type_suffixed_int (but that probably still doesn't > fit within one comment line ;)). Not with current implementation. I'll shorten it as you've suggested. > > > }; > > > > void visit_start_handle(Visitor *v, void **obj, const char *kind, > > @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > > + const int unit, Error **errp); > > > > #endif > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > > index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, > > + const char *name, const int > > unit, > > + Error **errp) > > +{ > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > + char *endp = (char *) siv->string; > > + long long val = 0; > > + > > + if (siv->string) { > > + val = strtosz_suffix_unit(siv->string, &endp, > > + STRTOSZ_DEFSUFFIX_B, unit); > > + } > > + if (!siv->string || val == -1 || *endp) { > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > + "a value representable as a non-negative int64"); > > Weird indentation remaining, looks as if we could align with errp within > 80 chars. Thanks, I'll fix it. > > However, I wonder if "unit" is the (physically etc.) correct term here? > Isn't the "unit" Hz / byte / ... and 1000 more of a conversion factor or > something? At least that's the way I've seen unit used in the API of > another project, passing an enum of Hertz, gram, meter/second, etc. If we are to generalize it to integer than units might not make much sense, they could be anything. Perhaps 'suffix_factor' would be descriptive enough + adding documentation comment to the visitor. > > Andreas > > > + 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_unit_suffixed_int = parse_type_unit_suffixed_int; > > > > v->string = str; > > return v; > > > >
On Mon, Dec 10, 2012 at 05:01:38PM +0100, Igor Mammedov wrote: > On Fri, 07 Dec 2012 19:57:35 +0100 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 06.12.2012 22:12, schrieb Igor Mammedov: > > > Caller of visit_type_unit_suffixed_int() will have to specify > > > value of 'K' suffix via unit argument. > > > For Kbytes it's 1024, for Khz it's 1000. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > v2: > > > - convert type_freq to type_unit_suffixed_int. > > > - provide qapi_dealloc_type_unit_suffixed_int() impl. > > > --- > > > qapi/qapi-dealloc-visitor.c | 7 +++++++ > > > qapi/qapi-visit-core.c | 13 +++++++++++++ > > > qapi/qapi-visit-core.h | 8 ++++++++ > > > qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ > > > 4 files changed, 50 insertions(+) > > > > > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > > > index 75214e7..57e662c 100644 > > > --- a/qapi/qapi-dealloc-visitor.c > > > +++ b/qapi/qapi-dealloc-visitor.c > > > @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int > > > *obj, const char *strings[], { > > > } > > > > > > +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, > > > + const char *name, > > > + const int unit, Error > > > **errp) +{ > > > +} > > > + > > > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > > > { > > > return &v->visitor; > > > @@ -170,6 +176,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_unit_suffixed_int = > > > qapi_dealloc_type_unit_suffixed_int; > > > QTAILQ_INIT(&v->stack); > > > > > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > > > index 7a82b63..dcbc1a9 100644 > > > --- a/qapi/qapi-visit-core.c > > > +++ b/qapi/qapi-visit-core.c > > > @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const > > > char *strings[], g_free(enum_str); > > > *obj = value; > > > } > > > + > > > +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char > > > *name, > > > + const int unit, Error **errp) > > > +{ > > > + if (!error_is_set(errp)) { > > > > if (error_is_set(errp)) { > Thanks, I'll fix it. > > > > + return; > > > + } > > > + if (v->type_unit_suffixed_int) { > > > + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 > > > --- a/qapi/qapi-visit-core.h > > > +++ b/qapi/qapi-visit-core.h > > > @@ -62,6 +62,12 @@ 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); > > > + /* > > > + * visit_unit_suffixed_int() falls back to (*type_int64)() > > > + * if type_unit_suffixed_int is unset > > > + */ > > > > Indentation is one off. > ditto > > > > > > + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char > > > *name, > > > + const int unit, Error **errp); > > > > Are we expecting differently suffixed ints? Otherwise we could > > optionally shorten to type_suffixed_int (but that probably still doesn't > > fit within one comment line ;)). > Not with current implementation. I'll shorten it as you've suggested. > > > > > > }; > > > > > > void visit_start_handle(Visitor *v, void **obj, const char *kind, > > > @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > > > + const int unit, Error **errp); > > > > > > #endif > > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > > > index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, > > > + const char *name, const int > > > unit, > > > + Error **errp) > > > +{ > > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > > + char *endp = (char *) siv->string; > > > + long long val = 0; > > > + > > > + if (siv->string) { > > > + val = strtosz_suffix_unit(siv->string, &endp, > > > + STRTOSZ_DEFSUFFIX_B, unit); > > > + } > > > + if (!siv->string || val == -1 || *endp) { > > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > > + "a value representable as a non-negative int64"); > > > > Weird indentation remaining, looks as if we could align with errp within > > 80 chars. > Thanks, I'll fix it. > > > > > However, I wonder if "unit" is the (physically etc.) correct term here? > > Isn't the "unit" Hz / byte / ... and 1000 more of a conversion factor or > > something? At least that's the way I've seen unit used in the API of > > another project, passing an enum of Hertz, gram, meter/second, etc. > If we are to generalize it to integer than units might not make much sense, > they could be anything. Perhaps 'suffix_factor' would be descriptive enough > + adding documentation comment to the visitor. The real distinction I think is base=2 vs. base=10, but that might cause confusion WRT to how the numberical value should be intepreted (10==2 vs. 10==10), so maybe suffix_base==2|10, or suffix_factor==1000/1024 as you suggested. I think I'd prefer the former but either works for me. > > > > > Andreas > > > > > + 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_unit_suffixed_int = parse_type_unit_suffixed_int; > > > > > > v->string = str; > > > return v; > > > > > > > >
On Mon, 10 Dec 2012 11:27:46 -0600 mdroth <mdroth@linux.vnet.ibm.com> wrote: > On Mon, Dec 10, 2012 at 05:01:38PM +0100, Igor Mammedov wrote: > > On Fri, 07 Dec 2012 19:57:35 +0100 > > Andreas Färber <afaerber@suse.de> wrote: > > > > > Am 06.12.2012 22:12, schrieb Igor Mammedov: > > > > Caller of visit_type_unit_suffixed_int() will have to specify > > > > value of 'K' suffix via unit argument. > > > > For Kbytes it's 1024, for Khz it's 1000. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > v2: > > > > - convert type_freq to type_unit_suffixed_int. > > > > - provide qapi_dealloc_type_unit_suffixed_int() impl. > > > > --- > > > > qapi/qapi-dealloc-visitor.c | 7 +++++++ > > > > qapi/qapi-visit-core.c | 13 +++++++++++++ > > > > qapi/qapi-visit-core.h | 8 ++++++++ > > > > qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ > > > > 4 files changed, 50 insertions(+) > > > > > > > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > > > > index 75214e7..57e662c 100644 > > > > --- a/qapi/qapi-dealloc-visitor.c > > > > +++ b/qapi/qapi-dealloc-visitor.c > > > > @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int > > > > *obj, const char *strings[], { > > > > } > > > > > > > > +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, > > > > + const char *name, > > > > + const int unit, Error > > > > **errp) +{ > > > > +} > > > > + > > > > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > > > > { > > > > return &v->visitor; > > > > @@ -170,6 +176,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_unit_suffixed_int = > > > > qapi_dealloc_type_unit_suffixed_int; > > > > QTAILQ_INIT(&v->stack); > > > > > > > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > > > > index 7a82b63..dcbc1a9 100644 > > > > --- a/qapi/qapi-visit-core.c > > > > +++ b/qapi/qapi-visit-core.c > > > > @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const > > > > char *strings[], g_free(enum_str); > > > > *obj = value; > > > > } > > > > + > > > > +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char > > > > *name, > > > > + const int unit, Error **errp) > > > > +{ > > > > + if (!error_is_set(errp)) { > > > > > > if (error_is_set(errp)) { > > Thanks, I'll fix it. > > > > > > + return; > > > > + } > > > > + if (v->type_unit_suffixed_int) { > > > > + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 > > > > --- a/qapi/qapi-visit-core.h > > > > +++ b/qapi/qapi-visit-core.h > > > > @@ -62,6 +62,12 @@ 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); > > > > + /* > > > > + * visit_unit_suffixed_int() falls back to (*type_int64)() > > > > + * if type_unit_suffixed_int is unset > > > > + */ > > > > > > Indentation is one off. > > ditto > > > > > > > > > + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char > > > > *name, > > > > + const int unit, Error **errp); > > > > > > Are we expecting differently suffixed ints? Otherwise we could > > > optionally shorten to type_suffixed_int (but that probably still doesn't > > > fit within one comment line ;)). > > Not with current implementation. I'll shorten it as you've suggested. > > > > > > > > > }; > > > > > > > > void visit_start_handle(Visitor *v, void **obj, const char *kind, > > > > @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, > > > > + const int unit, Error **errp); > > > > > > > > #endif > > > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > > > > index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, > > > > + const char *name, const int > > > > unit, > > > > + Error **errp) > > > > +{ > > > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > > > + char *endp = (char *) siv->string; > > > > + long long val = 0; > > > > + > > > > + if (siv->string) { > > > > + val = strtosz_suffix_unit(siv->string, &endp, > > > > + STRTOSZ_DEFSUFFIX_B, unit); > > > > + } > > > > + if (!siv->string || val == -1 || *endp) { > > > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > > > + "a value representable as a non-negative int64"); > > > > > > Weird indentation remaining, looks as if we could align with errp within > > > 80 chars. > > Thanks, I'll fix it. > > > > > > > > However, I wonder if "unit" is the (physically etc.) correct term here? > > > Isn't the "unit" Hz / byte / ... and 1000 more of a conversion factor or > > > something? At least that's the way I've seen unit used in the API of > > > another project, passing an enum of Hertz, gram, meter/second, etc. > > If we are to generalize it to integer than units might not make much sense, > > they could be anything. Perhaps 'suffix_factor' would be descriptive enough > > + adding documentation comment to the visitor. > > The real distinction I think is base=2 vs. base=10, but that might cause > confusion WRT to how the numberical value should be intepreted (10==2 > vs. 10==10), so maybe suffix_base==2|10, or suffix_factor==1000/1024 as you > suggested. I think I'd prefer the former but either works for me. I'd prefer suffix_factor to avoid confusion with word 'base' which is commonly used in conversion routines (i.e. man strtoll). > > > > > > > > > Andreas > > > > > > > + 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_unit_suffixed_int = parse_type_unit_suffixed_int; > > > > > > > > v->string = str; > > > > return v; > > > > > > > > > > > > >
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 75214e7..57e662c 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -143,6 +143,12 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[], { } +static void qapi_dealloc_type_unit_suffixed_int(Visitor *v, int64_t *obj, + const char *name, + const int unit, Error **errp) +{ +} + Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) { return &v->visitor; @@ -170,6 +176,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_unit_suffixed_int = qapi_dealloc_type_unit_suffixed_int; QTAILQ_INIT(&v->stack); diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 7a82b63..dcbc1a9 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -311,3 +311,16 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[], g_free(enum_str); *obj = value; } + +void visit_type_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, + const int unit, Error **errp) +{ + if (!error_is_set(errp)) { + return; + } + if (v->type_unit_suffixed_int) { + v->type_unit_suffixed_int(v, obj, name, unit, 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..04e690a 100644 --- a/qapi/qapi-visit-core.h +++ b/qapi/qapi-visit-core.h @@ -62,6 +62,12 @@ 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); + /* + * visit_unit_suffixed_int() falls back to (*type_int64)() + * if type_unit_suffixed_int is unset + */ + void (*type_unit_suffixed_int)(Visitor *v, int64_t *obj, const char *name, + const int unit, Error **errp); }; void visit_start_handle(Visitor *v, void **obj, const char *kind, @@ -91,5 +97,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_unit_suffixed_int(Visitor *v, int64_t *obj, const char *name, + const int unit, Error **errp); #endif diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 497eb9a..d2bd154 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_unit_suffixed_int(Visitor *v, int64_t *obj, + const char *name, const int unit, + Error **errp) +{ + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); + char *endp = (char *) siv->string; + long long val = 0; + + if (siv->string) { + val = strtosz_suffix_unit(siv->string, &endp, + STRTOSZ_DEFSUFFIX_B, unit); + } + 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_unit_suffixed_int = parse_type_unit_suffixed_int; v->string = str; return v;
Caller of visit_type_unit_suffixed_int() will have to specify value of 'K' suffix via unit argument. For Kbytes it's 1024, for Khz it's 1000. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v2: - convert type_freq to type_unit_suffixed_int. - provide qapi_dealloc_type_unit_suffixed_int() impl. --- qapi/qapi-dealloc-visitor.c | 7 +++++++ qapi/qapi-visit-core.c | 13 +++++++++++++ qapi/qapi-visit-core.h | 8 ++++++++ qapi/string-input-visitor.c | 22 ++++++++++++++++++++++ 4 files changed, 50 insertions(+)