[PATCH v2 0/4] Improve default object property_add uint helpers

[PATCH v2 0/4] Improve default object property_add uint helpers

[Felipe Franciosi]

This improves the family of object_property_add_uintXX_ptr helpers by enabling
a default getter/setter only when desired. To prevent an API behavioural change
(from clients that already used these helpers and did not want a setter), we
add a OBJ_PROP_FLAG_RD flag that allow clients to only have a getter. Patch 1
enhances the API and modify current users.

While modifying the clients of the API, a couple of improvement opportunities
were observed in ich9. These were added in separate patches (2 and 3).

Patch 3 cleans up a lot of existing code by moving various objects to the
enhanced API. Previously, those objects had their own getters/setters that only
updated the values without further checks. Some of them actually lacked a check
for setting overflows, which could have resulted in undesired values being set.
The new default setters include a check for that, not updating the values in
case of errors (and propagating them). If they did not provide an error
pointer, then that behaviour was maintained.


Felipe Franciosi (4):
  qom/object: enable setter for uint types
  ich9: fix getter type for sci_int property
  ich9: Simplify ich9_lpc_initfn
  qom/object: Use common get/set uint helpers

 hw/acpi/ich9.c       | 103 +++------------------
 hw/acpi/pcihp.c      |   7 +-
 hw/acpi/piix4.c      |  12 +--
 hw/isa/lpc_ich9.c    |  28 ++----
 hw/misc/edu.c        |  14 +--
 hw/pci-host/q35.c    |  14 +--
 hw/ppc/spapr.c       |  19 +---
 hw/ppc/spapr_drc.c   |   2 +-
 hw/vfio/pci-quirks.c |  20 ++--
 include/qom/object.h |  42 +++++++--
 memory.c             |  15 +--
 qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
 target/arm/cpu.c     |  23 +----
 target/i386/sev.c    | 106 ++-------------------
 ui/console.c         |   4 +-
 15 files changed, 293 insertions(+), 332 deletions(-)

-- 
2.20.1

Changelog:
- Update sci_int directly instead of using stack variable
- Defining an enhanced ObjectPropertyFlags instead of just 'readonly'
- Erroring out directly (instead of using gotos) on default setters
- Retaining lack of errp passing when it wasn't there

[PATCH v2 1/4] qom/object: enable setter for uint types

[Felipe Franciosi]

Traditionally, the uint-specific property helpers only offer getters.
When adding object (or class) uint types, one must therefore use the
generic property helper if a setter is needed (and probably duplicate
some code writing their own getters/setters).

This enhances the uint-specific property helper APIs by adding a
bitwise-or'd 'flags' field and modifying all clients of that API to set
this paramater to OBJ_PROP_FLAG_RD. This maintains the current behaviour
whilst allowing others to also set OBJ_PROP_FLAG_WR in the future (which
will automatically install a setter). Other flags may be added later.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 hw/acpi/ich9.c       |   4 +-
 hw/acpi/pcihp.c      |   7 +-
 hw/acpi/piix4.c      |  12 +--
 hw/isa/lpc_ich9.c    |   4 +-
 hw/ppc/spapr_drc.c   |   2 +-
 include/qom/object.h |  42 +++++++--
 qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
 ui/console.c         |   4 +-
 8 files changed, 243 insertions(+), 48 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2034dd749e..236300d2a9 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
     pm->s4_val = 2;
 
     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
-                                   &pm->pm_io_base, errp);
+                                   &pm->pm_io_base, OBJ_PROP_FLAG_RD, errp);
     object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
                         ich9_pm_get_gpe0_blk,
                         NULL, NULL, pm, NULL);
     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
-                                   &gpe0_len, errp);
+                                   &gpe0_len, OBJ_PROP_FLAG_RD, errp);
     object_property_add_bool(obj, "memory-hotplug-support",
                              ich9_pm_get_memory_hotplug_support,
                              ich9_pm_set_memory_hotplug_support,
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 8413348a33..c8a7194b19 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
 
         *bus_bsel = (*bsel_alloc)++;
         object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
-                                       bus_bsel, &error_abort);
+                                       bus_bsel, OBJ_PROP_FLAG_RD,
+                                       &error_abort);
     }
 
     return bsel_alloc;
@@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
     memory_region_add_subregion(address_space_io, s->io_base, &s->io);
 
     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
-                                   &error_abort);
+                                   OBJ_PROP_FLAG_RD, &error_abort);
     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
-                                   &error_abort);
+                                   OBJ_PROP_FLAG_RD, &error_abort);
 }
 
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93aec2dd2c..06d964a840 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
     static const uint16_t sci_int = 9;
 
     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-                                  &acpi_enable_cmd, NULL);
+                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
-                                  &acpi_disable_cmd, NULL);
+                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
-                                  &gpe0_blk, NULL);
+                                  &gpe0_blk, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
-                                  &gpe0_blk_len, NULL);
+                                  &gpe0_blk_len, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
-                                  &sci_int, NULL);
+                                  &sci_int, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
-                                  &s->io_base, NULL);
+                                  &s->io_base, OBJ_PROP_FLAG_RD, NULL);
 }
 
 static void piix4_pm_realize(PCIDevice *dev, Error **errp)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17c292e306..f5526f9c3b 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
                         ich9_lpc_get_sci_int,
                         NULL, NULL, NULL, NULL);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-                                  &acpi_enable_cmd, NULL);
+                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
-                                  &acpi_disable_cmd, NULL);
+                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
 
     ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
 }
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 62f1a42592..ace2db0413 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -553,7 +553,7 @@ static void spapr_dr_connector_instance_init(Object *obj)
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
-    object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
+    object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_RD, NULL);
     object_property_add(obj, "index", "uint32", prop_get_index,
                         NULL, NULL, NULL, NULL);
     object_property_add(obj, "fdt", "struct", prop_get_fdt,
diff --git a/include/qom/object.h b/include/qom/object.h
index 128d00c77f..4836c54e93 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1579,65 +1579,91 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
                                   void (*get)(Object *, struct tm *, Error **),
                                   Error **errp);
 
+typedef enum {
+    /* Automatically add a getter to the property */
+    OBJ_PROP_FLAG_RD = (1UL << 0),
+    /* Automatically add a setter to the property */
+    OBJ_PROP_FLAG_WR = (1UL << 1),
+} ObjectPropertyFlags;
+
 /**
  * object_property_add_uint8_ptr:
  * @obj: the object to add a property to
  * @name: the name of the property
  * @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
  * @errp: if an error occurs, a pointer to an area to store the error
  *
  * Add an integer property in memory.  This function will add a
  * property of type 'uint8'.
  */
 void object_property_add_uint8_ptr(Object *obj, const char *name,
-                                   const uint8_t *v, Error **errp);
+                                   const uint8_t *v, ObjectPropertyFlags flags,
+                                   Error **errp);
 void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
-                                         const uint8_t *v, Error **errp);
+                                         const uint8_t *v,
+                                         ObjectPropertyFlags flags,
+                                         Error **errp);
 
 /**
  * object_property_add_uint16_ptr:
  * @obj: the object to add a property to
  * @name: the name of the property
  * @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
  * @errp: if an error occurs, a pointer to an area to store the error
  *
  * Add an integer property in memory.  This function will add a
  * property of type 'uint16'.
  */
 void object_property_add_uint16_ptr(Object *obj, const char *name,
-                                    const uint16_t *v, Error **errp);
+                                    const uint16_t *v,
+                                    ObjectPropertyFlags flags,
+                                    Error **errp);
 void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
-                                          const uint16_t *v, Error **errp);
+                                          const uint16_t *v,
+                                          ObjectPropertyFlags flags,
+                                          Error **errp);
 
 /**
  * object_property_add_uint32_ptr:
  * @obj: the object to add a property to
  * @name: the name of the property
  * @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
  * @errp: if an error occurs, a pointer to an area to store the error
  *
  * Add an integer property in memory.  This function will add a
  * property of type 'uint32'.
  */
 void object_property_add_uint32_ptr(Object *obj, const char *name,
-                                    const uint32_t *v, Error **errp);
+                                    const uint32_t *v,
+                                    ObjectPropertyFlags flags,
+                                    Error **errp);
 void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
-                                          const uint32_t *v, Error **errp);
+                                          const uint32_t *v,
+                                          ObjectPropertyFlags flags,
+                                          Error **errp);
 
 /**
  * object_property_add_uint64_ptr:
  * @obj: the object to add a property to
  * @name: the name of the property
  * @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
  * @errp: if an error occurs, a pointer to an area to store the error
  *
  * Add an integer property in memory.  This function will add a
  * property of type 'uint64'.
  */
 void object_property_add_uint64_ptr(Object *obj, const char *name,
-                                    const uint64_t *v, Error **Errp);
+                                    const uint64_t *v,
+                                    ObjectPropertyFlags flags,
+                                    Error **Errp);
 void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
