Re: [Qemu-devel] [PATCH 1/7] qapi: add Visitor interfaces for uint*_t and int*_t

Re: [Qemu-devel] [PATCH 1/7] qapi: add Visitor interfaces for uint*_t and int*_t

[Andreas Färber]

Am 27.04.2012 22:21, schrieb Michael Roth:
> This adds visitor interfaces for fixed-width integers types.
> Implementing these in visitors is optional, otherwise we fall back to
> visit_type_int() (int64_t) with some additional bounds checking to avoid
> integer overflows for cases where the value fetched exceeds the bounds
> of our target C type.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

/-F

-- 
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 6/7] qdev: use int32_t container for devfn property

[Andreas Färber]

Am 27.04.2012 22:21, schrieb Michael Roth:
> Valid range for devfn is -1 to 255 (-1 for automatic assignment). We do
> not currently validate this due to devfn being stored as a uint32_t.
> This can lead to segfaults and other strange behavior.
> 
> We could technically just cast it to int32_t to implement the checking,
> but this will not work for visitor-based setting where we may do additional
> bounds-checking based on target container type, which is int32_t for this
> case.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Upper limit matches my limited PCI knowledge; cc'ing mst.

/-F

> ---
>  hw/pci.c             |    2 +-
>  hw/pci.h             |    2 +-
>  hw/qdev-properties.c |   11 ++++-------
>  hw/qdev.h            |    2 +-
>  4 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index b706e69..7818c9b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1538,7 +1538,7 @@ PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->qbus, name);
> -    qdev_prop_set_uint32(dev, "addr", devfn);
> +    qdev_prop_set_int32(dev, "addr", devfn);
>      qdev_prop_set_bit(dev, "multifunction", multifunction);
>      return PCI_DEVICE(dev);
>  }
> diff --git a/hw/pci.h b/hw/pci.h
> index 8d0aa49..3bc9218 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -193,7 +193,7 @@ struct PCIDevice {
>  
>      /* the following fields are read only */
>      PCIBus *bus;
> -    uint32_t devfn;
> +    int32_t devfn;
>      char name[64];
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>  
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 98dd06a..36d0aa0 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -822,7 +822,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque,
>  {
>      DeviceState *dev = DEVICE(obj);
>      Property *prop = opaque;
> -    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
>      unsigned int slot, fn, n;
>      Error *local_err = NULL;
>      char *str = (char *)"";
> @@ -855,7 +855,7 @@ invalid:
>  
>  static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t len)
>  {
> -    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
>  
>      if (*ptr == -1) {
>          return snprintf(dest, len, "<unset>");
> @@ -870,11 +870,8 @@ PropertyInfo qdev_prop_pci_devfn = {
>      .print = print_pci_devfn,
>      .get   = get_int32,
>      .set   = set_pci_devfn,
> -    /* FIXME: this should be -1...255, but the address is stored
> -     * into an uint32_t rather than int32_t.
> -     */
> -    .min   = 0,
> -    .max   = 0xFFFFFFFFULL,
> +    .min   = -1,
> +    .max   = 255,
>  };
>  
>  /* --- blocksize --- */
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 4e90119..d07da45 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -267,7 +267,7 @@ extern PropertyInfo qdev_prop_blocksize;
>  #define DEFINE_PROP_HEX64(_n, _s, _f, _d)                       \
>      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex64, uint64_t)
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
> -    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, uint32_t)
> +    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
>  
>  #define DEFINE_PROP_PTR(_n, _s, _f)             \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)


-- 
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 7/7] qdev: switch property accessors to fixed-width visitor interfaces

[Andreas Färber]

Am 27.04.2012 22:21, schrieb Michael Roth:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

/-F

-- 
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 v5 0/7] add fixed-width visitors and serialization tests/fixes

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

Am 27.04.2012 22:21, schrieb Michael Roth:
> These patches apply on top of qemu.git master, and can also be obtained from:
> git://github.com/mdroth/qemu.git visitor-fixed-width-v5

I've tested that branch by running some random guests without noticeable
problems and by testing X86CPU level/xlevel simplifications on top
(attached).

NOTE: There is a v6 patch with fixed commit message hidden as reply
within this series but there is no matching -v6 branch pushed yet.

That being said, v5 series

Tested-by: Andreas Färber <afaerber@suse.de>

