* [Qemu-devel] [PATCH 6/7] qdev: use int32_t container for devfn property
2012-03-28 23:02 [Qemu-devel] [PATCH v4 0/7] add fixed-width visitors and serialization tests Michael Roth
@ 2012-03-28 23:02 ` Michael Roth
0 siblings, 0 replies; 15+ messages in thread
From: Michael Roth @ 2012-03-28 23:02 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori
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>
---
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 ed8ec99..216e0a0 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 4f19fdb..3edf38a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -188,7 +188,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 bff9152..dd345ed 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,
};
/* --- public helpers --- */
diff --git a/hw/qdev.h b/hw/qdev.h
index 9cc3f98..d6f5f3a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -265,7 +265,7 @@ extern PropertyInfo qdev_prop_pci_devfn;
#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*)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] qapi: add Visitor interfaces for uint*_t and int*_t
[not found] ` <1335558083-26196-2-git-send-email-mdroth@linux.vnet.ibm.com>
@ 2012-05-01 21:37 ` Andreas Färber
0 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2012-05-01 21:37 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] qdev: use int32_t container for devfn property
[not found] ` <1335558083-26196-7-git-send-email-mdroth@linux.vnet.ibm.com>
@ 2012-05-01 21:52 ` Andreas Färber
0 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2012-05-01 21:52 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qdev: switch property accessors to fixed-width visitor interfaces
[not found] ` <1335558083-26196-8-git-send-email-mdroth@linux.vnet.ibm.com>
@ 2012-05-01 21:54 ` Andreas Färber
0 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2012-05-01 21:54 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/7] add fixed-width visitors and serialization tests/fixes
[not found] <1335558083-26196-1-git-send-email-mdroth@linux.vnet.ibm.com>
` (2 preceding siblings ...)
[not found] ` <1335558083-26196-8-git-send-email-mdroth@linux.vnet.ibm.com>
@ 2012-05-01 22:02 ` Andreas Färber
2012-05-11 1:22 ` Andreas Färber
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2012-05-01 22:02 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel
[-- 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/7] add fixed-width visitors and serialization tests/fixes
[not found] <1335558083-26196-1-git-send-email-mdroth@linux.vnet.ibm.com>
` (3 preceding siblings ...)
2012-05-01 22:02 ` [Qemu-devel] [PATCH v5 0/7] add fixed-width visitors and serialization tests/fixes Andreas Färber
@ 2012-05-11 1:22 ` Andreas Färber
2012-05-11 15:19 ` Michael Roth
[not found] ` <1335558083-26196-3-git-send-email-mdroth@linux.vnet.ibm.com>
[not found] ` <1335558083-26196-5-git-send-email-mdroth@linux.vnet.ibm.com>
6 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2012-05-11 1:22 UTC (permalink / raw)
To: Michael Roth, Luiz Capitulino
Cc: pbonzini, aliguori, qemu-devel, Markus Armbruster
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/7] add fixed-width visitors and serialization tests/fixes
2012-05-11 1:22 ` Andreas Färber
@ 2012-05-11 15:19 ` Michael Roth
0 siblings, 0 replies; 15+ messages in thread
From: Michael Roth @ 2012-05-11 15:19 UTC (permalink / raw)
To: Andreas Färber
Cc: pbonzini, aliguori, qemu-devel, Markus Armbruster,
Luiz Capitulino
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints
[not found] ` <1335558083-26196-3-git-send-email-mdroth@linux.vnet.ibm.com>
@ 2012-05-11 16:22 ` Luiz Capitulino
2012-05-11 17:04 ` Michael Roth
0 siblings, 1 reply; 15+ messages in thread
From: Luiz Capitulino @ 2012-05-11 16:22 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, afaerber
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,
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats
[not found] ` <1335558083-26196-5-git-send-email-mdroth@linux.vnet.ibm.com>
@ 2012-05-11 16:34 ` Luiz Capitulino
2012-05-11 17:32 ` Michael Roth
0 siblings, 1 reply; 15+ messages in thread
From: Luiz Capitulino @ 2012-05-11 16:34 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, afaerber
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);
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints
2012-05-11 16:22 ` [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints Luiz Capitulino
@ 2012-05-11 17:04 ` Michael Roth
2012-05-11 17:16 ` Andreas Färber
2012-05-11 17:38 ` Luiz Capitulino
0 siblings, 2 replies; 15+ messages in thread
From: Michael Roth @ 2012-05-11 17:04 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: pbonzini, aliguori, qemu-devel, afaerber
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,
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints
2012-05-11 17:04 ` Michael Roth
@ 2012-05-11 17:16 ` Andreas Färber
2012-05-11 17:34 ` Michael Roth
2012-05-11 17:38 ` Luiz Capitulino
1 sibling, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2012-05-11 17:16 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats
2012-05-11 16:34 ` [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats Luiz Capitulino
@ 2012-05-11 17:32 ` Michael Roth
2012-05-11 17:47 ` Andreas Färber
0 siblings, 1 reply; 15+ messages in thread
From: Michael Roth @ 2012-05-11 17:32 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: pbonzini, aliguori, qemu-devel, afaerber
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);
> > }
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints
2012-05-11 17:16 ` Andreas Färber
@ 2012-05-11 17:34 ` Michael Roth
0 siblings, 0 replies; 15+ messages in thread
From: Michael Roth @ 2012-05-11 17:34 UTC (permalink / raw)
To: Andreas Färber; +Cc: pbonzini, aliguori, qemu-devel, Luiz Capitulino
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints
2012-05-11 17:04 ` Michael Roth
2012-05-11 17:16 ` Andreas Färber
@ 2012-05-11 17:38 ` Luiz Capitulino
1 sibling, 0 replies; 15+ messages in thread
From: Luiz Capitulino @ 2012-05-11 17:38 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, afaerber
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).
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats
2012-05-11 17:32 ` Michael Roth
@ 2012-05-11 17:47 ` Andreas Färber
0 siblings, 0 replies; 15+ messages in thread
From: Andreas Färber @ 2012-05-11 17:47 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-05-11 18:30 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1335558083-26196-1-git-send-email-mdroth@linux.vnet.ibm.com>
[not found] ` <1335558083-26196-2-git-send-email-mdroth@linux.vnet.ibm.com>
2012-05-01 21:37 ` [Qemu-devel] [PATCH 1/7] qapi: add Visitor interfaces for uint*_t and int*_t Andreas Färber
[not found] ` <1335558083-26196-7-git-send-email-mdroth@linux.vnet.ibm.com>
2012-05-01 21:52 ` [Qemu-devel] [PATCH 6/7] qdev: use int32_t container for devfn property Andreas Färber
[not found] ` <1335558083-26196-8-git-send-email-mdroth@linux.vnet.ibm.com>
2012-05-01 21:54 ` [Qemu-devel] [PATCH 7/7] qdev: switch property accessors to fixed-width visitor interfaces Andreas Färber
2012-05-01 22:02 ` [Qemu-devel] [PATCH v5 0/7] add fixed-width visitors and serialization tests/fixes Andreas Färber
2012-05-11 1:22 ` Andreas Färber
2012-05-11 15:19 ` Michael Roth
[not found] ` <1335558083-26196-3-git-send-email-mdroth@linux.vnet.ibm.com>
2012-05-11 16:22 ` [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints Luiz Capitulino
2012-05-11 17:04 ` Michael Roth
2012-05-11 17:16 ` Andreas Färber
2012-05-11 17:34 ` Michael Roth
2012-05-11 17:38 ` Luiz Capitulino
[not found] ` <1335558083-26196-5-git-send-email-mdroth@linux.vnet.ibm.com>
2012-05-11 16:34 ` [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats Luiz Capitulino
2012-05-11 17:32 ` Michael Roth
2012-05-11 17:47 ` Andreas Färber
2012-03-28 23:02 [Qemu-devel] [PATCH v4 0/7] add fixed-width visitors and serialization tests Michael Roth
2012-03-28 23:02 ` [Qemu-devel] [PATCH 6/7] qdev: use int32_t container for devfn property Michael Roth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.