[Qemu-devel] [PATCH v4] hw/qdev-properties.c: Improve diagnostic for setting property after realize

* [Qemu-devel] [PATCH v4] hw/qdev-properties.c: Improve diagnostic for setting property after realize
@ 2013-03-25 13:40 Peter Maydell
  2013-03-25 18:36 ` Igor Mitsyanko
  2013-03-26 18:33 ` Anthony Liguori
  0 siblings, 2 replies; 3+ messages in thread
From: Peter Maydell @ 2013-03-25 13:40 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mitsyanko, Paolo Bonzini, Anthony Liguori,
	Andreas Färber, patches

Now we have error_setg() we can improve the error message emitted if
you attempt to set a property of a device after the device is realized
(the previous message was "permission denied" which was not very
informative).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Another attempt at updating this patch (v3 October, v2 July).
Changes v3->v4:
 * rebased; some functions have moved file, so the 'set the error'
   function can't be file-local. Renamed it to something more suitable
   for a public function and added a doc comment.
 * added set_taddr to the set of errors changed (and also checked we
   now have the complete set)

Not changed: Igor suggested using "after it was initialized" rather
than "after it was realized"; I think the original text is better,
because (a) we're moving towards having all devices implement realize
rather than init and (b) it's mostly an error message for QEMU developers
rather than end-users, so the technical terminology is not inappropriate.

 hw/qdev-addr.c              |    2 +-
 hw/qdev-properties-system.c |    4 ++--
 hw/qdev-properties.c        |   40 +++++++++++++++++++++++++++-------------
 hw/qdev-properties.h        |   12 ++++++++++++
 hw/qdev.c                   |    2 +-
 5 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 2398b4a..80a38bb 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -42,7 +42,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque,
     int64_t value;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index 8795144..28813d3 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -43,7 +43,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop,
     int ret;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -287,7 +287,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
     NetClientState *hubport;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 247ca6c..168c466 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -7,6 +7,20 @@
 #include "qapi/visitor.h"
 #include "char/char.h"
 
+void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
+                                  Error **errp)
+{
+    if (dev->id) {
+        error_setg(errp, "Attempt to set property '%s' on device '%s' "
+                   "(type '%s') after it was realized", name, dev->id,
+                   object_get_typename(OBJECT(dev)));
+    } else {
+        error_setg(errp, "Attempt to set property '%s' on anonymous device "
+                   "(type '%s') after it was realized", name,
+                   object_get_typename(OBJECT(dev)));
+    }
+}
+
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
 {
     void *ptr = dev;
@@ -33,7 +47,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque,
     int *ptr = qdev_get_prop_ptr(dev, prop);
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -86,7 +100,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque,
     bool value;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -126,7 +140,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque,
     uint8_t *ptr = qdev_get_prop_ptr(dev, prop);
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -193,7 +207,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque,
     uint16_t *ptr = qdev_get_prop_ptr(dev, prop);
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -226,7 +240,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque,
     uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -251,7 +265,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque,
     int32_t *ptr = qdev_get_prop_ptr(dev, prop);
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -324,7 +338,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque,
     uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -414,7 +428,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque,
     char *str;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -478,7 +492,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque,
     char *str, *p;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -570,7 +584,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque,
     char *str;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -641,7 +655,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
     const int64_t max = 32768;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -709,7 +723,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
     unsigned int slot = 0, func = 0;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
@@ -824,7 +838,7 @@ static void set_prop_arraylen(Object *obj, Visitor *v, void *opaque,
     int i;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
     if (*alenptr) {
diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
index c9bb246..a379339 100644
--- a/hw/qdev-properties.h
+++ b/hw/qdev-properties.h
@@ -167,4 +167,16 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
  */
 void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
 
+/**
+ * @qdev_prop_set_after_realize:
+ * @dev: device
+ * @name: name of property
+ * @errp: indirect pointer to Error to be set
+ * Set the Error object to report that an attempt was made to set a property
+ * on a device after it has already been realized. This is a utility function
+ * which allows property-setter functions to easily report the error in
+ * a friendly format identifying both the device and the property.
+ */
+void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
+                                 Error **errp);
 #endif
diff --git a/hw/qdev.c b/hw/qdev.c
index 909c405..6b1947e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -562,7 +562,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque,
     int ret;
 
     if (dev->realized) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        qdev_prop_set_after_realize(dev, name, errp);
         return;
     }
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 3+ messages in thread