Andreas

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-target-i386-Use-uint32-visitor-for-x-level-propertie.patch --]
[-- Type: text/x-patch; name="0001-target-i386-Use-uint32-visitor-for-x-level-propertie.patch", Size: 3075 bytes --]

>From b19a05c8dff628af5f0170cc53c8319af0074104 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andreas=20F=C3=A4rber?= <afaerber@suse.de>
Date: Tue, 1 May 2012 23:33:13 +0200
Subject: [PATCH] target-i386: Use uint32 visitor for [x]level properties
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This simplifies the code and resolves TODOs.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/cpu.c |   42 ++++--------------------------------------
 1 files changed, 4 insertions(+), 38 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 65d9af6..af8e1f3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -715,66 +715,32 @@ static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque,
                                 const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    int64_t value;
 
-    value = cpu->env.cpuid_level;
-    /* TODO Use visit_type_uint32() once available */
-    visit_type_int(v, &value, name, errp);
+    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
 }
 
 static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque,
                                 const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    const int64_t min = 0;
-    const int64_t max = UINT32_MAX;
-    int64_t value;
-
-    /* TODO Use visit_type_uint32() once available */
-    visit_type_int(v, &value, name, errp);
-    if (error_is_set(errp)) {
-        return;
-    }
-    if (value < min || value > max) {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                  name ? name : "null", value, min, max);
-        return;
-    }
 
-    cpu->env.cpuid_level = value;
+    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
 }
 
 static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque,
                                  const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    int64_t value;
 
-    value = cpu->env.cpuid_xlevel;
-    /* TODO Use visit_type_uint32() once available */
-    visit_type_int(v, &value, name, errp);
+    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
 }
 
 static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque,
                                  const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    const int64_t min = 0;
-    const int64_t max = UINT32_MAX;
-    int64_t value;
-
-    /* TODO Use visit_type_uint32() once available */
-    visit_type_int(v, &value, name, errp);
-    if (error_is_set(errp)) {
-        return;
-    }
-    if (value < min || value > max) {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                  name ? name : "null", value, min, max);
-        return;
-    }
 
-    cpu->env.cpuid_xlevel = value;
+    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
 }
 
 static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
-- 
1.7.7

Re: [Qemu-devel] [PATCH v5 0/7] add fixed-width visitors and serialization tests/fixes

[Andreas Färber]

Am 27.04.2012 22:21, schrieb Michael Roth:
> These patches apply on top of qemu.git master, and can also be obtained from:
> git://github.com/mdroth/qemu.git visitor-fixed-width-v5
> 
> Some of these were being carried as part of Paolo's realize series due to some
> conflicts, but that looks to be targetted for 1.2 now, and there's a QMP
> visitor bug and a small issue with String visitor that were caught by the test
> infrastructure introduced here and fixed as part of this series, so I'd like to
> get this in for 1.1

Thanks, I've applied v6 to qom-next (as usual massaging the commit
messages a bit, in particular extending the last one):
http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-next

Reasoning:
This series has been around since end of February. v3 fixed a breakage
reported by Anthony (use of signed rather than unsigned visitors); since
then changes were mostly rebasing, and v5/v6 pass make check and my
smoke tests.
While this is not strictly a QOM series, I am picking it as a
prerequisite since Paolo had picked up patches 1, 6, 7 for his Object
properties movement and because patch 1 is handy for newly added
properties such as of the x86 CPU.
Further, Paolo agreed to rebase onto this series.

However, it is my understanding that patches 2 and 4 are independent
bugfixes and as such should go into 1.1.

Luiz, should I send Anthony a PULL for 1.1-rc2 including those two? Can
you ack then? Or do you want to cherry-pick them from qom-next yourself?

Andreas

> CHANGES SINCE v4:
>  - Rebased on master (a8b69b8e2431edfcb6c4cfb069787e9071d6235b) and re-tested
>  - Re-ordered patches so visitor bugs are applied before the test cases that
>    that trigger them.
> 
> CHANGES SINCE V3:
>  - Rebased on master and re-tested
> 
> CHANGES SINCE V2:
>  - Fix qemu-test errors due to now-strict bounds-checking we doing assignment
>    between signed/unsigned types.
>  - uint* property getters/setters no longer use int* getters/setters.
>  - valid devfn range is now explicitly enforced.
> 
> CHANGES SINCE V1:
>  - unit tests: covert QmpOutputVisitor qobject to json before passing it to
>    QmpInputVisitor*. I.e., actually do the serialization :)
>  - QmpInputVisitor, add handling for when a serialized QFloat gets read back
>    as a QInt
>  - unit tests: add coverage for String visitor
>  - StringOutputVisitor: use %f for float representation
> 
> These patches add fixed-width visitor interfaces and switches all qdev users
> over to using them.
> 
> We also add a test suite which covers these interfaces, and also does some
> sanity checking on Visitors (Qmp/String currently, with a pluggable interface
> for future implementations) to ensure Visitor input/output handling remain
> self-consistent, which is not covered by the current visitor tests which mostly
> test input/output seperately. Maintaining this invariant is necessary to ensure
> that visitors can be used for serialization/deserialization in the future.
> 
>  hw/mc146818rtc.c                   |    7 -
>  hw/pci.c                           |    2 +-
>  hw/pci.h                           |    2 +-
>  hw/qdev-addr.c                     |    4 +-
>  hw/qdev-properties.c               |  161 +++++---
>  hw/qdev.h                          |    2 +-
>  qapi/qapi-visit-core.c             |  139 +++++++
>  qapi/qapi-visit-core.h             |   16 +
>  qapi/qmp-input-visitor.c           |    9 +-
>  qapi/string-output-visitor.c       |    2 +-
>  tests/Makefile                     |    4 +-
>  tests/test-string-output-visitor.c |    2 +-
>  tests/test-visitor-serialization.c |  784 ++++++++++++++++++++++++++++++++++++
>  13 files changed, 1049 insertions(+), 85 deletions(-)

-- 
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 v5 0/7] add fixed-width visitors and serialization tests/fixes

[Michael Roth]

On Fri, May 11, 2012 at 03:22:15AM +0200, Andreas Färber wrote:
> Am 27.04.2012 22:21, schrieb Michael Roth:
> > These patches apply on top of qemu.git master, and can also be obtained from:
> > git://github.com/mdroth/qemu.git visitor-fixed-width-v5
> > 
> > Some of these were being carried as part of Paolo's realize series due to some
> > conflicts, but that looks to be targetted for 1.2 now, and there's a QMP
> > visitor bug and a small issue with String visitor that were caught by the test
> > infrastructure introduced here and fixed as part of this series, so I'd like to
> > get this in for 1.1
> 
> Thanks, I've applied v6 to qom-next (as usual massaging the commit
> messages a bit, in particular extending the last one):
> http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-next
> 
> Reasoning:
> This series has been around since end of February. v3 fixed a breakage
> reported by Anthony (use of signed rather than unsigned visitors); since
> then changes were mostly rebasing, and v5/v6 pass make check and my
> smoke tests.
> While this is not strictly a QOM series, I am picking it as a
> prerequisite since Paolo had picked up patches 1, 6, 7 for his Object
> properties movement and because patch 1 is handy for newly added
> properties such as of the x86 CPU.
> Further, Paolo agreed to rebase onto this series.
> 
> However, it is my understanding that patches 2 and 4 are independent
> bugfixes and as such should go into 1.1.

Agreed, my intention as well.

Thanks for taking these in!

