[Qemu-devel] [PATCH 0/2] introduce visitor for parsing suffixed integer

[Qemu-devel] [PATCH 0/2] introduce visitor for parsing suffixed integer

[Igor Mammedov]

Igor Mammedov (2):
  qapi: add visitor for parsing int[KMGT] input string
  target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq
    property value

 qapi/qapi-dealloc-visitor.c |  7 +++++++
 qapi/qapi-visit-core.c      | 13 +++++++++++++
 qapi/qapi-visit-core.h      |  8 ++++++++
 qapi/string-input-visitor.c | 22 ++++++++++++++++++++++
 target-i386/cpu.c           |  2 +-
 5 files changed, 51 insertions(+), 1 deletion(-)

-- 
1.7.11.7

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

[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)) {
+        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

[Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
   - replace visit_type_freq() with visit_type_unit_suffixed_int()
     in x86_cpuid_set_tsc_freq()
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c6c2ca0..b7f0aba 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     const int64_t max = INT64_MAX;
     int64_t value;
 
-    visit_type_int(v, &value, name, errp);
+    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
     if (error_is_set(errp)) {
         return;
     }
-- 
1.7.11.7

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

[mdroth]

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
> 

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[mdroth]

On Thu, Dec 06, 2012 at 10:12:05PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>   v2:
>    - replace visit_type_freq() with visit_type_unit_suffixed_int()
>      in x86_cpuid_set_tsc_freq()
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..b7f0aba 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = INT64_MAX;
>      int64_t value;
> 
> -    visit_type_int(v, &value, name, errp);
> +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
>      if (error_is_set(errp)) {
>          return;
>      }
> -- 
> 1.7.11.7
> 

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

[Eduardo Habkost]

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
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[Eduardo Habkost]

On Thu, Dec 06, 2012 at 10:12:05PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v2:
>    - replace visit_type_freq() with visit_type_unit_suffixed_int()
>      in x86_cpuid_set_tsc_freq()

visit_type_unit_suffixed_int() matches the behavior of the current
tsc_freq parsing code on cpu_x86_parse_featurestr(), so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..b7f0aba 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = INT64_MAX;
>      int64_t value;
>  
> -    visit_type_int(v, &value, name, errp);
> +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
>      if (error_is_set(errp)) {
>          return;
>      }
> -- 
> 1.7.11.7
> 

-- 
Eduardo

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

[Andreas Färber]

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;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[Andreas Färber]

Am 06.12.2012 22:12, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v2:
>    - replace visit_type_freq() with visit_type_unit_suffixed_int()
>      in x86_cpuid_set_tsc_freq()
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..b7f0aba 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = INT64_MAX;
>      int64_t value;
>  
> -    visit_type_int(v, &value, name, errp);
> +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
>      if (error_is_set(errp)) {
>          return;
>      }

This trivial usage is fine obviously. But since this series set out to
make things more generic I am missing at least one use case for 1024.
Does nothing like that exist in qdev-properties.c or so already?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Eduardo Habkost]

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?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[Eduardo Habkost]

On Fri, Dec 07, 2012 at 08:00:09PM +0100, Andreas Färber wrote:
> Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v2:
> >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> >      in x86_cpuid_set_tsc_freq()
> > ---
> >  target-i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c6c2ca0..b7f0aba 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      const int64_t max = INT64_MAX;
> >      int64_t value;
> >  
> > -    visit_type_int(v, &value, name, errp);
> > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> >      if (error_is_set(errp)) {
> >          return;
> >      }
> 
> This trivial usage is fine obviously. But since this series set out to
> make things more generic I am missing at least one use case for 1024.
> Does nothing like that exist in qdev-properties.c or so already?

cutils.c has:

int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
    return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
}

$ git grep -w strtosz_suffix
[...]
qapi/opts-visitor.c:    val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
qemu-img.c:        sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
qemu-img.c:            sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);

The opts-visitor.c match, in turn, is inside opts_type_size(), that's the
->type_size method of OptsVisitor. There are many 'size' elements inside
qapi-schema.json.

I don't see any code using visit_type_size() directly, but I see two users of
type 'size' on qapi-schema.json: NetdevTapOptions and NetdevDumpOptions.

I didn't know that we already had a visitor method using the suffixed-int
parsing code. Should we change the visit_type_size() code to be to use use the
new generic ->type_suffixed_int method and kill ->type_size?

-- 
Eduardo

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

[Igor Mammedov]

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;
> > 
> 
> 

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[Igor Mammedov]

