Re: [Qemu-devel] [PATCH v2 6/6] qdev: switch property accessors to fixed-width visitor interfaces

2012-02-25 Thread Michael Roth 
On Fri, Feb 24, 2012 at 11:22:06AM -0600, Anthony Liguori wrote:
 According to git bisect and qemu-test, this breaks:
 
 qemu-system-x86_64 -kernel bin/vmlinuz-3.0 -initrd
 .tmp-26227/initramfs-26227.img.gz -append console=ttyS0 seed=1498
 -nographic -enable-kvm -device virtio-balloon-pci,id=balloon0
 -pidfile .tmp-26227/pidfile-26227.pid -qmp
 unix:.tmp-26227/qmpsock-26227.sock,server,nowait
 qemu-system-x86_64: Parameter 'id' expects int8_t
 Aborted

Sorry, put way too much faith in the unit tests catching this.

The issue is we currently use set_int* for both uint* and int*
properties. In this case the default uint8_t property value was
(uint8_t)-1 = 255, which we'd stick in a qobject and feed to the
visitors. Before, we'd just read that back into an int64_t container and
let it be re-interpreted as -1 or 255 depending on the property type.

Now, we still fall back to visit_type_int() for QmpInputVisitor, but in
the case of visit_type_int8() we check that the value falls within the
signed range, which isn't the case for 255.

There's a few other places where we hit similar issues. The 2 possible
solutions are:

1) Loosen the range checks in qapi-visit-core.c so that we ignore
signedness and only check that (uintX_t)value is small enough to fit
in X bytes, or

2) Add set_uint*/get_uint* accessors for uint* qdev properties.