> 
> Luiz, should I send Anthony a PULL for 1.1-rc2 including those two? Can
> you ack then? Or do you want to cherry-pick them from qom-next yourself?
> 
> Andreas
> 
> > CHANGES SINCE v4:
> >  - Rebased on master (a8b69b8e2431edfcb6c4cfb069787e9071d6235b) and re-tested
> >  - Re-ordered patches so visitor bugs are applied before the test cases that
> >    that trigger them.
> > 
> > CHANGES SINCE V3:
> >  - Rebased on master and re-tested
> > 
> > CHANGES SINCE V2:
> >  - Fix qemu-test errors due to now-strict bounds-checking we doing assignment
> >    between signed/unsigned types.
> >  - uint* property getters/setters no longer use int* getters/setters.
> >  - valid devfn range is now explicitly enforced.
> > 
> > CHANGES SINCE V1:
> >  - unit tests: covert QmpOutputVisitor qobject to json before passing it to
> >    QmpInputVisitor*. I.e., actually do the serialization :)
> >  - QmpInputVisitor, add handling for when a serialized QFloat gets read back
> >    as a QInt
> >  - unit tests: add coverage for String visitor
> >  - StringOutputVisitor: use %f for float representation
> > 
> > These patches add fixed-width visitor interfaces and switches all qdev users
> > over to using them.
> > 
> > We also add a test suite which covers these interfaces, and also does some
> > sanity checking on Visitors (Qmp/String currently, with a pluggable interface
> > for future implementations) to ensure Visitor input/output handling remain
> > self-consistent, which is not covered by the current visitor tests which mostly
> > test input/output seperately. Maintaining this invariant is necessary to ensure
> > that visitors can be used for serialization/deserialization in the future.
> > 
> >  hw/mc146818rtc.c                   |    7 -
> >  hw/pci.c                           |    2 +-
> >  hw/pci.h                           |    2 +-
> >  hw/qdev-addr.c                     |    4 +-
> >  hw/qdev-properties.c               |  161 +++++---
> >  hw/qdev.h                          |    2 +-
> >  qapi/qapi-visit-core.c             |  139 +++++++
> >  qapi/qapi-visit-core.h             |   16 +
> >  qapi/qmp-input-visitor.c           |    9 +-
> >  qapi/string-output-visitor.c       |    2 +-
> >  tests/Makefile                     |    4 +-
> >  tests/test-string-output-visitor.c |    2 +-
> >  tests/test-visitor-serialization.c |  784 ++++++++++++++++++++++++++++++++++++
> >  13 files changed, 1049 insertions(+), 85 deletions(-)
> 
> -- 
> 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/7] qapi: QMP input visitor, handle floats parsed as ints

[Luiz Capitulino]

On Fri, 27 Apr 2012 15:21:18 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> JSON numbers can be interpreted as either integers or floating point
> values depending on their representation. As a result, QMP input visitor
> might visit a QInt when it was expecting a QFloat, so add handling to
> account for this.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qapi/qmp-input-visitor.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 4cdc47d..bc91134 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -246,13 +246,18 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
>      QmpInputVisitor *qiv = to_qiv(v);
>      QObject *qobj = qmp_input_get_object(qiv, name);
>  
> -    if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
> +    if (!qobj || (qobject_type(qobj) != QTYPE_QFLOAT &&
> +        qobject_type(qobj) != QTYPE_QINT)) {
>          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                    "double");

s/double/number

It's also important to note that migrate_set_downtime is (positively) affected
by this change. Today, it only accepts a double, eg.:

{ "execute": "migrate_set_downtime", "arguments": { "value": 1 } }
{
    "error": {
        "class": "InvalidParameterType", 
        "desc": "Invalid parameter type for 'value', expected: double", 
        "data": {
            "name": "value", 
            "expected": "double"
        }
    }
}

That's a bug (as it's documented to accept a number) and this patch fixes it.
There's an interface change, but I think it won't cause problems in practice.

Please, fix the error message above and would be nice to get this (and patch
02/07) as a separate series for 1.1.


>          return;
>      }
>  
> -    *obj = qfloat_get_double(qobject_to_qfloat(qobj));
> +    if (qobject_type(qobj) == QTYPE_QINT) {
> +        *obj = qint_get_int(qobject_to_qint(qobj));
> +    } else {
> +        *obj = qfloat_get_double(qobject_to_qfloat(qobj));
> +    }
>  }
>  
>  static void qmp_input_start_optional(Visitor *v, bool *present,

Re: [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats

[Luiz Capitulino]

On Fri, 27 Apr 2012 15:21:20 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Currently string-output-visitor formats floats as %g, which is nice in
> that trailing 0's are automatically truncated, but otherwise this causes
> some issues:
> 
>  - it 6 uses significant figures instead of 6 decimal places, which
>    means something like 155777.5 (which even has an exact floating point
>    representation) will be rounded to 155778 when converted to a string.
> 
>  - output will be presented in scientific notation when the normalized
>    form requires a 10^x multiplier. Not a huge deal, but arguably less
>    readable for command-line arguments.
> 
>  - due to using sig figs instead of hard-defined decimal places, it
>    fails a lot of the test-visitor-serialization unit tests for floats.
> 
> Instead, let's just use %f, which is what the QJSON and the QMP visitors
> use.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qapi/string-output-visitor.c       |    2 +-
>  tests/test-string-output-visitor.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 92b0305..34e525e 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -52,7 +52,7 @@ static void print_type_number(Visitor *v, double *obj, const char *name,
>                                Error **errp)
>  {
>      StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
> -    string_output_set(sov, g_strdup_printf("%g", *obj));
> +    string_output_set(sov, g_strdup_printf("%f", *obj));

Doesn't look like a bug fix worth it for 1.1, am I wrong?

>  }
>  
>  char *string_output_get_string(StringOutputVisitor *sov)
> diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
> index 22909b8..608f14a 100644
> --- a/tests/test-string-output-visitor.c
> +++ b/tests/test-string-output-visitor.c
> @@ -84,7 +84,7 @@ static void test_visitor_out_number(TestOutputVisitorData *data,
>  
>      str = string_output_get_string(data->sov);
>      g_assert(str != NULL);
> -    g_assert_cmpstr(str, ==, "3.14");
> +    g_assert_cmpstr(str, ==, "3.140000");
>      g_free(str);
>  }
>  

Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints

[Michael Roth]

On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote:
> On Fri, 27 Apr 2012 15:21:18 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > JSON numbers can be interpreted as either integers or floating point
> > values depending on their representation. As a result, QMP input visitor
> > might visit a QInt when it was expecting a QFloat, so add handling to
> > account for this.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  qapi/qmp-input-visitor.c |    9 +++++++--
> >  1 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index 4cdc47d..bc91134 100644
> > --- a/qapi/qmp-input-visitor.c
> > +++ b/qapi/qmp-input-visitor.c
> > @@ -246,13 +246,18 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
> >      QmpInputVisitor *qiv = to_qiv(v);
> >      QObject *qobj = qmp_input_get_object(qiv, name);
> >  
> > -    if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
> > +    if (!qobj || (qobject_type(qobj) != QTYPE_QFLOAT &&
> > +        qobject_type(qobj) != QTYPE_QINT)) {
> >          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> >                    "double");
> 
> s/double/number
> 
> It's also important to note that migrate_set_downtime is (positively) affected
> by this change. Today, it only accepts a double, eg.:
> 
> { "execute": "migrate_set_downtime", "arguments": { "value": 1 } }
> {
>     "error": {
>         "class": "InvalidParameterType", 
>         "desc": "Invalid parameter type for 'value', expected: double", 
>         "data": {
>             "name": "value", 
>             "expected": "double"
>         }
>     }
> }
> 
> That's a bug (as it's documented to accept a number) and this patch fixes it.
> There's an interface change, but I think it won't cause problems in practice.
> 
> Please, fix the error message above and would be nice to get this (and patch
> 02/07) as a separate series for 1.1.

Hmm, this is kinda awkward because I don't think we can change it without
breaking any trees based off qom-next.

Since the error msg predates the bug fix (it just becomes more obviously
wrong as a result of the fix), can you pull these commits as is into your
QMP tree?

In the meantime I can send another patch, based on qom-next or qmp, that
fixes the error msg. We can then get all 3 into 1.1/master via QMP tree, and I
think qom-next should still merge cleanly back into master after 1.1

> 
> 
> >          return;
> >      }
> >  
> > -    *obj = qfloat_get_double(qobject_to_qfloat(qobj));
> > +    if (qobject_type(qobj) == QTYPE_QINT) {
> > +        *obj = qint_get_int(qobject_to_qint(qobj));
> > +    } else {
> > +        *obj = qfloat_get_double(qobject_to_qfloat(qobj));
> > +    }
> >  }
> >  
> >  static void qmp_input_start_optional(Visitor *v, bool *present,
> 

Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints

[Andreas Färber]

Am 11.05.2012 19:04, schrieb Michael Roth:
> On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote:
>> On Fri, 27 Apr 2012 15:21:18 -0500
>> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>>
>>> JSON numbers can be interpreted as either integers or floating point
>>> values depending on their representation. As a result, QMP input visitor
>>> might visit a QInt when it was expecting a QFloat, so add handling to
>>> account for this.
>>>
>>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> ---
>>>  qapi/qmp-input-visitor.c |    9 +++++++--
>>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
>>> index 4cdc47d..bc91134 100644
>>> --- a/qapi/qmp-input-visitor.c
>>> +++ b/qapi/qmp-input-visitor.c
>>> @@ -246,13 +246,18 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
>>>      QmpInputVisitor *qiv = to_qiv(v);
>>>      QObject *qobj = qmp_input_get_object(qiv, name);
>>>  
>>> -    if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
>>> +    if (!qobj || (qobject_type(qobj) != QTYPE_QFLOAT &&
>>> +        qobject_type(qobj) != QTYPE_QINT)) {
>>>          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>>                    "double");
>>
>> s/double/number
>>
>> It's also important to note that migrate_set_downtime is (positively) affected
>> by this change. Today, it only accepts a double, eg.:
>>
>> { "execute": "migrate_set_downtime", "arguments": { "value": 1 } }
>> {
>>     "error": {
>>         "class": "InvalidParameterType", 
>>         "desc": "Invalid parameter type for 'value', expected: double", 
>>         "data": {
>>             "name": "value", 
>>             "expected": "double"
>>         }
>>     }
>> }
>>
>> That's a bug (as it's documented to accept a number) and this patch fixes it.
>> There's an interface change, but I think it won't cause problems in practice.
>>
>> Please, fix the error message above and would be nice to get this (and patch
>> 02/07) as a separate series for 1.1.
> 
> Hmm, this is kinda awkward because I don't think we can change it without
> breaking any trees based off qom-next.

That's no problem at all, it is announced as rebasing and I have scripts
in place to pull and recursively rebase all my QOM branches.

For such a trivial textual change I can even fix it up manually. :)

Since Luiz is now taking care of this one I will simply drop it from my
qom-1.1 branch to avoid any confusion.

Andreas

> Since the error msg predates the bug fix (it just becomes more obviously
> wrong as a result of the fix), can you pull these commits as is into your
> QMP tree?
> 
> In the meantime I can send another patch, based on qom-next or qmp, that
> fixes the error msg. We can then get all 3 into 1.1/master via QMP tree, and I
> think qom-next should still merge cleanly back into master after 1.1
> 
>>
>>
>>>          return;
>>>      }
>>>  
>>> -    *obj = qfloat_get_double(qobject_to_qfloat(qobj));
>>> +    if (qobject_type(qobj) == QTYPE_QINT) {
>>> +        *obj = qint_get_int(qobject_to_qint(qobj));
>>> +    } else {
>>> +        *obj = qfloat_get_double(qobject_to_qfloat(qobj));
>>> +    }
>>>  }
>>>  
>>>  static void qmp_input_start_optional(Visitor *v, bool *present,

-- 
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 4/7] qapi: String visitor, use %f represenation for floats

[Michael Roth]

On Fri, May 11, 2012 at 01:34:01PM -0300, Luiz Capitulino wrote:
> On Fri, 27 Apr 2012 15:21:20 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Currently string-output-visitor formats floats as %g, which is nice in
> > that trailing 0's are automatically truncated, but otherwise this causes
> > some issues:
> > 
> >  - it 6 uses significant figures instead of 6 decimal places, which
> >    means something like 155777.5 (which even has an exact floating point
> >    representation) will be rounded to 155778 when converted to a string.
> > 
> >  - output will be presented in scientific notation when the normalized
> >    form requires a 10^x multiplier. Not a huge deal, but arguably less
> >    readable for command-line arguments.
> > 
> >  - due to using sig figs instead of hard-defined decimal places, it
> >    fails a lot of the test-visitor-serialization unit tests for floats.
> > 
> > Instead, let's just use %f, which is what the QJSON and the QMP visitors
> > use.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  qapi/string-output-visitor.c       |    2 +-
> >  tests/test-string-output-visitor.c |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> > index 92b0305..34e525e 100644
> > --- a/qapi/string-output-visitor.c
> > +++ b/qapi/string-output-visitor.c
> > @@ -52,7 +52,7 @@ static void print_type_number(Visitor *v, double *obj, const char *name,
> >                                Error **errp)
> >  {
> >      StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
> > -    string_output_set(sov, g_strdup_printf("%g", *obj));
> > +    string_output_set(sov, g_strdup_printf("%f", *obj));
> 
> Doesn't look like a bug fix worth it for 1.1, am I wrong?

Well, object_property_print() is the only string-output-visitor user,
and it's not currently used. I don't see this changing for 1.1, so this
can probably wait.

> 
> >  }
> >  
> >  char *string_output_get_string(StringOutputVisitor *sov)
> > diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
> > index 22909b8..608f14a 100644
> > --- a/tests/test-string-output-visitor.c
> > +++ b/tests/test-string-output-visitor.c
> > @@ -84,7 +84,7 @@ static void test_visitor_out_number(TestOutputVisitorData *data,
> >  
> >      str = string_output_get_string(data->sov);
> >      g_assert(str != NULL);
> > -    g_assert_cmpstr(str, ==, "3.14");
> > +    g_assert_cmpstr(str, ==, "3.140000");
> >      g_free(str);
> >  }
> >  
> 

Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints

[Michael Roth]

On Fri, May 11, 2012 at 07:16:18PM +0200, Andreas Färber wrote:
> Am 11.05.2012 19:04, schrieb Michael Roth:
> > On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote:
> >> On Fri, 27 Apr 2012 15:21:18 -0500
> >> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >>
> >>> JSON numbers can be interpreted as either integers or floating point
> >>> values depending on their representation. As a result, QMP input visitor
> >>> might visit a QInt when it was expecting a QFloat, so add handling to
> >>> account for this.
> >>>
> >>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >>> ---
> >>>  qapi/qmp-input-visitor.c |    9 +++++++--
> >>>  1 files changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> >>> index 4cdc47d..bc91134 100644
> >>> --- a/qapi/qmp-input-visitor.c
> >>> +++ b/qapi/qmp-input-visitor.c
> >>> @@ -246,13 +246,18 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
> >>>      QmpInputVisitor *qiv = to_qiv(v);
> >>>      QObject *qobj = qmp_input_get_object(qiv, name);
> >>>  
> >>> -    if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
> >>> +    if (!qobj || (qobject_type(qobj) != QTYPE_QFLOAT &&
> >>> +        qobject_type(qobj) != QTYPE_QINT)) {
> >>>          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> >>>                    "double");
> >>
> >> s/double/number
> >>
> >> It's also important to note that migrate_set_downtime is (positively) affected
> >> by this change. Today, it only accepts a double, eg.:
> >>
> >> { "execute": "migrate_set_downtime", "arguments": { "value": 1 } }
> >> {
> >>     "error": {
> >>         "class": "InvalidParameterType", 
> >>         "desc": "Invalid parameter type for 'value', expected: double", 
> >>         "data": {
> >>             "name": "value", 
> >>             "expected": "double"
> >>         }
> >>     }
> >> }
> >>
> >> That's a bug (as it's documented to accept a number) and this patch fixes it.
> >> There's an interface change, but I think it won't cause problems in practice.
> >>
> >> Please, fix the error message above and would be nice to get this (and patch
> >> 02/07) as a separate series for 1.1.
> > 
> > Hmm, this is kinda awkward because I don't think we can change it without
> > breaking any trees based off qom-next.
> 
> That's no problem at all, it is announced as rebasing and I have scripts
> in place to pull and recursively rebase all my QOM branches.
> 
> For such a trivial textual change I can even fix it up manually. :)
> 
> Since Luiz is now taking care of this one I will simply drop it from my
> qom-1.1 branch to avoid any confusion.

That'll work, thanks.

Luiz, I'll send a separate patch shortly with the error msg fixed.

> 
> Andreas
> 
> > Since the error msg predates the bug fix (it just becomes more obviously
> > wrong as a result of the fix), can you pull these commits as is into your
> > QMP tree?
> > 
> > In the meantime I can send another patch, based on qom-next or qmp, that
> > fixes the error msg. We can then get all 3 into 1.1/master via QMP tree, and I
> > think qom-next should still merge cleanly back into master after 1.1
> > 
> >>
> >>
> >>>          return;
> >>>      }
> >>>  
> >>> -    *obj = qfloat_get_double(qobject_to_qfloat(qobj));
> >>> +    if (qobject_type(qobj) == QTYPE_QINT) {
> >>> +        *obj = qint_get_int(qobject_to_qint(qobj));
> >>> +    } else {
> >>> +        *obj = qfloat_get_double(qobject_to_qfloat(qobj));
> >>> +    }
> >>>  }
> >>>  
> >>>  static void qmp_input_start_optional(Visitor *v, bool *present,
> 
> -- 
> 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/7] qapi: QMP input visitor, handle floats parsed as ints

[Luiz Capitulino]

On Fri, 11 May 2012 12:04:47 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote:
> > On Fri, 27 Apr 2012 15:21:18 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > JSON numbers can be interpreted as either integers or floating point
> > > values depending on their representation. As a result, QMP input visitor
> > > might visit a QInt when it was expecting a QFloat, so add handling to
> > > account for this.
> > > 
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > ---
> > >  qapi/qmp-input-visitor.c |    9 +++++++--
> > >  1 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > > index 4cdc47d..bc91134 100644
> > > --- a/qapi/qmp-input-visitor.c
> > > +++ b/qapi/qmp-input-visitor.c
> > > @@ -246,13 +246,18 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
> > >      QmpInputVisitor *qiv = to_qiv(v);
> > >      QObject *qobj = qmp_input_get_object(qiv, name);
> > >  
> > > -    if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
> > > +    if (!qobj || (qobject_type(qobj) != QTYPE_QFLOAT &&
> > > +        qobject_type(qobj) != QTYPE_QINT)) {
> > >          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> > >                    "double");
> > 
> > s/double/number
> > 
> > It's also important to note that migrate_set_downtime is (positively) affected
> > by this change. Today, it only accepts a double, eg.:
> > 
> > { "execute": "migrate_set_downtime", "arguments": { "value": 1 } }
> > {
> >     "error": {
> >         "class": "InvalidParameterType", 
> >         "desc": "Invalid parameter type for 'value', expected: double", 
> >         "data": {
> >             "name": "value", 
> >             "expected": "double"
> >         }
> >     }
> > }
> > 
> > That's a bug (as it's documented to accept a number) and this patch fixes it.
> > There's an interface change, but I think it won't cause problems in practice.
> > 
> > Please, fix the error message above and would be nice to get this (and patch
> > 02/07) as a separate series for 1.1.
> 
> Hmm, this is kinda awkward because I don't think we can change it without
> breaking any trees based off qom-next.
> 
> Since the error msg predates the bug fix (it just becomes more obviously
> wrong as a result of the fix), can you pull these commits as is into your
> QMP tree?
> 
> In the meantime I can send another patch, based on qom-next or qmp, that
> fixes the error msg. We can then get all 3 into 1.1/master via QMP tree, and I
> think qom-next should still merge cleanly back into master after 1.1

I think Andreas answered you here (in the sense that it's ok to respin this
patch), but I also would like to add that patch 4/7 doesn't seem a bug fix
to me (at least not worth it for 1.1).

Re: [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats

[Andreas Färber]

Am 11.05.2012 19:32, schrieb Michael Roth:
> On Fri, May 11, 2012 at 01:34:01PM -0300, Luiz Capitulino wrote:
>> On Fri, 27 Apr 2012 15:21:20 -0500
>> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>>
>>> Currently string-output-visitor formats floats as %g, which is nice in
>>> that trailing 0's are automatically truncated, but otherwise this causes
>>> some issues:
>>>
>>>  - it 6 uses significant figures instead of 6 decimal places, which
>>>    means something like 155777.5 (which even has an exact floating point
>>>    representation) will be rounded to 155778 when converted to a string.
>>>
>>>  - output will be presented in scientific notation when the normalized
>>>    form requires a 10^x multiplier. Not a huge deal, but arguably less
>>>    readable for command-line arguments.
>>>
>>>  - due to using sig figs instead of hard-defined decimal places, it
>>>    fails a lot of the test-visitor-serialization unit tests for floats.
>>>
>>> Instead, let's just use %f, which is what the QJSON and the QMP visitors
>>> use.
>>>
>>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> ---
>>>  qapi/string-output-visitor.c       |    2 +-
>>>  tests/test-string-output-visitor.c |    2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
>>> index 92b0305..34e525e 100644
>>> --- a/qapi/string-output-visitor.c
>>> +++ b/qapi/string-output-visitor.c
>>> @@ -52,7 +52,7 @@ static void print_type_number(Visitor *v, double *obj, const char *name,
>>>                                Error **errp)
>>>  {
>>>      StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
>>> -    string_output_set(sov, g_strdup_printf("%g", *obj));
>>> +    string_output_set(sov, g_strdup_printf("%f", *obj));
>>
>> Doesn't look like a bug fix worth it for 1.1, am I wrong?
> 
> Well, object_property_print() is the only string-output-visitor user,
> and it's not currently used. I don't see this changing for 1.1, so this
> can probably wait.

Actually it might be in 1.1: there's a pending patch by Paolo to use
that for info qtree, where some properties were missing. My review
comment has been resolved, so I will queue that patch for 1.1 and next.

Andreas

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