-                                          const uint64_t *v, Error **Errp);
+                                          const uint64_t *v,
+                                          ObjectPropertyFlags flags,
+                                          Error **Errp);
 
 /**
  * object_property_add_alias:
diff --git a/qom/object.c b/qom/object.c
index d51b57fba1..6f300b5317 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
     visit_type_uint8(v, name, &value, errp);
 }
 
+static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    uint8_t *field = opaque;
+    uint8_t value;
+    Error *local_err = NULL;
+
+    visit_type_uint8(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    *field = value;
+}
+
 static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
                                     void *opaque, Error **errp)
 {
@@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
     visit_type_uint16(v, name, &value, errp);
 }
 
+static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
+                                    void *opaque, Error **errp)
+{
+    uint16_t *field = opaque;
+    uint16_t value;
+    Error *local_err = NULL;
+
+    visit_type_uint16(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    *field = value;
+}
+
 static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
                                     void *opaque, Error **errp)
 {
@@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
     visit_type_uint32(v, name, &value, errp);
 }
 
+static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
+                                    void *opaque, Error **errp)
+{
+    uint32_t *field = opaque;
+    uint32_t value;
+    Error *local_err = NULL;
+
+    visit_type_uint32(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    *field = value;
+}
+
 static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
                                     void *opaque, Error **errp)
 {
@@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
     visit_type_uint64(v, name, &value, errp);
 }
 
+static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
+                                    void *opaque, Error **errp)
+{
+    uint64_t *field = opaque;
+    uint64_t value;
+    Error *local_err = NULL;
+
+    visit_type_uint64(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    *field = value;
+}
+
 void object_property_add_uint8_ptr(Object *obj, const char *name,
-                                   const uint8_t *v, Error **errp)
+                                   const uint8_t *v,
+                                   ObjectPropertyFlags flags,
+                                   Error **errp)
 {
-    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
-                        NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint8_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint8_ptr;
+    }
+
+    object_property_add(obj, name, "uint8",
+                        getter, setter, NULL, (void *)v, errp);
 }
 
 void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
-                                         const uint8_t *v, Error **errp)
+                                         const uint8_t *v,
+                                         ObjectPropertyFlags flags,
+                                         Error **errp)
 {
-    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
-                              NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint8_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint8_ptr;
+    }
+
+    object_class_property_add(klass, name, "uint8",
+                              getter, setter, NULL, (void *)v, errp);
 }
 
 void object_property_add_uint16_ptr(Object *obj, const char *name,
-                                    const uint16_t *v, Error **errp)
+                                    const uint16_t *v,
+                                    ObjectPropertyFlags flags,
+                                    Error **errp)
 {
-    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
-                        NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint16_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint16_ptr;
+    }
+
+    object_property_add(obj, name, "uint16",
+                        getter, setter, NULL, (void *)v, errp);
 }
 
 void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
-                                          const uint16_t *v, Error **errp)
+                                          const uint16_t *v,
+                                          ObjectPropertyFlags flags,
+                                          Error **errp)
 {
-    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
-                              NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint16_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint16_ptr;
+    }
+
+    object_class_property_add(klass, name, "uint16",
+                              getter, setter, NULL, (void *)v, errp);
 }
 
 void object_property_add_uint32_ptr(Object *obj, const char *name,
-                                    const uint32_t *v, Error **errp)
+                                    const uint32_t *v,
+                                    ObjectPropertyFlags flags,
+                                    Error **errp)
 {
-    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
-                        NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint32_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint32_ptr;
+    }
+
+    object_property_add(obj, name, "uint32",
+                        getter, setter, NULL, (void *)v, errp);
 }
 
 void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
-                                          const uint32_t *v, Error **errp)
+                                          const uint32_t *v,
+                                          ObjectPropertyFlags flags,
+                                          Error **errp)
 {
-    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
-                              NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint32_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint32_ptr;
+    }
+
+    object_class_property_add(klass, name, "uint32",
+                              getter, setter, NULL, (void *)v, errp);
 }
 
 void object_property_add_uint64_ptr(Object *obj, const char *name,
-                                    const uint64_t *v, Error **errp)
+                                    const uint64_t *v,
+                                    ObjectPropertyFlags flags,
+                                    Error **errp)
 {
-    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
-                        NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint64_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint64_ptr;
+    }
+
+    object_property_add(obj, name, "uint64",
+                        getter, setter, NULL, (void *)v, errp);
 }
 
 void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
-                                          const uint64_t *v, Error **errp)
+                                          const uint64_t *v,
+                                          ObjectPropertyFlags flags,
+                                          Error **errp)
 {
-    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
-                              NULL, NULL, (void *)v, errp);
+    ObjectPropertyAccessor *getter = NULL;
+    ObjectPropertyAccessor *setter = NULL;
+
+    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
+        getter = property_get_uint64_ptr;
+    }
+
+    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
+        setter = property_set_uint64_ptr;
+    }
+
+    object_class_property_add(klass, name, "uint64",
+                              getter, setter, NULL, (void *)v, errp);
 }
 
 typedef struct {
diff --git a/ui/console.c b/ui/console.c
index 82d1ddac9c..7d6ef90978 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
                              object_property_allow_set_link,
                              OBJ_PROP_LINK_STRONG,
                              &error_abort);
-    object_property_add_uint32_ptr(obj, "head",
-                                   &s->head, &error_abort);
+    object_property_add_uint32_ptr(obj, "head", &s->head,
+                                   OBJ_PROP_FLAG_RD, &error_abort);
 
     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
         (console_type == GRAPHIC_CONSOLE))) {
-- 
2.20.1

Re: [PATCH v2 1/4] qom/object: enable setter for uint types

[Marc-André Lureau]

Hi

On Thu, Nov 28, 2019 at 8:48 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Traditionally, the uint-specific property helpers only offer getters.
> When adding object (or class) uint types, one must therefore use the
> generic property helper if a setter is needed (and probably duplicate
> some code writing their own getters/setters).
>
> This enhances the uint-specific property helper APIs by adding a
> bitwise-or'd 'flags' field and modifying all clients of that API to set
> this paramater to OBJ_PROP_FLAG_RD. This maintains the current behaviour
> whilst allowing others to also set OBJ_PROP_FLAG_WR in the future (which
> will automatically install a setter). Other flags may be added later.

For readability, I would have the full spelling:
OBJECT_PROPERTY_FLAG_READ/OBJECT_PROPERTY_FLAG_WRITE (and an alias
OBJECT_PROPERTY_FLAG_READWRITE = READ|WRITE)

>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  hw/acpi/ich9.c       |   4 +-
>  hw/acpi/pcihp.c      |   7 +-
>  hw/acpi/piix4.c      |  12 +--
>  hw/isa/lpc_ich9.c    |   4 +-
>  hw/ppc/spapr_drc.c   |   2 +-
>  include/qom/object.h |  42 +++++++--
>  qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
>  ui/console.c         |   4 +-
>  8 files changed, 243 insertions(+), 48 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2034dd749e..236300d2a9 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>      pm->s4_val = 2;
>
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> -                                   &pm->pm_io_base, errp);
> +                                   &pm->pm_io_base, OBJ_PROP_FLAG_RD, errp);
>      object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
>                          ich9_pm_get_gpe0_blk,
>                          NULL, NULL, pm, NULL);
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                   &gpe0_len, errp);
> +                                   &gpe0_len, OBJ_PROP_FLAG_RD, errp);
>      object_property_add_bool(obj, "memory-hotplug-support",
>                               ich9_pm_get_memory_hotplug_support,
>                               ich9_pm_set_memory_hotplug_support,
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 8413348a33..c8a7194b19 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>
>          *bus_bsel = (*bsel_alloc)++;
>          object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> -                                       bus_bsel, &error_abort);
> +                                       bus_bsel, OBJ_PROP_FLAG_RD,
> +                                       &error_abort);
>      }
>
>      return bsel_alloc;
> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
>      memory_region_add_subregion(address_space_io, s->io_base, &s->io);
>
>      object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
> -                                   &error_abort);
> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>      object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
> -                                   &error_abort);
> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>  }
>
>  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 93aec2dd2c..06d964a840 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
>      static const uint16_t sci_int = 9;
>
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> -                                  &acpi_enable_cmd, NULL);
> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> -                                  &acpi_disable_cmd, NULL);
> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> -                                  &gpe0_blk, NULL);
> +                                  &gpe0_blk, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                  &gpe0_blk_len, NULL);
> +                                  &gpe0_blk_len, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> -                                  &sci_int, NULL);
> +                                  &sci_int, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> -                                  &s->io_base, NULL);
> +                                  &s->io_base, OBJ_PROP_FLAG_RD, NULL);
>  }
>
>  static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 17c292e306..f5526f9c3b 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>                          ich9_lpc_get_sci_int,
>                          NULL, NULL, NULL, NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> -                                  &acpi_enable_cmd, NULL);
> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> -                                  &acpi_disable_cmd, NULL);
> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
>
>      ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
>  }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 62f1a42592..ace2db0413 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -553,7 +553,7 @@ static void spapr_dr_connector_instance_init(Object *obj)
>      SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
>      SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> -    object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
> +    object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_RD, NULL);
>      object_property_add(obj, "index", "uint32", prop_get_index,
>                          NULL, NULL, NULL, NULL);
>      object_property_add(obj, "fdt", "struct", prop_get_fdt,
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 128d00c77f..4836c54e93 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1579,65 +1579,91 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
>                                    void (*get)(Object *, struct tm *, Error **),
>                                    Error **errp);
>
> +typedef enum {
> +    /* Automatically add a getter to the property */
> +    OBJ_PROP_FLAG_RD = (1UL << 0),
> +    /* Automatically add a setter to the property */
> +    OBJ_PROP_FLAG_WR = (1UL << 1),
> +} ObjectPropertyFlags;
> +
>  /**
>   * object_property_add_uint8_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint8'.
>   */
>  void object_property_add_uint8_ptr(Object *obj, const char *name,
> -                                   const uint8_t *v, Error **errp);
> +                                   const uint8_t *v, ObjectPropertyFlags flags,
> +                                   Error **errp);
>  void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> -                                         const uint8_t *v, Error **errp);
> +                                         const uint8_t *v,
> +                                         ObjectPropertyFlags flags,
> +                                         Error **errp);
>
>  /**
>   * object_property_add_uint16_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint16'.
>   */
>  void object_property_add_uint16_ptr(Object *obj, const char *name,
> -                                    const uint16_t *v, Error **errp);
> +                                    const uint16_t *v,
> +                                    ObjectPropertyFlags flags,
> +                                    Error **errp);
>  void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> -                                          const uint16_t *v, Error **errp);
> +                                          const uint16_t *v,
> +                                          ObjectPropertyFlags flags,
> +                                          Error **errp);
>
>  /**
>   * object_property_add_uint32_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint32'.
>   */
>  void object_property_add_uint32_ptr(Object *obj, const char *name,
> -                                    const uint32_t *v, Error **errp);
> +                                    const uint32_t *v,
> +                                    ObjectPropertyFlags flags,
> +                                    Error **errp);
>  void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> -                                          const uint32_t *v, Error **errp);
> +                                          const uint32_t *v,
> +                                          ObjectPropertyFlags flags,
> +                                          Error **errp);
>
>  /**
>   * object_property_add_uint64_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint64'.
>   */
>  void object_property_add_uint64_ptr(Object *obj, const char *name,
> -                                    const uint64_t *v, Error **Errp);
> +                                    const uint64_t *v,
> +                                    ObjectPropertyFlags flags,
> +                                    Error **Errp);
>  void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> -                                          const uint64_t *v, Error **Errp);
> +                                          const uint64_t *v,
> +                                          ObjectPropertyFlags flags,
> +                                          Error **Errp);
>
>  /**
>   * object_property_add_alias:
> diff --git a/qom/object.c b/qom/object.c
> index d51b57fba1..6f300b5317 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
>      visit_type_uint8(v, name, &value, errp);
>  }
>
> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    uint8_t *field = opaque;
> +    uint8_t value;
> +    Error *local_err = NULL;
> +
> +    visit_type_uint8(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    *field = value;
> +}
> +
>  static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>                                      void *opaque, Error **errp)
>  {
> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>      visit_type_uint16(v, name, &value, errp);
>  }
>
> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
> +                                    void *opaque, Error **errp)
> +{
> +    uint16_t *field = opaque;
> +    uint16_t value;
> +    Error *local_err = NULL;
> +
> +    visit_type_uint16(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    *field = value;
> +}
> +
>  static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>                                      void *opaque, Error **errp)
>  {
> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>      visit_type_uint32(v, name, &value, errp);
>  }
>
> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
> +                                    void *opaque, Error **errp)
> +{
> +    uint32_t *field = opaque;
> +    uint32_t value;
> +    Error *local_err = NULL;
> +
> +    visit_type_uint32(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    *field = value;
> +}
> +
>  static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>                                      void *opaque, Error **errp)
>  {
> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>      visit_type_uint64(v, name, &value, errp);
>  }
>
> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
> +                                    void *opaque, Error **errp)
> +{
> +    uint64_t *field = opaque;
> +    uint64_t value;
> +    Error *local_err = NULL;
> +
> +    visit_type_uint64(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    *field = value;
> +}
> +
>  void object_property_add_uint8_ptr(Object *obj, const char *name,
> -                                   const uint8_t *v, Error **errp)
> +                                   const uint8_t *v,
> +                                   ObjectPropertyFlags flags,
> +                                   Error **errp)
>  {
> -    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> -                        NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint8_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint8_ptr;
> +    }
> +
> +    object_property_add(obj, name, "uint8",
> +                        getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> -                                         const uint8_t *v, Error **errp)
> +                                         const uint8_t *v,
> +                                         ObjectPropertyFlags flags,
> +                                         Error **errp)
>  {
> -    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
> -                              NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint8_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint8_ptr;
> +    }
> +
> +    object_class_property_add(klass, name, "uint8",
> +                              getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_property_add_uint16_ptr(Object *obj, const char *name,
> -                                    const uint16_t *v, Error **errp)
> +                                    const uint16_t *v,
> +                                    ObjectPropertyFlags flags,
> +                                    Error **errp)
>  {
> -    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> -                        NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint16_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint16_ptr;
> +    }
> +
> +    object_property_add(obj, name, "uint16",
> +                        getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> -                                          const uint16_t *v, Error **errp)
> +                                          const uint16_t *v,
> +                                          ObjectPropertyFlags flags,
> +                                          Error **errp)
>  {
> -    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
> -                              NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint16_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint16_ptr;
> +    }
> +
> +    object_class_property_add(klass, name, "uint16",
> +                              getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_property_add_uint32_ptr(Object *obj, const char *name,
> -                                    const uint32_t *v, Error **errp)
> +                                    const uint32_t *v,
> +                                    ObjectPropertyFlags flags,
> +                                    Error **errp)
>  {
> -    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> -                        NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint32_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint32_ptr;
> +    }
> +
> +    object_property_add(obj, name, "uint32",
> +                        getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> -                                          const uint32_t *v, Error **errp)
> +                                          const uint32_t *v,
> +                                          ObjectPropertyFlags flags,
> +                                          Error **errp)
>  {
> -    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
> -                              NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint32_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint32_ptr;
> +    }
> +
> +    object_class_property_add(klass, name, "uint32",
> +                              getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_property_add_uint64_ptr(Object *obj, const char *name,
> -                                    const uint64_t *v, Error **errp)
> +                                    const uint64_t *v,
> +                                    ObjectPropertyFlags flags,
> +                                    Error **errp)
>  {
> -    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> -                        NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint64_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint64_ptr;
> +    }
> +
> +    object_property_add(obj, name, "uint64",
> +                        getter, setter, NULL, (void *)v, errp);
>  }
>
>  void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> -                                          const uint64_t *v, Error **errp)
> +                                          const uint64_t *v,
> +                                          ObjectPropertyFlags flags,
> +                                          Error **errp)
>  {
> -    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
> -                              NULL, NULL, (void *)v, errp);
> +    ObjectPropertyAccessor *getter = NULL;
> +    ObjectPropertyAccessor *setter = NULL;
> +
> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> +        getter = property_get_uint64_ptr;
> +    }
> +
> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> +        setter = property_set_uint64_ptr;
> +    }
> +
> +    object_class_property_add(klass, name, "uint64",
> +                              getter, setter, NULL, (void *)v, errp);
>  }
>
>  typedef struct {
> diff --git a/ui/console.c b/ui/console.c
> index 82d1ddac9c..7d6ef90978 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
>                               object_property_allow_set_link,
>                               OBJ_PROP_LINK_STRONG,
>                               &error_abort);
> -    object_property_add_uint32_ptr(obj, "head",
> -                                   &s->head, &error_abort);
> +    object_property_add_uint32_ptr(obj, "head", &s->head,
> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>
>      if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
>          (console_type == GRAPHIC_CONSOLE))) {
> --
> 2.20.1
>


looks good otherwise

-- 
Marc-André Lureau

Re: [PATCH v2 1/4] qom/object: enable setter for uint types

[Felipe Franciosi]

Heya,

> On Nov 28, 2019, at 8:35 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> 
> Hi
> 
> On Thu, Nov 28, 2019 at 8:48 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>> 
>> Traditionally, the uint-specific property helpers only offer getters.
>> When adding object (or class) uint types, one must therefore use the
>> generic property helper if a setter is needed (and probably duplicate
>> some code writing their own getters/setters).
>> 
>> This enhances the uint-specific property helper APIs by adding a
>> bitwise-or'd 'flags' field and modifying all clients of that API to set
>> this paramater to OBJ_PROP_FLAG_RD. This maintains the current behaviour
>> whilst allowing others to also set OBJ_PROP_FLAG_WR in the future (which
>> will automatically install a setter). Other flags may be added later.
> 
> For readability, I would have the full spelling:
> OBJECT_PROPERTY_FLAG_READ/OBJECT_PROPERTY_FLAG_WRITE (and an alias
> OBJECT_PROPERTY_FLAG_READWRITE = READ|WRITE)

I personally prefer the abbreviated version because, IMHO, it's
sufficiently understandable and make the code cleaner.

Clients of these are already having to use fairly long function names
(eg.  object_property_add_uint64_ptr -- 30 chars long) and then would
need to use equally long flags (eg. OBJECT_PROPERTY_FLAG_READWRITE
also has 30 chars).

I looked it up and found precedence for both models:

https://github.com/qemu/qemu/blob/master/include/block/block.h#L234
BlockOpType defines enums like BLOCK_OP_TYPE_EJECT

https://github.com/qemu/qemu/blob/master/include/block/block.h#L264
The permission enums are abbreviated (eg. BLK_PERM_RESIZE)

Is there a broader direction that we should follow here?

Otherwise, can I persuade you to meet me in the middle and we change
_RD to _READ, _WR to _WRITE, but continue using OBJ_PROP_FLAG?
Note that I didn't abuse it by going with OBJ_PROP_FL_ :)

As for the alias, I noticed your suggestion in the previous e-mail.
But I have never seen such combinations in enums that define
bitwise-or'd flags. For example:
https://github.com/qemu/qemu/blob/master/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h#L106
https://github.com/qemu/qemu/blob/master/include/qapi/qmp/dispatch.h#L21

It would make sense if this was pound-defined and not enum'd. What do
you reckon? I'm happy to switch to pound-define and include the
_READWRITE, but if sticking to enum I think we're better without it.

> 
>> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> hw/acpi/ich9.c       |   4 +-
>> hw/acpi/pcihp.c      |   7 +-
>> hw/acpi/piix4.c      |  12 +--
>> hw/isa/lpc_ich9.c    |   4 +-
>> hw/ppc/spapr_drc.c   |   2 +-
>> include/qom/object.h |  42 +++++++--
>> qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
>> ui/console.c         |   4 +-
>> 8 files changed, 243 insertions(+), 48 deletions(-)
>> 
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 2034dd749e..236300d2a9 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>>     pm->s4_val = 2;
>> 
>>     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>> -                                   &pm->pm_io_base, errp);
>> +                                   &pm->pm_io_base, OBJ_PROP_FLAG_RD, errp);
>>     object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
>>                         ich9_pm_get_gpe0_blk,
>>                         NULL, NULL, pm, NULL);
>>     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
>> -                                   &gpe0_len, errp);
>> +                                   &gpe0_len, OBJ_PROP_FLAG_RD, errp);
>>     object_property_add_bool(obj, "memory-hotplug-support",
>>                              ich9_pm_get_memory_hotplug_support,
>>                              ich9_pm_set_memory_hotplug_support,
>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>> index 8413348a33..c8a7194b19 100644
>> --- a/hw/acpi/pcihp.c
>> +++ b/hw/acpi/pcihp.c
>> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>> 
>>         *bus_bsel = (*bsel_alloc)++;
>>         object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
>> -                                       bus_bsel, &error_abort);
>> +                                       bus_bsel, OBJ_PROP_FLAG_RD,
>> +                                       &error_abort);
>>     }
>> 
>>     return bsel_alloc;
>> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
>>     memory_region_add_subregion(address_space_io, s->io_base, &s->io);
>> 
>>     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
>> -                                   &error_abort);
>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>>     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
>> -                                   &error_abort);
>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>> }
>> 
>> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 93aec2dd2c..06d964a840 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
>>     static const uint16_t sci_int = 9;
>> 
>>     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>> -                                  &acpi_enable_cmd, NULL);
>> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> -                                  &acpi_disable_cmd, NULL);
>> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
>> -                                  &gpe0_blk, NULL);
>> +                                  &gpe0_blk, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>> -                                  &gpe0_blk_len, NULL);
>> +                                  &gpe0_blk_len, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>> -                                  &sci_int, NULL);
>> +                                  &sci_int, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>> -                                  &s->io_base, NULL);
>> +                                  &s->io_base, OBJ_PROP_FLAG_RD, NULL);
>> }
>> 
>> static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 17c292e306..f5526f9c3b 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>>                         ich9_lpc_get_sci_int,
>>                         NULL, NULL, NULL, NULL);
>>     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>> -                                  &acpi_enable_cmd, NULL);
>> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> -                                  &acpi_disable_cmd, NULL);
>> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
>> 
>>     ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
>> }
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 62f1a42592..ace2db0413 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -553,7 +553,7 @@ static void spapr_dr_connector_instance_init(Object *obj)
>>     SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
>>     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>> 
>> -    object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
>> +    object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_RD, NULL);
>>     object_property_add(obj, "index", "uint32", prop_get_index,
>>                         NULL, NULL, NULL, NULL);
>>     object_property_add(obj, "fdt", "struct", prop_get_fdt,
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 128d00c77f..4836c54e93 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1579,65 +1579,91 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
>>                                   void (*get)(Object *, struct tm *, Error **),
>>                                   Error **errp);
>> 
>> +typedef enum {
>> +    /* Automatically add a getter to the property */
>> +    OBJ_PROP_FLAG_RD = (1UL << 0),
>> +    /* Automatically add a setter to the property */
>> +    OBJ_PROP_FLAG_WR = (1UL << 1),
>> +} ObjectPropertyFlags;
>> +
>> /**
>>  * object_property_add_uint8_ptr:
>>  * @obj: the object to add a property to
>>  * @name: the name of the property
>>  * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>  * @errp: if an error occurs, a pointer to an area to store the error
>>  *
>>  * Add an integer property in memory.  This function will add a
>>  * property of type 'uint8'.
>>  */
>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>> -                                   const uint8_t *v, Error **errp);
>> +                                   const uint8_t *v, ObjectPropertyFlags flags,
>> +                                   Error **errp);
>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>> -                                         const uint8_t *v, Error **errp);
>> +                                         const uint8_t *v,
>> +                                         ObjectPropertyFlags flags,
>> +                                         Error **errp);
>> 
>> /**
>>  * object_property_add_uint16_ptr:
>>  * @obj: the object to add a property to
>>  * @name: the name of the property
>>  * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>  * @errp: if an error occurs, a pointer to an area to store the error
>>  *
>>  * Add an integer property in memory.  This function will add a
>>  * property of type 'uint16'.
>>  */
>> void object_property_add_uint16_ptr(Object *obj, const char *name,
>> -                                    const uint16_t *v, Error **errp);
>> +                                    const uint16_t *v,
>> +                                    ObjectPropertyFlags flags,
>> +                                    Error **errp);
>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>> -                                          const uint16_t *v, Error **errp);
>> +                                          const uint16_t *v,
>> +                                          ObjectPropertyFlags flags,
>> +                                          Error **errp);
>> 
>> /**
>>  * object_property_add_uint32_ptr:
>>  * @obj: the object to add a property to
>>  * @name: the name of the property
>>  * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>  * @errp: if an error occurs, a pointer to an area to store the error
>>  *
>>  * Add an integer property in memory.  This function will add a
>>  * property of type 'uint32'.
>>  */
>> void object_property_add_uint32_ptr(Object *obj, const char *name,
>> -                                    const uint32_t *v, Error **errp);
>> +                                    const uint32_t *v,
>> +                                    ObjectPropertyFlags flags,
>> +                                    Error **errp);
>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>> -                                          const uint32_t *v, Error **errp);
>> +                                          const uint32_t *v,
>> +                                          ObjectPropertyFlags flags,
>> +                                          Error **errp);
>> 
>> /**
>>  * object_property_add_uint64_ptr:
>>  * @obj: the object to add a property to
>>  * @name: the name of the property
>>  * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>  * @errp: if an error occurs, a pointer to an area to store the error
>>  *
>>  * Add an integer property in memory.  This function will add a
>>  * property of type 'uint64'.
>>  */
>> void object_property_add_uint64_ptr(Object *obj, const char *name,
>> -                                    const uint64_t *v, Error **Errp);
>> +                                    const uint64_t *v,
>> +                                    ObjectPropertyFlags flags,
>> +                                    Error **Errp);
>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>> -                                          const uint64_t *v, Error **Errp);
>> +                                          const uint64_t *v,
>> +                                          ObjectPropertyFlags flags,
>> +                                          Error **Errp);
>> 
>> /**
>>  * object_property_add_alias:
>> diff --git a/qom/object.c b/qom/object.c
>> index d51b57fba1..6f300b5317 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
>>     visit_type_uint8(v, name, &value, errp);
>> }
>> 
>> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
>> +                                   void *opaque, Error **errp)
>> +{
>> +    uint8_t *field = opaque;
>> +    uint8_t value;
>> +    Error *local_err = NULL;
>> +
>> +    visit_type_uint8(v, name, &value, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    *field = value;
>> +}
>> +
>> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>>                                     void *opaque, Error **errp)
>> {
>> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>>     visit_type_uint16(v, name, &value, errp);
>> }
>> 
>> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
>> +                                    void *opaque, Error **errp)
>> +{
>> +    uint16_t *field = opaque;
>> +    uint16_t value;
>> +    Error *local_err = NULL;
>> +
>> +    visit_type_uint16(v, name, &value, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    *field = value;
>> +}
>> +
>> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>>                                     void *opaque, Error **errp)
>> {
>> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>>     visit_type_uint32(v, name, &value, errp);
>> }
>> 
>> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
>> +                                    void *opaque, Error **errp)
>> +{
>> +    uint32_t *field = opaque;
>> +    uint32_t value;
>> +    Error *local_err = NULL;
>> +
>> +    visit_type_uint32(v, name, &value, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    *field = value;
>> +}
>> +
>> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>>                                     void *opaque, Error **errp)
>> {
>> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>>     visit_type_uint64(v, name, &value, errp);
>> }
>> 
>> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
>> +                                    void *opaque, Error **errp)
>> +{
>> +    uint64_t *field = opaque;
>> +    uint64_t value;
>> +    Error *local_err = NULL;
>> +
>> +    visit_type_uint64(v, name, &value, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    *field = value;
>> +}
>> +
>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>> -                                   const uint8_t *v, Error **errp)
>> +                                   const uint8_t *v,
>> +                                   ObjectPropertyFlags flags,
>> +                                   Error **errp)
>> {
>> -    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint8_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint8_ptr;
>> +    }
>> +
>> +    object_property_add(obj, name, "uint8",
>> +                        getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>> -                                         const uint8_t *v, Error **errp)
>> +                                         const uint8_t *v,
>> +                                         ObjectPropertyFlags flags,
>> +                                         Error **errp)
>> {
>> -    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
>> -                              NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint8_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint8_ptr;
>> +    }
>> +
>> +    object_class_property_add(klass, name, "uint8",
>> +                              getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_property_add_uint16_ptr(Object *obj, const char *name,
>> -                                    const uint16_t *v, Error **errp)
>> +                                    const uint16_t *v,
>> +                                    ObjectPropertyFlags flags,
>> +                                    Error **errp)
>> {
>> -    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint16_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint16_ptr;
>> +    }
>> +
>> +    object_property_add(obj, name, "uint16",
>> +                        getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>> -                                          const uint16_t *v, Error **errp)
>> +                                          const uint16_t *v,
>> +                                          ObjectPropertyFlags flags,
>> +                                          Error **errp)
>> {
>> -    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
>> -                              NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint16_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint16_ptr;
>> +    }
>> +
>> +    object_class_property_add(klass, name, "uint16",
>> +                              getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_property_add_uint32_ptr(Object *obj, const char *name,
>> -                                    const uint32_t *v, Error **errp)
>> +                                    const uint32_t *v,
>> +                                    ObjectPropertyFlags flags,
>> +                                    Error **errp)
>> {
>> -    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint32_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint32_ptr;
>> +    }
>> +
>> +    object_property_add(obj, name, "uint32",
>> +                        getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>> -                                          const uint32_t *v, Error **errp)
>> +                                          const uint32_t *v,
>> +                                          ObjectPropertyFlags flags,
>> +                                          Error **errp)
>> {
>> -    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
>> -                              NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint32_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint32_ptr;
>> +    }
>> +
>> +    object_class_property_add(klass, name, "uint32",
>> +                              getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_property_add_uint64_ptr(Object *obj, const char *name,
>> -                                    const uint64_t *v, Error **errp)
>> +                                    const uint64_t *v,
>> +                                    ObjectPropertyFlags flags,
>> +                                    Error **errp)
>> {
>> -    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint64_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint64_ptr;
>> +    }
>> +
>> +    object_property_add(obj, name, "uint64",
>> +                        getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>> -                                          const uint64_t *v, Error **errp)
>> +                                          const uint64_t *v,
>> +                                          ObjectPropertyFlags flags,
>> +                                          Error **errp)
>> {
>> -    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
>> -                              NULL, NULL, (void *)v, errp);
>> +    ObjectPropertyAccessor *getter = NULL;
>> +    ObjectPropertyAccessor *setter = NULL;
>> +
>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>> +        getter = property_get_uint64_ptr;
>> +    }
>> +
>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>> +        setter = property_set_uint64_ptr;
>> +    }
>> +
>> +    object_class_property_add(klass, name, "uint64",
>> +                              getter, setter, NULL, (void *)v, errp);
>> }
>> 
>> typedef struct {
>> diff --git a/ui/console.c b/ui/console.c
>> index 82d1ddac9c..7d6ef90978 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
>>                              object_property_allow_set_link,
>>                              OBJ_PROP_LINK_STRONG,
>>                              &error_abort);
>> -    object_property_add_uint32_ptr(obj, "head",
>> -                                   &s->head, &error_abort);
>> +    object_property_add_uint32_ptr(obj, "head", &s->head,
>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>> 
>>     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
>>         (console_type == GRAPHIC_CONSOLE))) {
>> --
>> 2.20.1
>> 
> 
> 
> looks good otherwise

Thanks,
Felipe


> 

> -- 
> Marc-André Lureau

Re: [PATCH v2 1/4] qom/object: enable setter for uint types

[Marc-André Lureau]

Hi

On Fri, Nov 29, 2019 at 7:14 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Heya,
>
> > On Nov 28, 2019, at 8:35 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Nov 28, 2019 at 8:48 PM Felipe Franciosi <felipe@nutanix.com> wrote:
> >>
> >> Traditionally, the uint-specific property helpers only offer getters.
> >> When adding object (or class) uint types, one must therefore use the
> >> generic property helper if a setter is needed (and probably duplicate
> >> some code writing their own getters/setters).
> >>
> >> This enhances the uint-specific property helper APIs by adding a
> >> bitwise-or'd 'flags' field and modifying all clients of that API to set
> >> this paramater to OBJ_PROP_FLAG_RD. This maintains the current behaviour
> >> whilst allowing others to also set OBJ_PROP_FLAG_WR in the future (which
> >> will automatically install a setter). Other flags may be added later.
> >
> > For readability, I would have the full spelling:
> > OBJECT_PROPERTY_FLAG_READ/OBJECT_PROPERTY_FLAG_WRITE (and an alias
> > OBJECT_PROPERTY_FLAG_READWRITE = READ|WRITE)
>
> I personally prefer the abbreviated version because, IMHO, it's
> sufficiently understandable and make the code cleaner.

I disagree, abbreviations make the code unnecessarily harder to read.
Typing it is a one time thing, and often editor can auto-complete.

>
> Clients of these are already having to use fairly long function names
> (eg.  object_property_add_uint64_ptr -- 30 chars long) and then would
> need to use equally long flags (eg. OBJECT_PROPERTY_FLAG_READWRITE
> also has 30 chars).
>
> I looked it up and found precedence for both models:
>
> https://github.com/qemu/qemu/blob/master/include/block/block.h#L234
> BlockOpType defines enums like BLOCK_OP_TYPE_EJECT
>
> https://github.com/qemu/qemu/blob/master/include/block/block.h#L264
> The permission enums are abbreviated (eg. BLK_PERM_RESIZE)
>
> Is there a broader direction that we should follow here?
>
> Otherwise, can I persuade you to meet me in the middle and we change
> _RD to _READ, _WR to _WRITE, but continue using OBJ_PROP_FLAG?
> Note that I didn't abuse it by going with OBJ_PROP_FL_ :)

We use OBJECT_ prefix for object.h pretty consistently

OBJ_PROP_LINK is imho a bad precedent (I just noticed), but since it
exists, one may want to stick to OBJ_PROP_ prefix for now for
consitency.

>
> As for the alias, I noticed your suggestion in the previous e-mail.
> But I have never seen such combinations in enums that define
> bitwise-or'd flags. For example:
> https://github.com/qemu/qemu/blob/master/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h#L106
> https://github.com/qemu/qemu/blob/master/include/qapi/qmp/dispatch.h#L21
>
> It would make sense if this was pound-defined and not enum'd. What do
> you reckon? I'm happy to switch to pound-define and include the
> _READWRITE, but if sticking to enum I think we're better without it.

enum are better imho, see for ex G_PARAM_READWRITE:

https://gitlab.gnome.org/GNOME/glib/blob/master/gobject/gparam.h#L150


> >
> >>
> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> >> ---
> >> hw/acpi/ich9.c       |   4 +-
> >> hw/acpi/pcihp.c      |   7 +-
> >> hw/acpi/piix4.c      |  12 +--
> >> hw/isa/lpc_ich9.c    |   4 +-
> >> hw/ppc/spapr_drc.c   |   2 +-
> >> include/qom/object.h |  42 +++++++--
> >> qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
> >> ui/console.c         |   4 +-
> >> 8 files changed, 243 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >> index 2034dd749e..236300d2a9 100644
> >> --- a/hw/acpi/ich9.c
> >> +++ b/hw/acpi/ich9.c
> >> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> >>     pm->s4_val = 2;
> >>
> >>     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> >> -                                   &pm->pm_io_base, errp);
> >> +                                   &pm->pm_io_base, OBJ_PROP_FLAG_RD, errp);
> >>     object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
> >>                         ich9_pm_get_gpe0_blk,
> >>                         NULL, NULL, pm, NULL);
> >>     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> >> -                                   &gpe0_len, errp);
> >> +                                   &gpe0_len, OBJ_PROP_FLAG_RD, errp);
> >>     object_property_add_bool(obj, "memory-hotplug-support",
> >>                              ich9_pm_get_memory_hotplug_support,
> >>                              ich9_pm_set_memory_hotplug_support,
> >> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >> index 8413348a33..c8a7194b19 100644
> >> --- a/hw/acpi/pcihp.c
> >> +++ b/hw/acpi/pcihp.c
> >> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> >>
> >>         *bus_bsel = (*bsel_alloc)++;
> >>         object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> >> -                                       bus_bsel, &error_abort);
> >> +                                       bus_bsel, OBJ_PROP_FLAG_RD,
> >> +                                       &error_abort);
> >>     }
> >>
> >>     return bsel_alloc;
> >> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> >>     memory_region_add_subregion(address_space_io, s->io_base, &s->io);
> >>
> >>     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
> >> -                                   &error_abort);
> >> +                                   OBJ_PROP_FLAG_RD, &error_abort);
> >>     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
> >> -                                   &error_abort);
> >> +                                   OBJ_PROP_FLAG_RD, &error_abort);
> >> }
> >>
> >> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >> index 93aec2dd2c..06d964a840 100644
> >> --- a/hw/acpi/piix4.c
> >> +++ b/hw/acpi/piix4.c
> >> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
> >>     static const uint16_t sci_int = 9;
> >>
> >>     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> >> -                                  &acpi_enable_cmd, NULL);
> >> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >> -                                  &acpi_disable_cmd, NULL);
> >> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> >> -                                  &gpe0_blk, NULL);
> >> +                                  &gpe0_blk, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> >> -                                  &gpe0_blk_len, NULL);
> >> +                                  &gpe0_blk_len, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> >> -                                  &sci_int, NULL);
> >> +                                  &sci_int, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> >> -                                  &s->io_base, NULL);
> >> +                                  &s->io_base, OBJ_PROP_FLAG_RD, NULL);
> >> }
> >>
> >> static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> >> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >> index 17c292e306..f5526f9c3b 100644
> >> --- a/hw/isa/lpc_ich9.c
> >> +++ b/hw/isa/lpc_ich9.c
> >> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> >>                         ich9_lpc_get_sci_int,
> >>                         NULL, NULL, NULL, NULL);
> >>     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> >> -                                  &acpi_enable_cmd, NULL);
> >> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >> -                                  &acpi_disable_cmd, NULL);
> >> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>
> >>     ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
> >> }
> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >> index 62f1a42592..ace2db0413 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -553,7 +553,7 @@ static void spapr_dr_connector_instance_init(Object *obj)
> >>     SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
> >>     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >>
> >> -    object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
> >> +    object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_RD, NULL);
> >>     object_property_add(obj, "index", "uint32", prop_get_index,
> >>                         NULL, NULL, NULL, NULL);
> >>     object_property_add(obj, "fdt", "struct", prop_get_fdt,
> >> diff --git a/include/qom/object.h b/include/qom/object.h
> >> index 128d00c77f..4836c54e93 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> >> @@ -1579,65 +1579,91 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
> >>                                   void (*get)(Object *, struct tm *, Error **),
> >>                                   Error **errp);
> >>
> >> +typedef enum {
> >> +    /* Automatically add a getter to the property */
> >> +    OBJ_PROP_FLAG_RD = (1UL << 0),
> >> +    /* Automatically add a setter to the property */
> >> +    OBJ_PROP_FLAG_WR = (1UL << 1),
> >> +} ObjectPropertyFlags;
> >> +
> >> /**
> >>  * object_property_add_uint8_ptr:
> >>  * @obj: the object to add a property to
> >>  * @name: the name of the property
> >>  * @v: pointer to value
> >> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>  * @errp: if an error occurs, a pointer to an area to store the error
> >>  *
> >>  * Add an integer property in memory.  This function will add a
> >>  * property of type 'uint8'.
> >>  */
> >> void object_property_add_uint8_ptr(Object *obj, const char *name,
> >> -                                   const uint8_t *v, Error **errp);
> >> +                                   const uint8_t *v, ObjectPropertyFlags flags,
> >> +                                   Error **errp);
> >> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> >> -                                         const uint8_t *v, Error **errp);
> >> +                                         const uint8_t *v,
> >> +                                         ObjectPropertyFlags flags,
> >> +                                         Error **errp);
> >>
> >> /**
> >>  * object_property_add_uint16_ptr:
> >>  * @obj: the object to add a property to
> >>  * @name: the name of the property
> >>  * @v: pointer to value
> >> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>  * @errp: if an error occurs, a pointer to an area to store the error
> >>  *
> >>  * Add an integer property in memory.  This function will add a
> >>  * property of type 'uint16'.
> >>  */
> >> void object_property_add_uint16_ptr(Object *obj, const char *name,
> >> -                                    const uint16_t *v, Error **errp);
> >> +                                    const uint16_t *v,
> >> +                                    ObjectPropertyFlags flags,
> >> +                                    Error **errp);
> >> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> >> -                                          const uint16_t *v, Error **errp);
> >> +                                          const uint16_t *v,
> >> +                                          ObjectPropertyFlags flags,
> >> +                                          Error **errp);
> >>
> >> /**
> >>  * object_property_add_uint32_ptr:
> >>  * @obj: the object to add a property to
> >>  * @name: the name of the property
> >>  * @v: pointer to value
> >> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>  * @errp: if an error occurs, a pointer to an area to store the error
> >>  *
> >>  * Add an integer property in memory.  This function will add a
> >>  * property of type 'uint32'.
> >>  */
> >> void object_property_add_uint32_ptr(Object *obj, const char *name,
> >> -                                    const uint32_t *v, Error **errp);
> >> +                                    const uint32_t *v,
> >> +                                    ObjectPropertyFlags flags,
> >> +                                    Error **errp);
> >> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> >> -                                          const uint32_t *v, Error **errp);
> >> +                                          const uint32_t *v,
> >> +                                          ObjectPropertyFlags flags,
> >> +                                          Error **errp);
> >>
> >> /**
> >>  * object_property_add_uint64_ptr:
> >>  * @obj: the object to add a property to
> >>  * @name: the name of the property
> >>  * @v: pointer to value
> >> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>  * @errp: if an error occurs, a pointer to an area to store the error
> >>  *
> >>  * Add an integer property in memory.  This function will add a
> >>  * property of type 'uint64'.
> >>  */
> >> void object_property_add_uint64_ptr(Object *obj, const char *name,
> >> -                                    const uint64_t *v, Error **Errp);
> >> +                                    const uint64_t *v,
> >> +                                    ObjectPropertyFlags flags,
> >> +                                    Error **Errp);
> >> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> >> -                                          const uint64_t *v, Error **Errp);
> >> +                                          const uint64_t *v,
> >> +                                          ObjectPropertyFlags flags,
> >> +                                          Error **Errp);
> >>
> >> /**
> >>  * object_property_add_alias:
> >> diff --git a/qom/object.c b/qom/object.c
> >> index d51b57fba1..6f300b5317 100644
> >> --- a/qom/object.c
> >> +++ b/qom/object.c
> >> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
> >>     visit_type_uint8(v, name, &value, errp);
> >> }
> >>
> >> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
> >> +                                   void *opaque, Error **errp)
> >> +{
> >> +    uint8_t *field = opaque;
> >> +    uint8_t value;
> >> +    Error *local_err = NULL;
> >> +
> >> +    visit_type_uint8(v, name, &value, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >> +
> >> +    *field = value;
> >> +}
> >> +
> >> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >>                                     void *opaque, Error **errp)
> >> {
> >> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >>     visit_type_uint16(v, name, &value, errp);
> >> }
> >>
> >> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >> +                                    void *opaque, Error **errp)
> >> +{
> >> +    uint16_t *field = opaque;
> >> +    uint16_t value;
> >> +    Error *local_err = NULL;
> >> +
> >> +    visit_type_uint16(v, name, &value, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >> +
> >> +    *field = value;
> >> +}
> >> +
> >> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >>                                     void *opaque, Error **errp)
> >> {
> >> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >>     visit_type_uint32(v, name, &value, errp);
> >> }
> >>
> >> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >> +                                    void *opaque, Error **errp)
> >> +{
> >> +    uint32_t *field = opaque;
> >> +    uint32_t value;
> >> +    Error *local_err = NULL;
> >> +
> >> +    visit_type_uint32(v, name, &value, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >> +
> >> +    *field = value;
> >> +}
> >> +
> >> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >>                                     void *opaque, Error **errp)
> >> {
> >> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >>     visit_type_uint64(v, name, &value, errp);
> >> }
> >>
> >> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >> +                                    void *opaque, Error **errp)
> >> +{
> >> +    uint64_t *field = opaque;
> >> +    uint64_t value;
> >> +    Error *local_err = NULL;
> >> +
> >> +    visit_type_uint64(v, name, &value, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >> +
> >> +    *field = value;
> >> +}
> >> +
> >> void object_property_add_uint8_ptr(Object *obj, const char *name,
> >> -                                   const uint8_t *v, Error **errp)
> >> +                                   const uint8_t *v,
> >> +                                   ObjectPropertyFlags flags,
> >> +                                   Error **errp)
> >> {
> >> -    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> >> -                        NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint8_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint8_ptr;
> >> +    }
> >> +
> >> +    object_property_add(obj, name, "uint8",
> >> +                        getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> >> -                                         const uint8_t *v, Error **errp)
> >> +                                         const uint8_t *v,
> >> +                                         ObjectPropertyFlags flags,
> >> +                                         Error **errp)
> >> {
> >> -    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
> >> -                              NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint8_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint8_ptr;
> >> +    }
> >> +
> >> +    object_class_property_add(klass, name, "uint8",
> >> +                              getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_property_add_uint16_ptr(Object *obj, const char *name,
> >> -                                    const uint16_t *v, Error **errp)
> >> +                                    const uint16_t *v,
> >> +                                    ObjectPropertyFlags flags,
> >> +                                    Error **errp)
> >> {
> >> -    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> >> -                        NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint16_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint16_ptr;
> >> +    }
> >> +
> >> +    object_property_add(obj, name, "uint16",
> >> +                        getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> >> -                                          const uint16_t *v, Error **errp)
> >> +                                          const uint16_t *v,
> >> +                                          ObjectPropertyFlags flags,
> >> +                                          Error **errp)
> >> {
> >> -    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
> >> -                              NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint16_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint16_ptr;
> >> +    }
> >> +
> >> +    object_class_property_add(klass, name, "uint16",
> >> +                              getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_property_add_uint32_ptr(Object *obj, const char *name,
> >> -                                    const uint32_t *v, Error **errp)
> >> +                                    const uint32_t *v,
> >> +                                    ObjectPropertyFlags flags,
> >> +                                    Error **errp)
> >> {
> >> -    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> >> -                        NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint32_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint32_ptr;
> >> +    }
> >> +
> >> +    object_property_add(obj, name, "uint32",
> >> +                        getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> >> -                                          const uint32_t *v, Error **errp)
> >> +                                          const uint32_t *v,
> >> +                                          ObjectPropertyFlags flags,
> >> +                                          Error **errp)
> >> {
> >> -    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
> >> -                              NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint32_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint32_ptr;
> >> +    }
> >> +
> >> +    object_class_property_add(klass, name, "uint32",
> >> +                              getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_property_add_uint64_ptr(Object *obj, const char *name,
> >> -                                    const uint64_t *v, Error **errp)
> >> +                                    const uint64_t *v,
> >> +                                    ObjectPropertyFlags flags,
> >> +                                    Error **errp)
> >> {
> >> -    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> >> -                        NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint64_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint64_ptr;
> >> +    }
> >> +
> >> +    object_property_add(obj, name, "uint64",
> >> +                        getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> >> -                                          const uint64_t *v, Error **errp)
> >> +                                          const uint64_t *v,
> >> +                                          ObjectPropertyFlags flags,
> >> +                                          Error **errp)
> >> {
> >> -    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
> >> -                              NULL, NULL, (void *)v, errp);
> >> +    ObjectPropertyAccessor *getter = NULL;
> >> +    ObjectPropertyAccessor *setter = NULL;
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >> +        getter = property_get_uint64_ptr;
> >> +    }
> >> +
> >> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >> +        setter = property_set_uint64_ptr;
> >> +    }
> >> +
> >> +    object_class_property_add(klass, name, "uint64",
> >> +                              getter, setter, NULL, (void *)v, errp);
> >> }
> >>
> >> typedef struct {
> >> diff --git a/ui/console.c b/ui/console.c
> >> index 82d1ddac9c..7d6ef90978 100644
> >> --- a/ui/console.c
> >> +++ b/ui/console.c
> >> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
> >>                              object_property_allow_set_link,
> >>                              OBJ_PROP_LINK_STRONG,
> >>                              &error_abort);
> >> -    object_property_add_uint32_ptr(obj, "head",
> >> -                                   &s->head, &error_abort);
> >> +    object_property_add_uint32_ptr(obj, "head", &s->head,
> >> +                                   OBJ_PROP_FLAG_RD, &error_abort);
> >>
> >>     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
> >>         (console_type == GRAPHIC_CONSOLE))) {
> >> --
> >> 2.20.1
> >>
> >
> >
> > looks good otherwise
>
> Thanks,
> Felipe
>
>
> >
>
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

Re: [PATCH v2 1/4] qom/object: enable setter for uint types

[Felipe Franciosi]

> On Nov 29, 2019, at 3:35 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> 
> Hi
> 
> On Fri, Nov 29, 2019 at 7:14 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>> 
>> Heya,
>> 
>>> On Nov 28, 2019, at 8:35 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>> 
>>> Hi
>>> 
>>> On Thu, Nov 28, 2019 at 8:48 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>>>> 
>>>> Traditionally, the uint-specific property helpers only offer getters.
>>>> When adding object (or class) uint types, one must therefore use the
>>>> generic property helper if a setter is needed (and probably duplicate
>>>> some code writing their own getters/setters).
>>>> 
>>>> This enhances the uint-specific property helper APIs by adding a
>>>> bitwise-or'd 'flags' field and modifying all clients of that API to set
>>>> this paramater to OBJ_PROP_FLAG_RD. This maintains the current behaviour
>>>> whilst allowing others to also set OBJ_PROP_FLAG_WR in the future (which
>>>> will automatically install a setter). Other flags may be added later.
>>> 
>>> For readability, I would have the full spelling:
>>> OBJECT_PROPERTY_FLAG_READ/OBJECT_PROPERTY_FLAG_WRITE (and an alias
>>> OBJECT_PROPERTY_FLAG_READWRITE = READ|WRITE)
>> 
>> I personally prefer the abbreviated version because, IMHO, it's
>> sufficiently understandable and make the code cleaner.
> 
> I disagree, abbreviations make the code unnecessarily harder to read.
> Typing it is a one time thing, and often editor can auto-complete.

You make a fair point. That's why I stressed "personally" and "IMHO",
as I don't really like when I can't fit my code in 80 cols and am
otherwise forced to split trivial ORs over multiple lines.

> 
>> 
>> Clients of these are already having to use fairly long function names
>> (eg.  object_property_add_uint64_ptr -- 30 chars long) and then would
>> need to use equally long flags (eg. OBJECT_PROPERTY_FLAG_READWRITE
>> also has 30 chars).
>> 
>> I looked it up and found precedence for both models:
>> 
>> https://github.com/qemu/qemu/blob/master/include/block/block.h#L234
>> BlockOpType defines enums like BLOCK_OP_TYPE_EJECT
>> 
>> https://github.com/qemu/qemu/blob/master/include/block/block.h#L264
>> The permission enums are abbreviated (eg. BLK_PERM_RESIZE)
>> 
>> Is there a broader direction that we should follow here?
>> 
>> Otherwise, can I persuade you to meet me in the middle and we change
>> _RD to _READ, _WR to _WRITE, but continue using OBJ_PROP_FLAG?
>> Note that I didn't abuse it by going with OBJ_PROP_FL_ :)
> 
> We use OBJECT_ prefix for object.h pretty consistently
> 
> OBJ_PROP_LINK is imho a bad precedent (I just noticed), but since it
> exists, one may want to stick to OBJ_PROP_ prefix for now for
> consitency.

Ok, so I'll send a v3 and rename _RD to _READ and _WR to _WRITE as
proposed, but stick with OBJ_PROP_FLAG_ given the precedence.
Obviously I won't object if someone overhauls the whole thing later. :)

> 
>> 
>> As for the alias, I noticed your suggestion in the previous e-mail.
>> But I have never seen such combinations in enums that define
>> bitwise-or'd flags. For example:
>> https://github.com/qemu/qemu/blob/master/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h#L106
>> https://github.com/qemu/qemu/blob/master/include/qapi/qmp/dispatch.h#L21
>> 
>> It would make sense if this was pound-defined and not enum'd. What do
>> you reckon? I'm happy to switch to pound-define and include the
>> _READWRITE, but if sticking to enum I think we're better without it.
> 
> enum are better imho, see for ex G_PARAM_READWRITE:
> 
> https://gitlab.gnome.org/GNOME/glib/blob/master/gobject/gparam.h#L150

Ah, ok. I couldn't find precedence in qemu.git, but if glib has it I'm
fine with it. Will include the _READWRITE in my v3 as well.

Cheers,
Felipe

> 
> 
>>> 
>>>> 
>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>>>> ---
>>>> hw/acpi/ich9.c       |   4 +-
>>>> hw/acpi/pcihp.c      |   7 +-
>>>> hw/acpi/piix4.c      |  12 +--
>>>> hw/isa/lpc_ich9.c    |   4 +-
>>>> hw/ppc/spapr_drc.c   |   2 +-
>>>> include/qom/object.h |  42 +++++++--
>>>> qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
>>>> ui/console.c         |   4 +-
>>>> 8 files changed, 243 insertions(+), 48 deletions(-)
>>>> 
>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>>> index 2034dd749e..236300d2a9 100644
>>>> --- a/hw/acpi/ich9.c
>>>> +++ b/hw/acpi/ich9.c
>>>> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>>>>    pm->s4_val = 2;
>>>> 
>>>>    object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>>>> -                                   &pm->pm_io_base, errp);
>>>> +                                   &pm->pm_io_base, OBJ_PROP_FLAG_RD, errp);
>>>>    object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
>>>>                        ich9_pm_get_gpe0_blk,
>>>>                        NULL, NULL, pm, NULL);
>>>>    object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
>>>> -                                   &gpe0_len, errp);
>>>> +                                   &gpe0_len, OBJ_PROP_FLAG_RD, errp);
>>>>    object_property_add_bool(obj, "memory-hotplug-support",
>>>>                             ich9_pm_get_memory_hotplug_support,
>>>>                             ich9_pm_set_memory_hotplug_support,
>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>>>> index 8413348a33..c8a7194b19 100644
>>>> --- a/hw/acpi/pcihp.c
>>>> +++ b/hw/acpi/pcihp.c
>>>> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>>>> 
>>>>        *bus_bsel = (*bsel_alloc)++;
>>>>        object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
>>>> -                                       bus_bsel, &error_abort);
>>>> +                                       bus_bsel, OBJ_PROP_FLAG_RD,
>>>> +                                       &error_abort);
>>>>    }
>>>> 
>>>>    return bsel_alloc;
>>>> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
>>>>    memory_region_add_subregion(address_space_io, s->io_base, &s->io);
>>>> 
>>>>    object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
>>>> -                                   &error_abort);
>>>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>>>>    object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
>>>> -                                   &error_abort);
>>>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>>>> }
>>>> 
>>>> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>> index 93aec2dd2c..06d964a840 100644
>>>> --- a/hw/acpi/piix4.c
>>>> +++ b/hw/acpi/piix4.c
>>>> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
>>>>    static const uint16_t sci_int = 9;
>>>> 
>>>>    object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>>>> -                                  &acpi_enable_cmd, NULL);
>>>> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>>> -                                  &acpi_disable_cmd, NULL);
>>>> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
>>>> -                                  &gpe0_blk, NULL);
>>>> +                                  &gpe0_blk, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>>>> -                                  &gpe0_blk_len, NULL);
>>>> +                                  &gpe0_blk_len, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>>>> -                                  &sci_int, NULL);
>>>> +                                  &sci_int, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>>>> -                                  &s->io_base, NULL);
>>>> +                                  &s->io_base, OBJ_PROP_FLAG_RD, NULL);
>>>> }
>>>> 
>>>> static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>> index 17c292e306..f5526f9c3b 100644
>>>> --- a/hw/isa/lpc_ich9.c
>>>> +++ b/hw/isa/lpc_ich9.c
>>>> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>>>>                        ich9_lpc_get_sci_int,
>>>>                        NULL, NULL, NULL, NULL);
>>>>    object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>>>> -                                  &acpi_enable_cmd, NULL);
>>>> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>>> -                                  &acpi_disable_cmd, NULL);
>>>> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
>>>> 
>>>>    ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
>>>> }
>>>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>>>> index 62f1a42592..ace2db0413 100644
>>>> --- a/hw/ppc/spapr_drc.c
>>>> +++ b/hw/ppc/spapr_drc.c
>>>> @@ -553,7 +553,7 @@ static void spapr_dr_connector_instance_init(Object *obj)
>>>>    SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
>>>>    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>>>> 
>>>> -    object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
>>>> +    object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_RD, NULL);
>>>>    object_property_add(obj, "index", "uint32", prop_get_index,
>>>>                        NULL, NULL, NULL, NULL);
>>>>    object_property_add(obj, "fdt", "struct", prop_get_fdt,
>>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>>> index 128d00c77f..4836c54e93 100644
>>>> --- a/include/qom/object.h
>>>> +++ b/include/qom/object.h
>>>> @@ -1579,65 +1579,91 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
>>>>                                  void (*get)(Object *, struct tm *, Error **),
>>>>                                  Error **errp);
>>>> 
>>>> +typedef enum {
>>>> +    /* Automatically add a getter to the property */
>>>> +    OBJ_PROP_FLAG_RD = (1UL << 0),
>>>> +    /* Automatically add a setter to the property */
>>>> +    OBJ_PROP_FLAG_WR = (1UL << 1),
>>>> +} ObjectPropertyFlags;
>>>> +
>>>> /**
>>>> * object_property_add_uint8_ptr:
>>>> * @obj: the object to add a property to
>>>> * @name: the name of the property
>>>> * @v: pointer to value
>>>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>>> * @errp: if an error occurs, a pointer to an area to store the error
>>>> *
>>>> * Add an integer property in memory.  This function will add a
>>>> * property of type 'uint8'.
>>>> */
>>>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>>>> -                                   const uint8_t *v, Error **errp);
>>>> +                                   const uint8_t *v, ObjectPropertyFlags flags,
>>>> +                                   Error **errp);
>>>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>>>> -                                         const uint8_t *v, Error **errp);
>>>> +                                         const uint8_t *v,
>>>> +                                         ObjectPropertyFlags flags,
>>>> +                                         Error **errp);
>>>> 
>>>> /**
>>>> * object_property_add_uint16_ptr:
>>>> * @obj: the object to add a property to
>>>> * @name: the name of the property
>>>> * @v: pointer to value
>>>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>>> * @errp: if an error occurs, a pointer to an area to store the error
>>>> *
>>>> * Add an integer property in memory.  This function will add a
>>>> * property of type 'uint16'.
>>>> */
>>>> void object_property_add_uint16_ptr(Object *obj, const char *name,
>>>> -                                    const uint16_t *v, Error **errp);
>>>> +                                    const uint16_t *v,
>>>> +                                    ObjectPropertyFlags flags,
>>>> +                                    Error **errp);
>>>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>>>> -                                          const uint16_t *v, Error **errp);
>>>> +                                          const uint16_t *v,
>>>> +                                          ObjectPropertyFlags flags,
>>>> +                                          Error **errp);
>>>> 
>>>> /**
>>>> * object_property_add_uint32_ptr:
>>>> * @obj: the object to add a property to
>>>> * @name: the name of the property
>>>> * @v: pointer to value
>>>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>>> * @errp: if an error occurs, a pointer to an area to store the error
>>>> *
>>>> * Add an integer property in memory.  This function will add a
>>>> * property of type 'uint32'.
>>>> */
>>>> void object_property_add_uint32_ptr(Object *obj, const char *name,
>>>> -                                    const uint32_t *v, Error **errp);
>>>> +                                    const uint32_t *v,
>>>> +                                    ObjectPropertyFlags flags,
>>>> +                                    Error **errp);
>>>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>>>> -                                          const uint32_t *v, Error **errp);
>>>> +                                          const uint32_t *v,
>>>> +                                          ObjectPropertyFlags flags,
>>>> +                                          Error **errp);
>>>> 
>>>> /**
>>>> * object_property_add_uint64_ptr:
>>>> * @obj: the object to add a property to
>>>> * @name: the name of the property
>>>> * @v: pointer to value
>>>> + * @flags: bitwise-or'd ObjectPropertyFlags
>>>> * @errp: if an error occurs, a pointer to an area to store the error
>>>> *
>>>> * Add an integer property in memory.  This function will add a
>>>> * property of type 'uint64'.
>>>> */
>>>> void object_property_add_uint64_ptr(Object *obj, const char *name,
>>>> -                                    const uint64_t *v, Error **Errp);
>>>> +                                    const uint64_t *v,
>>>> +                                    ObjectPropertyFlags flags,
>>>> +                                    Error **Errp);
>>>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>>>> -                                          const uint64_t *v, Error **Errp);
>>>> +                                          const uint64_t *v,
>>>> +                                          ObjectPropertyFlags flags,
>>>> +                                          Error **Errp);
>>>> 
>>>> /**
>>>> * object_property_add_alias:
>>>> diff --git a/qom/object.c b/qom/object.c
>>>> index d51b57fba1..6f300b5317 100644
>>>> --- a/qom/object.c
>>>> +++ b/qom/object.c
>>>> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
>>>>    visit_type_uint8(v, name, &value, errp);
>>>> }
>>>> 
>>>> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
>>>> +                                   void *opaque, Error **errp)
>>>> +{
>>>> +    uint8_t *field = opaque;
>>>> +    uint8_t value;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    visit_type_uint8(v, name, &value, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    *field = value;
>>>> +}
>>>> +
>>>> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>>>>                                    void *opaque, Error **errp)
>>>> {
>>>> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>>>>    visit_type_uint16(v, name, &value, errp);
>>>> }
>>>> 
>>>> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
>>>> +                                    void *opaque, Error **errp)
>>>> +{
>>>> +    uint16_t *field = opaque;
>>>> +    uint16_t value;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    visit_type_uint16(v, name, &value, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    *field = value;
>>>> +}
>>>> +
>>>> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>>>>                                    void *opaque, Error **errp)
>>>> {
>>>> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>>>>    visit_type_uint32(v, name, &value, errp);
>>>> }
>>>> 
>>>> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
>>>> +                                    void *opaque, Error **errp)
>>>> +{
>>>> +    uint32_t *field = opaque;
>>>> +    uint32_t value;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    visit_type_uint32(v, name, &value, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    *field = value;
>>>> +}
>>>> +
>>>> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>>>>                                    void *opaque, Error **errp)
>>>> {
>>>> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>>>>    visit_type_uint64(v, name, &value, errp);
>>>> }
>>>> 
>>>> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
>>>> +                                    void *opaque, Error **errp)
>>>> +{
>>>> +    uint64_t *field = opaque;
>>>> +    uint64_t value;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    visit_type_uint64(v, name, &value, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    *field = value;
>>>> +}
>>>> +
>>>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>>>> -                                   const uint8_t *v, Error **errp)
>>>> +                                   const uint8_t *v,
>>>> +                                   ObjectPropertyFlags flags,
>>>> +                                   Error **errp)
>>>> {
>>>> -    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>>>> -                        NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint8_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint8_ptr;
>>>> +    }
>>>> +
>>>> +    object_property_add(obj, name, "uint8",
>>>> +                        getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>>>> -                                         const uint8_t *v, Error **errp)
>>>> +                                         const uint8_t *v,
>>>> +                                         ObjectPropertyFlags flags,
>>>> +                                         Error **errp)
>>>> {
>>>> -    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
>>>> -                              NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint8_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint8_ptr;
>>>> +    }
>>>> +
>>>> +    object_class_property_add(klass, name, "uint8",
>>>> +                              getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_property_add_uint16_ptr(Object *obj, const char *name,
>>>> -                                    const uint16_t *v, Error **errp)
>>>> +                                    const uint16_t *v,
>>>> +                                    ObjectPropertyFlags flags,
>>>> +                                    Error **errp)
>>>> {
>>>> -    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>>>> -                        NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint16_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint16_ptr;
>>>> +    }
>>>> +
>>>> +    object_property_add(obj, name, "uint16",
>>>> +                        getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>>>> -                                          const uint16_t *v, Error **errp)
>>>> +                                          const uint16_t *v,
>>>> +                                          ObjectPropertyFlags flags,
>>>> +                                          Error **errp)
>>>> {
>>>> -    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
>>>> -                              NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint16_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint16_ptr;
>>>> +    }
>>>> +
>>>> +    object_class_property_add(klass, name, "uint16",
>>>> +                              getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_property_add_uint32_ptr(Object *obj, const char *name,
>>>> -                                    const uint32_t *v, Error **errp)
>>>> +                                    const uint32_t *v,
>>>> +                                    ObjectPropertyFlags flags,
>>>> +                                    Error **errp)
>>>> {
>>>> -    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>>>> -                        NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint32_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint32_ptr;
>>>> +    }
>>>> +
>>>> +    object_property_add(obj, name, "uint32",
>>>> +                        getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>>>> -                                          const uint32_t *v, Error **errp)
>>>> +                                          const uint32_t *v,
>>>> +                                          ObjectPropertyFlags flags,
>>>> +                                          Error **errp)
>>>> {
>>>> -    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
>>>> -                              NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint32_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint32_ptr;
>>>> +    }
>>>> +
>>>> +    object_class_property_add(klass, name, "uint32",
>>>> +                              getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_property_add_uint64_ptr(Object *obj, const char *name,
>>>> -                                    const uint64_t *v, Error **errp)
>>>> +                                    const uint64_t *v,
>>>> +                                    ObjectPropertyFlags flags,
>>>> +                                    Error **errp)
>>>> {
>>>> -    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>>>> -                        NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint64_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint64_ptr;
>>>> +    }
>>>> +
>>>> +    object_property_add(obj, name, "uint64",
>>>> +                        getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>>>> -                                          const uint64_t *v, Error **errp)
>>>> +                                          const uint64_t *v,
>>>> +                                          ObjectPropertyFlags flags,
>>>> +                                          Error **errp)
>>>> {
>>>> -    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
>>>> -                              NULL, NULL, (void *)v, errp);
>>>> +    ObjectPropertyAccessor *getter = NULL;
>>>> +    ObjectPropertyAccessor *setter = NULL;
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
>>>> +        getter = property_get_uint64_ptr;
>>>> +    }
>>>> +
>>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
>>>> +        setter = property_set_uint64_ptr;
>>>> +    }
>>>> +
>>>> +    object_class_property_add(klass, name, "uint64",
>>>> +                              getter, setter, NULL, (void *)v, errp);
>>>> }
>>>> 
>>>> typedef struct {
>>>> diff --git a/ui/console.c b/ui/console.c
>>>> index 82d1ddac9c..7d6ef90978 100644
>>>> --- a/ui/console.c
>>>> +++ b/ui/console.c
>>>> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
>>>>                             object_property_allow_set_link,
>>>>                             OBJ_PROP_LINK_STRONG,
>>>>                             &error_abort);
>>>> -    object_property_add_uint32_ptr(obj, "head",
>>>> -                                   &s->head, &error_abort);
>>>> +    object_property_add_uint32_ptr(obj, "head", &s->head,
>>>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
>>>> 
>>>>    if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
>>>>        (console_type == GRAPHIC_CONSOLE))) {
>>>> --
>>>> 2.20.1
>>>> 
>>> 
>>> 
>>> looks good otherwise
>> 
>> Thanks,
>> Felipe
>> 
>> 
>>> 
>> 
>>> --
>>> Marc-André Lureau
>> 
> 
> 
> -- 
> Marc-André Lureau

Re: [PATCH v2 1/4] qom/object: enable setter for uint types

[Marc-André Lureau]

Hi

On Fri, Nov 29, 2019 at 8:25 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
>
>
> > On Nov 29, 2019, at 3:35 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Fri, Nov 29, 2019 at 7:14 PM Felipe Franciosi <felipe@nutanix.com> wrote:
> >>
> >> Heya,
> >>
> >>> On Nov 28, 2019, at 8:35 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >>>
> >>> Hi
> >>>
> >>> On Thu, Nov 28, 2019 at 8:48 PM Felipe Franciosi <felipe@nutanix.com> wrote:
> >>>>
> >>>> Traditionally, the uint-specific property helpers only offer getters.
> >>>> When adding object (or class) uint types, one must therefore use the
> >>>> generic property helper if a setter is needed (and probably duplicate
> >>>> some code writing their own getters/setters).
> >>>>
> >>>> This enhances the uint-specific property helper APIs by adding a
> >>>> bitwise-or'd 'flags' field and modifying all clients of that API to set
> >>>> this paramater to OBJ_PROP_FLAG_RD. This maintains the current behaviour
> >>>> whilst allowing others to also set OBJ_PROP_FLAG_WR in the future (which
> >>>> will automatically install a setter). Other flags may be added later.
> >>>
> >>> For readability, I would have the full spelling:
> >>> OBJECT_PROPERTY_FLAG_READ/OBJECT_PROPERTY_FLAG_WRITE (and an alias
> >>> OBJECT_PROPERTY_FLAG_READWRITE = READ|WRITE)
> >>
> >> I personally prefer the abbreviated version because, IMHO, it's
> >> sufficiently understandable and make the code cleaner.
> >
> > I disagree, abbreviations make the code unnecessarily harder to read.
> > Typing it is a one time thing, and often editor can auto-complete.
>
> You make a fair point. That's why I stressed "personally" and "IMHO",
> as I don't really like when I can't fit my code in 80 cols and am
> otherwise forced to split trivial ORs over multiple lines.

I didn't mean to be strict. Of course, we use abbreviations all the
time. I meant, there are some limits, rd for read doesn't help much
for example.

>
> >
> >>
> >> Clients of these are already having to use fairly long function names
> >> (eg.  object_property_add_uint64_ptr -- 30 chars long) and then would
> >> need to use equally long flags (eg. OBJECT_PROPERTY_FLAG_READWRITE
> >> also has 30 chars).
> >>
> >> I looked it up and found precedence for both models:
> >>
> >> https://github.com/qemu/qemu/blob/master/include/block/block.h#L234
> >> BlockOpType defines enums like BLOCK_OP_TYPE_EJECT
> >>
> >> https://github.com/qemu/qemu/blob/master/include/block/block.h#L264
> >> The permission enums are abbreviated (eg. BLK_PERM_RESIZE)
> >>
> >> Is there a broader direction that we should follow here?
> >>
> >> Otherwise, can I persuade you to meet me in the middle and we change
> >> _RD to _READ, _WR to _WRITE, but continue using OBJ_PROP_FLAG?
> >> Note that I didn't abuse it by going with OBJ_PROP_FL_ :)
> >
> > We use OBJECT_ prefix for object.h pretty consistently
> >
> > OBJ_PROP_LINK is imho a bad precedent (I just noticed), but since it
> > exists, one may want to stick to OBJ_PROP_ prefix for now for
> > consitency.
>
> Ok, so I'll send a v3 and rename _RD to _READ and _WR to _WRITE as
> proposed, but stick with OBJ_PROP_FLAG_ given the precedence.
> Obviously I won't object if someone overhauls the whole thing later. :)

good for me, \o/

>
> >
> >>
> >> As for the alias, I noticed your suggestion in the previous e-mail.
> >> But I have never seen such combinations in enums that define
> >> bitwise-or'd flags. For example:
> >> https://github.com/qemu/qemu/blob/master/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h#L106
> >> https://github.com/qemu/qemu/blob/master/include/qapi/qmp/dispatch.h#L21
> >>
> >> It would make sense if this was pound-defined and not enum'd. What do
> >> you reckon? I'm happy to switch to pound-define and include the
> >> _READWRITE, but if sticking to enum I think we're better without it.
> >
> > enum are better imho, see for ex G_PARAM_READWRITE:
> >
> > https://gitlab.gnome.org/GNOME/glib/blob/master/gobject/gparam.h#L150
>
> Ah, ok. I couldn't find precedence in qemu.git, but if glib has it I'm
> fine with it. Will include the _READWRITE in my v3 as well.

thanks!

>
> Cheers,
> Felipe
>
> >
> >
> >>>
> >>>>
> >>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> >>>> ---
> >>>> hw/acpi/ich9.c       |   4 +-
> >>>> hw/acpi/pcihp.c      |   7 +-
> >>>> hw/acpi/piix4.c      |  12 +--
> >>>> hw/isa/lpc_ich9.c    |   4 +-
> >>>> hw/ppc/spapr_drc.c   |   2 +-
> >>>> include/qom/object.h |  42 +++++++--
> >>>> qom/object.c         | 216 ++++++++++++++++++++++++++++++++++++++-----
> >>>> ui/console.c         |   4 +-
> >>>> 8 files changed, 243 insertions(+), 48 deletions(-)
> >>>>
> >>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >>>> index 2034dd749e..236300d2a9 100644
> >>>> --- a/hw/acpi/ich9.c
> >>>> +++ b/hw/acpi/ich9.c
> >>>> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> >>>>    pm->s4_val = 2;
> >>>>
> >>>>    object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> >>>> -                                   &pm->pm_io_base, errp);
> >>>> +                                   &pm->pm_io_base, OBJ_PROP_FLAG_RD, errp);
> >>>>    object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
> >>>>                        ich9_pm_get_gpe0_blk,
> >>>>                        NULL, NULL, pm, NULL);
> >>>>    object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> >>>> -                                   &gpe0_len, errp);
> >>>> +                                   &gpe0_len, OBJ_PROP_FLAG_RD, errp);
> >>>>    object_property_add_bool(obj, "memory-hotplug-support",
> >>>>                             ich9_pm_get_memory_hotplug_support,
> >>>>                             ich9_pm_set_memory_hotplug_support,
> >>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >>>> index 8413348a33..c8a7194b19 100644
> >>>> --- a/hw/acpi/pcihp.c
> >>>> +++ b/hw/acpi/pcihp.c
> >>>> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> >>>>
> >>>>        *bus_bsel = (*bsel_alloc)++;
> >>>>        object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> >>>> -                                       bus_bsel, &error_abort);
> >>>> +                                       bus_bsel, OBJ_PROP_FLAG_RD,
> >>>> +                                       &error_abort);
> >>>>    }
> >>>>
> >>>>    return bsel_alloc;
> >>>> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> >>>>    memory_region_add_subregion(address_space_io, s->io_base, &s->io);
> >>>>
> >>>>    object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
> >>>> -                                   &error_abort);
> >>>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
> >>>>    object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
> >>>> -                                   &error_abort);
> >>>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
> >>>> }
> >>>>
> >>>> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> >>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >>>> index 93aec2dd2c..06d964a840 100644
> >>>> --- a/hw/acpi/piix4.c
> >>>> +++ b/hw/acpi/piix4.c
> >>>> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
> >>>>    static const uint16_t sci_int = 9;
> >>>>
> >>>>    object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> >>>> -                                  &acpi_enable_cmd, NULL);
> >>>> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >>>> -                                  &acpi_disable_cmd, NULL);
> >>>> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> >>>> -                                  &gpe0_blk, NULL);
> >>>> +                                  &gpe0_blk, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> >>>> -                                  &gpe0_blk_len, NULL);
> >>>> +                                  &gpe0_blk_len, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> >>>> -                                  &sci_int, NULL);
> >>>> +                                  &sci_int, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> >>>> -                                  &s->io_base, NULL);
> >>>> +                                  &s->io_base, OBJ_PROP_FLAG_RD, NULL);
> >>>> }
> >>>>
> >>>> static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> >>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >>>> index 17c292e306..f5526f9c3b 100644
> >>>> --- a/hw/isa/lpc_ich9.c
> >>>> +++ b/hw/isa/lpc_ich9.c
> >>>> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> >>>>                        ich9_lpc_get_sci_int,
> >>>>                        NULL, NULL, NULL, NULL);
> >>>>    object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> >>>> -                                  &acpi_enable_cmd, NULL);
> >>>> +                                  &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >>>> -                                  &acpi_disable_cmd, NULL);
> >>>> +                                  &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
> >>>>
> >>>>    ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
> >>>> }
> >>>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >>>> index 62f1a42592..ace2db0413 100644
> >>>> --- a/hw/ppc/spapr_drc.c
> >>>> +++ b/hw/ppc/spapr_drc.c
> >>>> @@ -553,7 +553,7 @@ static void spapr_dr_connector_instance_init(Object *obj)
> >>>>    SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
> >>>>    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >>>>
> >>>> -    object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
> >>>> +    object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_RD, NULL);
> >>>>    object_property_add(obj, "index", "uint32", prop_get_index,
> >>>>                        NULL, NULL, NULL, NULL);
> >>>>    object_property_add(obj, "fdt", "struct", prop_get_fdt,
> >>>> diff --git a/include/qom/object.h b/include/qom/object.h
> >>>> index 128d00c77f..4836c54e93 100644
> >>>> --- a/include/qom/object.h
> >>>> +++ b/include/qom/object.h
> >>>> @@ -1579,65 +1579,91 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
> >>>>                                  void (*get)(Object *, struct tm *, Error **),
> >>>>                                  Error **errp);
> >>>>
> >>>> +typedef enum {
> >>>> +    /* Automatically add a getter to the property */
> >>>> +    OBJ_PROP_FLAG_RD = (1UL << 0),
> >>>> +    /* Automatically add a setter to the property */
> >>>> +    OBJ_PROP_FLAG_WR = (1UL << 1),
> >>>> +} ObjectPropertyFlags;
> >>>> +
> >>>> /**
> >>>> * object_property_add_uint8_ptr:
> >>>> * @obj: the object to add a property to
> >>>> * @name: the name of the property
> >>>> * @v: pointer to value
> >>>> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>>> * @errp: if an error occurs, a pointer to an area to store the error
> >>>> *
> >>>> * Add an integer property in memory.  This function will add a
> >>>> * property of type 'uint8'.
> >>>> */
> >>>> void object_property_add_uint8_ptr(Object *obj, const char *name,
> >>>> -                                   const uint8_t *v, Error **errp);
> >>>> +                                   const uint8_t *v, ObjectPropertyFlags flags,
> >>>> +                                   Error **errp);
> >>>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> >>>> -                                         const uint8_t *v, Error **errp);
> >>>> +                                         const uint8_t *v,
> >>>> +                                         ObjectPropertyFlags flags,
> >>>> +                                         Error **errp);
> >>>>
> >>>> /**
> >>>> * object_property_add_uint16_ptr:
> >>>> * @obj: the object to add a property to
> >>>> * @name: the name of the property
> >>>> * @v: pointer to value
> >>>> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>>> * @errp: if an error occurs, a pointer to an area to store the error
> >>>> *
> >>>> * Add an integer property in memory.  This function will add a
> >>>> * property of type 'uint16'.
> >>>> */
> >>>> void object_property_add_uint16_ptr(Object *obj, const char *name,
> >>>> -                                    const uint16_t *v, Error **errp);
> >>>> +                                    const uint16_t *v,
> >>>> +                                    ObjectPropertyFlags flags,
> >>>> +                                    Error **errp);
> >>>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> >>>> -                                          const uint16_t *v, Error **errp);
> >>>> +                                          const uint16_t *v,
> >>>> +                                          ObjectPropertyFlags flags,
> >>>> +                                          Error **errp);
> >>>>
> >>>> /**
> >>>> * object_property_add_uint32_ptr:
> >>>> * @obj: the object to add a property to
> >>>> * @name: the name of the property
> >>>> * @v: pointer to value
> >>>> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>>> * @errp: if an error occurs, a pointer to an area to store the error
> >>>> *
> >>>> * Add an integer property in memory.  This function will add a
> >>>> * property of type 'uint32'.
> >>>> */
> >>>> void object_property_add_uint32_ptr(Object *obj, const char *name,
> >>>> -                                    const uint32_t *v, Error **errp);
> >>>> +                                    const uint32_t *v,
> >>>> +                                    ObjectPropertyFlags flags,
> >>>> +                                    Error **errp);
> >>>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> >>>> -                                          const uint32_t *v, Error **errp);
> >>>> +                                          const uint32_t *v,
> >>>> +                                          ObjectPropertyFlags flags,
> >>>> +                                          Error **errp);
> >>>>
> >>>> /**
> >>>> * object_property_add_uint64_ptr:
> >>>> * @obj: the object to add a property to
> >>>> * @name: the name of the property
> >>>> * @v: pointer to value
> >>>> + * @flags: bitwise-or'd ObjectPropertyFlags
> >>>> * @errp: if an error occurs, a pointer to an area to store the error
> >>>> *
> >>>> * Add an integer property in memory.  This function will add a
> >>>> * property of type 'uint64'.
> >>>> */
> >>>> void object_property_add_uint64_ptr(Object *obj, const char *name,
> >>>> -                                    const uint64_t *v, Error **Errp);
> >>>> +                                    const uint64_t *v,
> >>>> +                                    ObjectPropertyFlags flags,
> >>>> +                                    Error **Errp);
> >>>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> >>>> -                                          const uint64_t *v, Error **Errp);
> >>>> +                                          const uint64_t *v,
> >>>> +                                          ObjectPropertyFlags flags,
> >>>> +                                          Error **Errp);
> >>>>
> >>>> /**
> >>>> * object_property_add_alias:
> >>>> diff --git a/qom/object.c b/qom/object.c
> >>>> index d51b57fba1..6f300b5317 100644
> >>>> --- a/qom/object.c
> >>>> +++ b/qom/object.c
> >>>> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
> >>>>    visit_type_uint8(v, name, &value, errp);
> >>>> }
> >>>>
> >>>> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
> >>>> +                                   void *opaque, Error **errp)
> >>>> +{
> >>>> +    uint8_t *field = opaque;
> >>>> +    uint8_t value;
> >>>> +    Error *local_err = NULL;
> >>>> +
> >>>> +    visit_type_uint8(v, name, &value, &local_err);
> >>>> +    if (local_err) {
> >>>> +        error_propagate(errp, local_err);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    *field = value;
> >>>> +}
> >>>> +
> >>>> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >>>>                                    void *opaque, Error **errp)
> >>>> {
> >>>> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >>>>    visit_type_uint16(v, name, &value, errp);
> >>>> }
> >>>>
> >>>> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >>>> +                                    void *opaque, Error **errp)
> >>>> +{
> >>>> +    uint16_t *field = opaque;
> >>>> +    uint16_t value;
> >>>> +    Error *local_err = NULL;
> >>>> +
> >>>> +    visit_type_uint16(v, name, &value, &local_err);
> >>>> +    if (local_err) {
> >>>> +        error_propagate(errp, local_err);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    *field = value;
> >>>> +}
> >>>> +
> >>>> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >>>>                                    void *opaque, Error **errp)
> >>>> {
> >>>> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >>>>    visit_type_uint32(v, name, &value, errp);
> >>>> }
> >>>>
> >>>> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >>>> +                                    void *opaque, Error **errp)
> >>>> +{
> >>>> +    uint32_t *field = opaque;
> >>>> +    uint32_t value;
> >>>> +    Error *local_err = NULL;
> >>>> +
> >>>> +    visit_type_uint32(v, name, &value, &local_err);
> >>>> +    if (local_err) {
> >>>> +        error_propagate(errp, local_err);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    *field = value;
> >>>> +}
> >>>> +
> >>>> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >>>>                                    void *opaque, Error **errp)
> >>>> {
> >>>> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >>>>    visit_type_uint64(v, name, &value, errp);
> >>>> }
> >>>>
> >>>> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >>>> +                                    void *opaque, Error **errp)
> >>>> +{
> >>>> +    uint64_t *field = opaque;
> >>>> +    uint64_t value;
> >>>> +    Error *local_err = NULL;
> >>>> +
> >>>> +    visit_type_uint64(v, name, &value, &local_err);
> >>>> +    if (local_err) {
> >>>> +        error_propagate(errp, local_err);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    *field = value;
> >>>> +}
> >>>> +
> >>>> void object_property_add_uint8_ptr(Object *obj, const char *name,
> >>>> -                                   const uint8_t *v, Error **errp)
> >>>> +                                   const uint8_t *v,
> >>>> +                                   ObjectPropertyFlags flags,
> >>>> +                                   Error **errp)
> >>>> {
> >>>> -    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> >>>> -                        NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint8_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint8_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_property_add(obj, name, "uint8",
> >>>> +                        getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> >>>> -                                         const uint8_t *v, Error **errp)
> >>>> +                                         const uint8_t *v,
> >>>> +                                         ObjectPropertyFlags flags,
> >>>> +                                         Error **errp)
> >>>> {
> >>>> -    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
> >>>> -                              NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint8_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint8_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_class_property_add(klass, name, "uint8",
> >>>> +                              getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_property_add_uint16_ptr(Object *obj, const char *name,
> >>>> -                                    const uint16_t *v, Error **errp)
> >>>> +                                    const uint16_t *v,
> >>>> +                                    ObjectPropertyFlags flags,
> >>>> +                                    Error **errp)
> >>>> {
> >>>> -    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> >>>> -                        NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint16_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint16_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_property_add(obj, name, "uint16",
> >>>> +                        getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> >>>> -                                          const uint16_t *v, Error **errp)
> >>>> +                                          const uint16_t *v,
> >>>> +                                          ObjectPropertyFlags flags,
> >>>> +                                          Error **errp)
> >>>> {
> >>>> -    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
> >>>> -                              NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint16_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint16_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_class_property_add(klass, name, "uint16",
> >>>> +                              getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_property_add_uint32_ptr(Object *obj, const char *name,
> >>>> -                                    const uint32_t *v, Error **errp)
> >>>> +                                    const uint32_t *v,
> >>>> +                                    ObjectPropertyFlags flags,
> >>>> +                                    Error **errp)
> >>>> {
> >>>> -    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> >>>> -                        NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint32_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint32_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_property_add(obj, name, "uint32",
> >>>> +                        getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> >>>> -                                          const uint32_t *v, Error **errp)
> >>>> +                                          const uint32_t *v,
> >>>> +                                          ObjectPropertyFlags flags,
> >>>> +                                          Error **errp)
> >>>> {
> >>>> -    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
> >>>> -                              NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint32_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint32_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_class_property_add(klass, name, "uint32",
> >>>> +                              getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_property_add_uint64_ptr(Object *obj, const char *name,
> >>>> -                                    const uint64_t *v, Error **errp)
> >>>> +                                    const uint64_t *v,
> >>>> +                                    ObjectPropertyFlags flags,
> >>>> +                                    Error **errp)
> >>>> {
> >>>> -    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> >>>> -                        NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint64_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint64_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_property_add(obj, name, "uint64",
> >>>> +                        getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> >>>> -                                          const uint64_t *v, Error **errp)
> >>>> +                                          const uint64_t *v,
> >>>> +                                          ObjectPropertyFlags flags,
> >>>> +                                          Error **errp)
> >>>> {
> >>>> -    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
> >>>> -                              NULL, NULL, (void *)v, errp);
> >>>> +    ObjectPropertyAccessor *getter = NULL;
> >>>> +    ObjectPropertyAccessor *setter = NULL;
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_RD) == OBJ_PROP_FLAG_RD) {
> >>>> +        getter = property_get_uint64_ptr;
> >>>> +    }
> >>>> +
> >>>> +    if ((flags & OBJ_PROP_FLAG_WR) == OBJ_PROP_FLAG_WR) {
> >>>> +        setter = property_set_uint64_ptr;
> >>>> +    }
> >>>> +
> >>>> +    object_class_property_add(klass, name, "uint64",
> >>>> +                              getter, setter, NULL, (void *)v, errp);
> >>>> }
> >>>>
> >>>> typedef struct {
> >>>> diff --git a/ui/console.c b/ui/console.c
> >>>> index 82d1ddac9c..7d6ef90978 100644
> >>>> --- a/ui/console.c
> >>>> +++ b/ui/console.c
> >>>> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
> >>>>                             object_property_allow_set_link,
> >>>>                             OBJ_PROP_LINK_STRONG,
> >>>>                             &error_abort);
> >>>> -    object_property_add_uint32_ptr(obj, "head",
> >>>> -                                   &s->head, &error_abort);
> >>>> +    object_property_add_uint32_ptr(obj, "head", &s->head,
> >>>> +                                   OBJ_PROP_FLAG_RD, &error_abort);
> >>>>
> >>>>    if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
> >>>>        (console_type == GRAPHIC_CONSOLE))) {
> >>>> --
> >>>> 2.20.1
> >>>>
> >>>
> >>>
> >>> looks good otherwise
> >>
> >> Thanks,
> >> Felipe
> >>
> >>
> >>>
> >>
> >>> --
> >>> Marc-André Lureau
> >>
> >
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

[PATCH v2 2/4] ich9: fix getter type for sci_int property

[Felipe Franciosi]

When QOM APIs were added to ich9 in 6f1426ab, the getter for sci_int was
written using uint32_t. However, the object property is uint8_t. This
fixes the getter for correctness.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 hw/isa/lpc_ich9.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index f5526f9c3b..3a9c4f0503 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -631,9 +631,7 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
                                  void *opaque, Error **errp)
 {
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
-    uint32_t value = lpc->sci_gsi;
-
-    visit_type_uint32(v, name, &value, errp);
+    visit_type_uint8(v, name, &lpc->sci_gsi, errp);
 }
 
 static void ich9_lpc_add_properties(ICH9LPCState *lpc)
@@ -641,7 +639,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
     static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
     static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
-    object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint32",
+    object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
                         ich9_lpc_get_sci_int,
                         NULL, NULL, NULL, NULL);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-- 
2.20.1

Re: [PATCH v2 2/4] ich9: fix getter type for sci_int property

[Marc-André Lureau]

On Thu, Nov 28, 2019 at 8:48 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> When QOM APIs were added to ich9 in 6f1426ab, the getter for sci_int was
> written using uint32_t. However, the object property is uint8_t. This
> fixes the getter for correctness.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/isa/lpc_ich9.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index f5526f9c3b..3a9c4f0503 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -631,9 +631,7 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
>                                   void *opaque, Error **errp)
>  {
>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> -    uint32_t value = lpc->sci_gsi;
> -
> -    visit_type_uint32(v, name, &value, errp);
> +    visit_type_uint8(v, name, &lpc->sci_gsi, errp);
>  }
>
>  static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> @@ -641,7 +639,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>      static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
>      static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>
> -    object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint32",
> +    object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
>                          ich9_lpc_get_sci_int,
>                          NULL, NULL, NULL, NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> --
> 2.20.1
>


-- 
Marc-André Lureau

[PATCH v2 3/4] ich9: Simplify ich9_lpc_initfn

[Felipe Franciosi]

Currently, ich9_lpc_initfn simply serves as a caller to
ich9_lpc_add_properties. This simplifies the code a bit by eliminating
ich9_lpc_add_properties altogether and executing its logic in the parent
object initialiser function.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 hw/isa/lpc_ich9.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 3a9c4f0503..9a5457c83b 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -634,12 +634,14 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
     visit_type_uint8(v, name, &lpc->sci_gsi, errp);
 }
 
-static void ich9_lpc_add_properties(ICH9LPCState *lpc)
+static void ich9_lpc_initfn(Object *obj)
 {
+    ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
+
     static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
     static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
-    object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
+    object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
                         ich9_lpc_get_sci_int,
                         NULL, NULL, NULL, NULL);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
@@ -647,14 +649,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
                                   &acpi_disable_cmd, OBJ_PROP_FLAG_RD, NULL);
 
-    ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
-}
-
-static void ich9_lpc_initfn(Object *obj)
-{
-    ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
-
-    ich9_lpc_add_properties(lpc);
+    ich9_pm_add_properties(obj, &lpc->pm, NULL);
 }
 
 static void ich9_lpc_realize(PCIDevice *d, Error **errp)
-- 
2.20.1

[PATCH v2 4/4] qom/object: Use common get/set uint helpers

[Felipe Franciosi]

Several objects implemented their own uint property getters and setters,
despite them being straightforward (without any checks/validations on
the values themselves) and identical across objects. This makes use of
an enhanced API for object_property_add_uintXX_ptr() which offers
default setters.

Some of these setters used to update the value even if the type visit
failed (eg. because the value being set overflowed over the given type).
The new setter introduces a check for these errors, not updating the
value if an error occurred. The error is propagated.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 hw/acpi/ich9.c       |  99 +++++-----------------------------------
 hw/isa/lpc_ich9.c    |  13 ++----
 hw/misc/edu.c        |  14 ++----
 hw/pci-host/q35.c    |  14 ++----
 hw/ppc/spapr.c       |  19 ++------
 hw/vfio/pci-quirks.c |  20 +++-----
 memory.c             |  15 +-----
 target/arm/cpu.c     |  23 ++--------
 target/i386/sev.c    | 106 ++++---------------------------------------
 9 files changed, 48 insertions(+), 275 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 236300d2a9..e1bb1afb69 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
     s->pm.cpu_hotplug_legacy = value;
 }
 
-static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    ICH9LPCPMRegs *pm = opaque;
-    uint8_t value = pm->disable_s3;
-
-    visit_type_uint8(v, name, &value, errp);
-}
-
-static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    ICH9LPCPMRegs *pm = opaque;
-    Error *local_err = NULL;
-    uint8_t value;
-
-    visit_type_uint8(v, name, &value, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    pm->disable_s3 = value;
-out:
-    error_propagate(errp, local_err);
-}
-
-static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    ICH9LPCPMRegs *pm = opaque;
-    uint8_t value = pm->disable_s4;
-
-    visit_type_uint8(v, name, &value, errp);
-}
-
-static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    ICH9LPCPMRegs *pm = opaque;
-    Error *local_err = NULL;
-    uint8_t value;
-
-    visit_type_uint8(v, name, &value, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    pm->disable_s4 = value;
-out:
-    error_propagate(errp, local_err);
-}
-
-static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
-{
-    ICH9LPCPMRegs *pm = opaque;
-    uint8_t value = pm->s4_val;
-
-    visit_type_uint8(v, name, &value, errp);
-}
-
-static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
-{
-    ICH9LPCPMRegs *pm = opaque;
-    Error *local_err = NULL;
-    uint8_t value;
-
-    visit_type_uint8(v, name, &value, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    pm->s4_val = value;
-out:
-    error_propagate(errp, local_err);
-}
-
 static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
 {
     ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
@@ -468,18 +393,18 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
                              ich9_pm_get_cpu_hotplug_legacy,
                              ich9_pm_set_cpu_hotplug_legacy,
                              NULL);
-    object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
-                        ich9_pm_get_disable_s3,
-                        ich9_pm_set_disable_s3,
-                        NULL, pm, NULL);
-    object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
-                        ich9_pm_get_disable_s4,
-                        ich9_pm_set_disable_s4,
-                        NULL, pm, NULL);
-    object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
-                        ich9_pm_get_s4_val,
-                        ich9_pm_set_s4_val,
-                        NULL, pm, NULL);
+    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
+                                  &pm->disable_s3,
+                                  OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR,
+                                  NULL);
+    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
+                                  &pm->disable_s4,
+                                  OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR,
+                                  NULL);
+    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
+                                  &pm->s4_val,
+                                  OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR,
+                                  NULL);
     object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
                              ich9_pm_get_enable_tco,
                              ich9_pm_set_enable_tco,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 9a5457c83b..6674ad84c1 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -627,13 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN
 };
 
-static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
-                                 void *opaque, Error **errp)
-{
-    ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
-    visit_type_uint8(v, name, &lpc->sci_gsi, errp);
-}
-
 static void ich9_lpc_initfn(Object *obj)
 {
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
@@ -641,9 +634,9 @@ static void ich9_lpc_initfn(Object *obj)
     static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
     static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
-    object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
-                        ich9_lpc_get_sci_int,
-                        NULL, NULL, NULL, NULL);
+    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
+                                  &lpc->sci_gsi, OBJ_PROP_FLAG_RD,
+                                  NULL);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
                                   &acpi_enable_cmd, OBJ_PROP_FLAG_RD, NULL);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index d5e2bdbb57..14058aa57b 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -396,21 +396,15 @@ static void pci_edu_uninit(PCIDevice *pdev)
     msi_uninit(pdev);
 }
 
-static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
-                           void *opaque, Error **errp)
-{
-    uint64_t *val = opaque;
-
-    visit_type_uint64(v, name, val, errp);
-}
-
 static void edu_instance_init(Object *obj)
 {
     EduState *edu = EDU(obj);
 
     edu->dma_mask = (1UL << 28) - 1;
-    object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
-                    edu_obj_uint64, NULL, &edu->dma_mask, NULL);
+    object_property_add_uint64_ptr(obj, "dma_mask",
+                                   &edu->dma_mask,
+                                   OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR,
+                                   NULL);
 }
 
 static void edu_class_init(ObjectClass *class, void *data)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 158d270b9f..e50ebcfc0f 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
     visit_type_uint64(v, name, &value, errp);
 }
 
-static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
-                                    void *opaque, Error **errp)
-{
-    PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
-
-    visit_type_uint64(v, name, &e->size, errp);
-}
-
 /*
  * NOTE: setting defaults for the mch.* fields in this table
  * doesn't work, because mch is a separate QOM object that is
@@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
     PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+    PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
 
     memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
                           "pci-conf-idx", 4);
@@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
                         q35_host_get_pci_hole64_end,
                         NULL, NULL, NULL, NULL);
 
-    object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
-                        q35_host_get_mmcfg_size,
-                        NULL, NULL, NULL, NULL);
+    object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
+                                   &pehb->size, OBJ_PROP_FLAG_RD, NULL);
 
     object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.ram_memory,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e076f6023c..74f47ced32 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const char *value, Error **errp)
     }
 }
 
-static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    visit_type_uint32(v, name, (uint32_t *)opaque, errp);
-}
-
-static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    visit_type_uint32(v, name, (uint32_t *)opaque, errp);
-}
-
 static char *spapr_get_ic_mode(Object *obj, Error **errp)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(obj);
@@ -3336,8 +3324,11 @@ static void spapr_instance_init(Object *obj)
     object_property_set_description(obj, "resize-hpt",
                                     "Resizing of the Hash Page Table (enabled, disabled, required)",
                                     NULL);
-    object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
-                        spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
+    object_property_add_uint32_ptr(obj, "vsmt",
+                                   &spapr->vsmt,
+                                   OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR,
+                                   &error_abort);
+
     object_property_set_description(obj, "vsmt",
                                     "Virtual SMT: KVM behaves as if this were"
                                     " the host's SMT mode", &error_abort);
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 136f3a9ad6..97492cfd51 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
     return 0;
 }
 
-static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
-                                     const char *name,
-                                     void *opaque, Error **errp)
-{
-    uint64_t tgt = (uintptr_t) opaque;
-    visit_type_uint64(v, name, &tgt, errp);
-}
-
 static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
                                                  const char *name,
                                                  void *opaque, Error **errp)
@@ -2240,9 +2232,9 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
                                nv2reg->size, p);
     QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
 
-    object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
-                        vfio_pci_nvlink2_get_tgt, NULL, NULL,
-                        (void *) (uintptr_t) cap->tgt, NULL);
+    object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
+                                   (void *)(uintptr_t)cap->tgt,
+                                   OBJ_PROP_FLAG_RD, NULL);
     trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
                                           nv2reg->size);
 free_exit:
@@ -2301,9 +2293,9 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error **errp)
         QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
     }
 
-    object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
-                        vfio_pci_nvlink2_get_tgt, NULL, NULL,
-                        (void *) (uintptr_t) captgt->tgt, NULL);
+    object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
+                                   (void *)(uintptr_t)captgt->tgt,
+                                   OBJ_PROP_FLAG_RD, NULL);
     trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, captgt->tgt,
                                               atsdreg->size);
 
diff --git a/memory.c b/memory.c
index 06484c2bff..97861ce30e 100644
--- a/memory.c
+++ b/memory.c
@@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
     memory_region_do_init(mr, owner, name, size);
 }
 
-static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    MemoryRegion *mr = MEMORY_REGION(obj);
-    uint64_t value = mr->addr;
-
-    visit_type_uint64(v, name, &value, errp);
-}
-
 static void memory_region_get_container(Object *obj, Visitor *v,
                                         const char *name, void *opaque,
                                         Error **errp)
@@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
                              NULL, NULL, &error_abort);
     op->resolve = memory_region_resolve_container;
 
-    object_property_add(OBJECT(mr), "addr", "uint64",
-                        memory_region_get_addr,
-                        NULL, /* memory_region_set_addr */
-                        NULL, NULL, &error_abort);
+    object_property_add_uint64_ptr(OBJECT(mr), "addr",
+                                   &mr->addr, OBJ_PROP_FLAG_RD, &error_abort);
     object_property_add(OBJECT(mr), "priority", "uint32",
                         memory_region_get_priority,
                         NULL, /* memory_region_set_priority */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7a4ac9339b..3fddd61ab3 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
     cpu->has_pmu = value;
 }
 
-static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    visit_type_uint32(v, name, &cpu->init_svtor, errp);
-}
-
-static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    visit_type_uint32(v, name, &cpu->init_svtor, errp);
-}
-
 void arm_cpu_post_init(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -1165,9 +1149,10 @@ void arm_cpu_post_init(Object *obj)
          * a simple DEFINE_PROP_UINT32 for this because we want to permit
          * the property to be set after realize.
          */
-        object_property_add(obj, "init-svtor", "uint32",
-                            arm_get_init_svtor, arm_set_init_svtor,
-                            NULL, NULL, &error_abort);
+        object_property_add_uint32_ptr(obj, "init-svtor",
+                                       &cpu->init_svtor,
+                                       OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR,
+                                       &error_abort);
     }
 
     qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 024bb24e51..5b63a27c91 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
             "guest owners session parameters (encoded with base64)", NULL);
 }
 
-static void
-qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
-                      void *opaque, Error **errp)
-{
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-    uint32_t value;
-
-    visit_type_uint32(v, name, &value, errp);
-    sev->handle = value;
-}
-
-static void
-qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
-                      void *opaque, Error **errp)
-{
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-    uint32_t value;
-
-    visit_type_uint32(v, name, &value, errp);
-    sev->policy = value;
-}
-
-static void
-qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
-                       void *opaque, Error **errp)
-{
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-    uint32_t value;
-
-    visit_type_uint32(v, name, &value, errp);
-    sev->cbitpos = value;
-}
-
-static void
-qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-    uint32_t value;
-
-    visit_type_uint32(v, name, &value, errp);
-    sev->reduced_phys_bits = value;
-}
-
-static void
-qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
-                      void *opaque, Error **errp)
-{
-    uint32_t value;
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
-    value = sev->policy;
-    visit_type_uint32(v, name, &value, errp);
-}
-
-static void
-qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
-                      void *opaque, Error **errp)
-{
-    uint32_t value;
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
-    value = sev->handle;
-    visit_type_uint32(v, name, &value, errp);
-}
-
-static void
-qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
-                       void *opaque, Error **errp)
-{
-    uint32_t value;
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
-    value = sev->cbitpos;
-    visit_type_uint32(v, name, &value, errp);
-}
-
-static void
-qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
-                                   void *opaque, Error **errp)
-{
-    uint32_t value;
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
-    value = sev->reduced_phys_bits;
-    visit_type_uint32(v, name, &value, errp);
-}
-
 static void
 qsev_guest_init(Object *obj)
 {
@@ -361,15 +273,15 @@ qsev_guest_init(Object *obj)
 
     sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
     sev->policy = DEFAULT_GUEST_POLICY;
-    object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
-                        qsev_guest_set_policy, NULL, NULL, NULL);
-    object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
-                        qsev_guest_set_handle, NULL, NULL, NULL);
-    object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
-                        qsev_guest_set_cbitpos, NULL, NULL, NULL);
-    object_property_add(obj, "reduced-phys-bits", "uint32",
-                        qsev_guest_get_reduced_phys_bits,
-                        qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
+    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
+                                   OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR, NULL);
+    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
+                                   OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR, NULL);
+    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
+                                   OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR, NULL);
+    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
+                                   &sev->reduced_phys_bits,
+                                   OBJ_PROP_FLAG_RD | OBJ_PROP_FLAG_WR, NULL);
 }
 
 /* sev guest info */
-- 
2.20.1