1 is less code, and more forgiving of cases were we might use int*/uint*
interchangeably, but 2 I think is more correct and tightens up the
bounds checking for qdev and whatever else we use QmpInputVisitor for.
 
 Regards,
 
 Anthony Liguori
 
 On 02/23/2012 02:22 PM, Michael Roth wrote:
 Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
 ---
   hw/qdev-addr.c   |4 ++--
   hw/qdev-properties.c |   42 +-
   2 files changed, 19 insertions(+), 27 deletions(-)
 
 diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
 index 0bb16c7..b711b6b 100644
 --- a/hw/qdev-addr.c
 +++ b/hw/qdev-addr.c
 @@ -27,7 +27,7 @@ static void get_taddr(Object *obj, Visitor *v, void 
 *opaque,
   int64_t value;
 
   value = *ptr;
 -visit_type_int(v,value, name, errp);
 +visit_type_int64(v,value, name, errp);
   }
 
   static void set_taddr(Object *obj, Visitor *v, void *opaque,
 @@ -44,7 +44,7 @@ static void set_taddr(Object *obj, Visitor *v, void 
 *opaque,
   return;
   }
 
 -visit_type_int(v,value, name,local_err);
 +visit_type_int64(v,value, name,local_err);
   if (local_err) {
   error_propagate(errp, local_err);
   return;
 diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
 index 0423af1..98d95fb 100644
 --- a/hw/qdev-properties.c
 +++ b/hw/qdev-properties.c
 @@ -82,10 +82,8 @@ static void get_int8(Object *obj, Visitor *v, void 
 *opaque,
   DeviceState *dev = DEVICE(obj);
   Property *prop = opaque;
   int8_t *ptr = qdev_get_prop_ptr(dev, prop);
 -int64_t value;
 
 -value = *ptr;
 -visit_type_int(v,value, name, errp);
 +visit_type_int8(v, ptr, name, errp);
   }
 
   static void set_int8(Object *obj, Visitor *v, void *opaque,
 @@ -93,16 +91,15 @@ static void set_int8(Object *obj, Visitor *v, void 
 *opaque,
   {
   DeviceState *dev = DEVICE(obj);
   Property *prop = opaque;
 -int8_t *ptr = qdev_get_prop_ptr(dev, prop);
 +int8_t value, *ptr = qdev_get_prop_ptr(dev, prop);
   Error *local_err = NULL;
 -int64_t value;
 
   if (dev-state != DEV_STATE_CREATED) {
   error_set(errp, QERR_PERMISSION_DENIED);
   return;
   }
 
 -visit_type_int(v,value, name,local_err);
 +visit_type_int8(v,value, name,local_err);
   if (local_err) {
   error_propagate(errp, local_err);
   return;
 @@ -111,7 +108,7 @@ static void set_int8(Object *obj, Visitor *v, void 
 *opaque,
   *ptr = value;
   } else {
   error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
 -  dev-id?:, name, value, prop-info-min,
 +  dev-id?:, name, (int64_t)value, prop-info-min,
 prop-info-max);
   }
   }
 @@ -168,10 +165,8 @@ static void get_int16(Object *obj, Visitor *v, void 
 *opaque,
   DeviceState *dev = DEVICE(obj);
   Property *prop = opaque;
   int16_t *ptr = qdev_get_prop_ptr(dev, prop);
 -int64_t value;
 
 -value = *ptr;
 -visit_type_int(v,value, name, errp);
 +visit_type_int16(v, ptr, name, errp);
   }
 
   static void set_int16(Object *obj, Visitor *v, void *opaque,
 @@ -179,16 +174,15 @@ static void set_int16(Object *obj, Visitor *v, void 
 *opaque,
   {
   DeviceState *dev = DEVICE(obj);
   Property *prop = opaque;
 -int16_t *ptr = qdev_get_prop_ptr(dev, prop);
 +int16_t value, *ptr = qdev_get_prop_ptr(dev, prop);
   Error *local_err = NULL;
 -int64_t value;
 
   if (dev-state != DEV_STATE_CREATED) {
   error_set(errp, QERR_PERMISSION_DENIED);
   return;
   }
 
 -visit_type_int(v,value,

Re: [Qemu-devel] [PATCH v2 6/6] qdev: switch property accessors to fixed-width visitor interfaces

2012-02-25 Thread Andreas Färber 
Am 25.02.2012 16:41, schrieb Michael Roth:
 On Fri, Feb 24, 2012 at 11:22:06AM -0600, Anthony Liguori wrote:
 According to git bisect and qemu-test, this breaks:

 qemu-system-x86_64 -kernel bin/vmlinuz-3.0 -initrd
 .tmp-26227/initramfs-26227.img.gz -append console=ttyS0 seed=1498
 -nographic -enable-kvm -device virtio-balloon-pci,id=balloon0
 -pidfile .tmp-26227/pidfile-26227.pid -qmp
 unix:.tmp-26227/qmpsock-26227.sock,server,nowait
 qemu-system-x86_64: Parameter 'id' expects int8_t
 Aborted
 
 Sorry, put way too much faith in the unit tests catching this.
 
 The issue is we currently use set_int* for both uint* and int*
 properties. In this case the default uint8_t property value was
 (uint8_t)-1 = 255, which we'd stick in a qobject and feed to the
 visitors. Before, we'd just read that back into an int64_t container and
 let it be re-interpreted as -1 or 255 depending on the property type.
 
 Now, we still fall back to visit_type_int() for QmpInputVisitor, but in
 the case of visit_type_int8() we check that the value falls within the
 signed range, which isn't the case for 255.
 
 There's a few other places where we hit similar issues. The 2 possible
 solutions are:
 
 1) Loosen the range checks in qapi-visit-core.c so that we ignore
 signedness and only check that (uintX_t)value is small enough to fit
 in X bytes, or
 
 2) Add set_uint*/get_uint* accessors for uint* qdev properties.
 
 1 is less code, and more forgiving of cases were we might use int*/uint*
 interchangeably, but 2 I think is more correct and tightens up the
 bounds checking for qdev and whatever else we use QmpInputVisitor for.

I'm not too deep into visitors yet but 2) sounds better to me.

I've seen a couple of places where command line args are not properly
checked before passing them on (will send patches for those I remember)
so having the checks close to where the values came from sounds good.

Paolo did provide separate object_property_set_[u]int* accessors so we
should be good in QOM land when not fiddling with these things at such a
deep level.

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

2012-02-25 Thread Paolo Bonzini 
On 02/25/2012 05:08 PM, Andreas Färber wrote:
 Paolo did provide separate object_property_set_[u]int* accessors so we
 should be good in QOM land when not fiddling with these things at such a
 deep level.

There is only object_property_set_int right now, but it'd be great to
add the others.

Paolo

Re: [Qemu-devel] [PATCH v2 6/6] qdev: switch property accessors to fixed-width visitor interfaces

2012-02-24 Thread Anthony Liguori 

According to git bisect and qemu-test, this breaks:

qemu-system-x86_64 -kernel bin/vmlinuz-3.0 -initrd 
.tmp-26227/initramfs-26227.img.gz -append console=ttyS0 seed=1498 -nographic 
-enable-kvm -device virtio-balloon-pci,id=balloon0 -pidfile 
.tmp-26227/pidfile-26227.pid -qmp unix:.tmp-26227/qmpsock-26227.sock,server,nowait

qemu-system-x86_64: Parameter 'id' expects int8_t
Aborted

Regards,

Anthony Liguori

On 02/23/2012 02:22 PM, Michael Roth wrote:

Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
  hw/qdev-addr.c   |4 ++--
  hw/qdev-properties.c |   42 +-
  2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 0bb16c7..b711b6b 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -27,7 +27,7 @@ static void get_taddr(Object *obj, Visitor *v, void *opaque,
  int64_t value;

  value = *ptr;
-visit_type_int(v,value, name, errp);
+visit_type_int64(v,value, name, errp);
  }

  static void set_taddr(Object *obj, Visitor *v, void *opaque,
@@ -44,7 +44,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque,
  return;
  }

-visit_type_int(v,value, name,local_err);
+visit_type_int64(v,value, name,local_err);
  if (local_err) {
  error_propagate(errp, local_err);
  return;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 0423af1..98d95fb 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -82,10 +82,8 @@ static void get_int8(Object *obj, Visitor *v, void *opaque,
  DeviceState *dev = DEVICE(obj);
  Property *prop = opaque;
  int8_t *ptr = qdev_get_prop_ptr(dev, prop);
-int64_t value;

-value = *ptr;
-visit_type_int(v,value, name, errp);
+visit_type_int8(v, ptr, name, errp);
  }

  static void set_int8(Object *obj, Visitor *v, void *opaque,
@@ -93,16 +91,15 @@ static void set_int8(Object *obj, Visitor *v, void *opaque,
  {
  DeviceState *dev = DEVICE(obj);
  Property *prop = opaque;
-int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+int8_t value, *ptr = qdev_get_prop_ptr(dev, prop);
  Error *local_err = NULL;
-int64_t value;

  if (dev-state != DEV_STATE_CREATED) {
  error_set(errp, QERR_PERMISSION_DENIED);
  return;
  }

-visit_type_int(v,value, name,local_err);
+visit_type_int8(v,value, name,local_err);
  if (local_err) {
  error_propagate(errp, local_err);
  return;
@@ -111,7 +108,7 @@ static void set_int8(Object *obj, Visitor *v, void *opaque,
  *ptr = value;
  } else {
  error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-  dev-id?:, name, value, prop-info-min,
+  dev-id?:, name, (int64_t)value, prop-info-min,
prop-info-max);
  }
  }
@@ -168,10 +165,8 @@ static void get_int16(Object *obj, Visitor *v, void 
*opaque,
  DeviceState *dev = DEVICE(obj);
  Property *prop = opaque;
  int16_t *ptr = qdev_get_prop_ptr(dev, prop);
-int64_t value;

-value = *ptr;
-visit_type_int(v,value, name, errp);
+visit_type_int16(v, ptr, name, errp);
  }

  static void set_int16(Object *obj, Visitor *v, void *opaque,
@@ -179,16 +174,15 @@ static void set_int16(Object *obj, Visitor *v, void 
*opaque,
  {
  DeviceState *dev = DEVICE(obj);
  Property *prop = opaque;
-int16_t *ptr = qdev_get_prop_ptr(dev, prop);
+int16_t value, *ptr = qdev_get_prop_ptr(dev, prop);
  Error *local_err = NULL;
-int64_t value;

  if (dev-state != DEV_STATE_CREATED) {
  error_set(errp, QERR_PERMISSION_DENIED);
  return;
  }

-visit_type_int(v,value, name,local_err);
+visit_type_int16(v,value, name,local_err);
  if (local_err) {
  error_propagate(errp, local_err);
  return;
@@ -197,7 +191,7 @@ static void set_int16(Object *obj, Visitor *v, void *opaque,
  *ptr = value;
  } else {
  error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-  dev-id?:, name, value, prop-info-min,
+  dev-id?:, name, (int64_t)value, prop-info-min,
prop-info-max);
  }
  }
@@ -217,11 +211,10 @@ static void get_int32(Object *obj, Visitor *v, void 
*opaque,
  {
  DeviceState *dev = DEVICE(obj);
  Property *prop = opaque;
-int32_t *ptr = qdev_get_prop_ptr(dev, prop);
-int64_t value;
+int32_t value, *ptr = qdev_get_prop_ptr(dev, prop);

  value = *ptr;
-visit_type_int(v,value, name, errp);
+visit_type_int32(v,value, name, errp);
  }

  static void set_int32(Object *obj, Visitor *v, void *opaque,
@@ -229,16 +222,15 @@ static void set_int32(Object *obj, Visitor *v, void 
*opaque,
  {
  DeviceState *dev = DEVICE(obj);
  Property *prop = opaque;
-int32_t *ptr = qdev_get_prop_ptr(dev, prop);
+int32_t value, *ptr = qdev_get_prop_ptr(dev, prop);
  Error *local_err = NULL;
-int64_t value;

  if

[Qemu-devel] [PATCH v2 6/6] qdev: switch property accessors to fixed-width visitor interfaces

2012-02-23 Thread Michael Roth 

Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
 hw/qdev-addr.c   |4 ++--
 hw/qdev-properties.c |   42 +-
 2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 0bb16c7..b711b6b 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -27,7 +27,7 @@ static void get_taddr(Object *obj, Visitor *v, void *opaque,
 int64_t value;
 
 value = *ptr;
-visit_type_int(v, value, name, errp);
+visit_type_int64(v, value, name, errp);
 }
 
 static void set_taddr(Object *obj, Visitor *v, void *opaque,
@@ -44,7 +44,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque,
 return;
 }
 
-visit_type_int(v, value, name, local_err);
+visit_type_int64(v, value, name, local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 0423af1..98d95fb 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -82,10 +82,8 @@ static void get_int8(Object *obj, Visitor *v, void *opaque,
 DeviceState *dev = DEVICE(obj);
 Property *prop = opaque;
 int8_t *ptr = qdev_get_prop_ptr(dev, prop);
-int64_t value;
 
-value = *ptr;
-visit_type_int(v, value, name, errp);
+visit_type_int8(v, ptr, name, errp);
 }
 
 static void set_int8(Object *obj, Visitor *v, void *opaque,
@@ -93,16 +91,15 @@ static void set_int8(Object *obj, Visitor *v, void *opaque,
 {
 DeviceState *dev = DEVICE(obj);
 Property *prop = opaque;
-int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+int8_t value, *ptr = qdev_get_prop_ptr(dev, prop);
 Error *local_err = NULL;
-int64_t value;
 
 if (dev-state != DEV_STATE_CREATED) {
 error_set(errp, QERR_PERMISSION_DENIED);
 return;
 }
 
-visit_type_int(v, value, name, local_err);
+visit_type_int8(v, value, name, local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
@@ -111,7 +108,7 @@ static void set_int8(Object *obj, Visitor *v, void *opaque,
 *ptr = value;
 } else {
 error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-  dev-id?:, name, value, prop-info-min,
+  dev-id?:, name, (int64_t)value, prop-info-min,
   prop-info-max);
 }
 }
@@ -168,10 +165,8 @@ static void get_int16(Object *obj, Visitor *v, void 
*opaque,
 DeviceState *dev = DEVICE(obj);
 Property *prop = opaque;
 int16_t *ptr = qdev_get_prop_ptr(dev, prop);
-int64_t value;
 
-value = *ptr;
-visit_type_int(v, value, name, errp);
+visit_type_int16(v, ptr, name, errp);
 }
 
 static void set_int16(Object *obj, Visitor *v, void *opaque,
@@ -179,16 +174,15 @@ static void set_int16(Object *obj, Visitor *v, void 
*opaque,
 {
 DeviceState *dev = DEVICE(obj);
 Property *prop = opaque;
-int16_t *ptr = qdev_get_prop_ptr(dev, prop);
+int16_t value, *ptr = qdev_get_prop_ptr(dev, prop);
 Error *local_err = NULL;
-int64_t value;
 
 if (dev-state != DEV_STATE_CREATED) {
 error_set(errp, QERR_PERMISSION_DENIED);
 return;
 }
 
-visit_type_int(v, value, name, local_err);
+visit_type_int16(v, value, name, local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
@@ -197,7 +191,7 @@ static void set_int16(Object *obj, Visitor *v, void *opaque,
 *ptr = value;
 } else {
 error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-  dev-id?:, name, value, prop-info-min,
+  dev-id?:, name, (int64_t)value, prop-info-min,
   prop-info-max);
 }
 }
@@ -217,11 +211,10 @@ static void get_int32(Object *obj, Visitor *v, void 
*opaque,
 {
 DeviceState *dev = DEVICE(obj);
 Property *prop = opaque;
-int32_t *ptr = qdev_get_prop_ptr(dev, prop);
-int64_t value;
+int32_t value, *ptr = qdev_get_prop_ptr(dev, prop);
 
 value = *ptr;
-visit_type_int(v, value, name, errp);
+visit_type_int32(v, value, name, errp);
 }
 
 static void set_int32(Object *obj, Visitor *v, void *opaque,
@@ -229,16 +222,15 @@ static void set_int32(Object *obj, Visitor *v, void 
*opaque,
 {
 DeviceState *dev = DEVICE(obj);
 Property *prop = opaque;
-int32_t *ptr = qdev_get_prop_ptr(dev, prop);
+int32_t value, *ptr = qdev_get_prop_ptr(dev, prop);
 Error *local_err = NULL;
-int64_t value;
 
 if (dev-state != DEV_STATE_CREATED) {
 error_set(errp, QERR_PERMISSION_DENIED);
 return;
 }
 
-visit_type_int(v, value, name, local_err);
+visit_type_int32(v, value, name, local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
@@ -247,7 +239,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque,
 *ptr = value;
 } else {
 error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-  dev-id?:,