On Fri, 7 Dec 2012 18:09:06 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 07, 2012 at 08:00:09PM +0100, Andreas Färber wrote:
> > Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >   v2:
> > >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> > >      in x86_cpuid_set_tsc_freq()
> > > ---
> > >  target-i386/cpu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index c6c2ca0..b7f0aba 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> > > Visitor *v, void *opaque, const int64_t max = INT64_MAX;
> > >      int64_t value;
> > >  
> > > -    visit_type_int(v, &value, name, errp);
> > > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> > >      if (error_is_set(errp)) {
> > >          return;
> > >      }
> > 
> > This trivial usage is fine obviously. But since this series set out to
> > make things more generic I am missing at least one use case for 1024.
> > Does nothing like that exist in qdev-properties.c or so already?
> 
> cutils.c has:
> 
> int64_t strtosz_suffix(const char *nptr, char **end, const char
> default_suffix) {
>     return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
> }
> 
> $ git grep -w strtosz_suffix
> [...]
> qapi/opts-visitor.c:    val = strtosz_suffix(opt->str ? opt->str : "",
> &endptr, qemu-img.c:        sval = strtosz_suffix(argv[optind++], &end,
> STRTOSZ_DEFSUFFIX_B); qemu-img.c:            sval = strtosz_suffix(optarg,
> &end, STRTOSZ_DEFSUFFIX_B);
> 
> The opts-visitor.c match, in turn, is inside opts_type_size(), that's the
> ->type_size method of OptsVisitor. There are many 'size' elements inside
> qapi-schema.json.
> 
> I don't see any code using visit_type_size() directly, but I see two users
> of type 'size' on qapi-schema.json: NetdevTapOptions and NetdevDumpOptions.
> 
> I didn't know that we already had a visitor method using the suffixed-int
> parsing code. Should we change the visit_type_size() code to be to use use
> the new generic ->type_suffixed_int method and kill ->type_size?

If there isn't strong opposition to do it in incremental way,
I'd prefer for these patches go in first.

And then later fix users of visit_type_size() to use type_suffixed_int() or
maybe have a new type_suffixed_uint() so that size could be represented as
uint64_t instead of int64_t as it's now. That would require to
rewrite strtosz_* and its callers a bit.

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[mdroth]

On Mon, Dec 10, 2012 at 05:13:45PM +0100, Igor Mammedov wrote:
> On Fri, 7 Dec 2012 18:09:06 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Dec 07, 2012 at 08:00:09PM +0100, Andreas Färber wrote:
> > > Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >   v2:
> > > >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> > > >      in x86_cpuid_set_tsc_freq()
> > > > ---
> > > >  target-i386/cpu.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index c6c2ca0..b7f0aba 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> > > > Visitor *v, void *opaque, const int64_t max = INT64_MAX;
> > > >      int64_t value;
> > > >  
> > > > -    visit_type_int(v, &value, name, errp);
> > > > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> > > >      if (error_is_set(errp)) {
> > > >          return;
> > > >      }
> > > 
> > > This trivial usage is fine obviously. But since this series set out to
> > > make things more generic I am missing at least one use case for 1024.
> > > Does nothing like that exist in qdev-properties.c or so already?
> > 
> > cutils.c has:
> > 
> > int64_t strtosz_suffix(const char *nptr, char **end, const char
> > default_suffix) {
> >     return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
> > }
> > 
> > $ git grep -w strtosz_suffix
> > [...]
> > qapi/opts-visitor.c:    val = strtosz_suffix(opt->str ? opt->str : "",
> > &endptr, qemu-img.c:        sval = strtosz_suffix(argv[optind++], &end,
> > STRTOSZ_DEFSUFFIX_B); qemu-img.c:            sval = strtosz_suffix(optarg,
> > &end, STRTOSZ_DEFSUFFIX_B);
> > 
> > The opts-visitor.c match, in turn, is inside opts_type_size(), that's the
> > ->type_size method of OptsVisitor. There are many 'size' elements inside
> > qapi-schema.json.
> > 
> > I don't see any code using visit_type_size() directly, but I see two users
> > of type 'size' on qapi-schema.json: NetdevTapOptions and NetdevDumpOptions.
> > 
> > I didn't know that we already had a visitor method using the suffixed-int
> > parsing code. Should we change the visit_type_size() code to be to use use
> > the new generic ->type_suffixed_int method and kill ->type_size?
> 
> If there isn't strong opposition to do it in incremental way,
> I'd prefer for these patches go in first.
> 
> And then later fix users of visit_type_size() to use type_suffixed_int() or
> maybe have a new type_suffixed_uint() so that size could be represented as
> uint64_t instead of int64_t as it's now. That would require to
> rewrite strtosz_* and its callers a bit.

I think that seems reasonable. Between the 2 that should give us a nice
way to support [KMGx] suffixes for options in a backward-compatible way.

Shame that users don't have a well-established way to specify base 2 vs.
10, but so long as we document our choice for each option that should
cover most cases well enough.

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

[mdroth]

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;
> > > 
> > 
> > 
> 

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

[Igor Mammedov]

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;
> > > > 
> > > 
> > > 
> > 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

[Igor Mammedov]

On Fri, 07 Dec 2012 20:00:09 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v2:
> >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> >      in x86_cpuid_set_tsc_freq()
> > ---
> >  target-i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c6c2ca0..b7f0aba 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      const int64_t max = INT64_MAX;
> >      int64_t value;
> >  
> > -    visit_type_int(v, &value, name, errp);
> > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> >      if (error_is_set(errp)) {
> >          return;
> >      }
> 
> This trivial usage is fine obviously. But since this series set out to
> make things more generic I am missing at least one use case for 1024.
> Does nothing like that exist in qdev-properties.c or so already?
It would be nice to have qdev property for this, perhaps after cpu properties
series we could introduce it and simplify target-i386/cpu.c code a bit.

> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor