Heya > On Nov 30, 2019, at 7:50 AM, Marc-André Lureau <marcandre.lur...@gmail.com> > wrote: > > Hi > > On Fri, Nov 29, 2019 at 9:46 PM Felipe Franciosi <fel...@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_READ. This maintains the current >> behaviour whilst allowing others to also set OBJ_PROP_FLAG_WRITE (or use >> the more convenient OBJ_PROP_FLAG_READWRITE) in the future (which will >> automatically install a setter). Other flags may be added later. >> >> Signed-off-by: Felipe Franciosi <fel...@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 | 3 +- >> include/qom/object.h | 44 +++++++-- >> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- >> ui/console.c | 4 +- >> 8 files changed, 246 insertions(+), 48 deletions(-) >> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c >> index 2034dd749e..742fb78226 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_READ, >> 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_READ, 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..4dcef372bf 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_READ, >> + &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_READ, &error_abort); >> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len, >> - &error_abort); >> + OBJ_PROP_FLAG_READ, &error_abort); >> } >> >> const VMStateDescription vmstate_acpi_pcihp_pci_status = { >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c >> index 93aec2dd2c..fe05a3ce46 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_READ, >> NULL); >> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD, >> - &acpi_disable_cmd, NULL); >> + &acpi_disable_cmd, OBJ_PROP_FLAG_READ, >> NULL); >> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK, >> - &gpe0_blk, NULL); >> + &gpe0_blk, OBJ_PROP_FLAG_READ, NULL); >> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN, >> - &gpe0_blk_len, NULL); >> + &gpe0_blk_len, OBJ_PROP_FLAG_READ, NULL); >> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT, >> - &sci_int, NULL); >> + &sci_int, OBJ_PROP_FLAG_READ, NULL); >> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE, >> - &s->io_base, NULL); >> + &s->io_base, OBJ_PROP_FLAG_READ, 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..99517c3946 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_READ, >> NULL); >> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD, >> - &acpi_disable_cmd, NULL); >> + &acpi_disable_cmd, OBJ_PROP_FLAG_READ, >> 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..bbd4bf35c7 100644 >> --- a/hw/ppc/spapr_drc.c >> +++ b/hw/ppc/spapr_drc.c >> @@ -553,7 +553,8 @@ 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_READ, >> + 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..d2bfd76741 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -1579,65 +1579,93 @@ 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_READ = 1 << 0, >> + /* Automatically add a setter to the property */ >> + OBJ_PROP_FLAG_WRITE = 1 << 1, >> + /* Automatically add a getter and a setter to the property */ >> + OBJ_PROP_FLAG_READWRITE = (OBJ_PROP_FLAG_READ | OBJ_PROP_FLAG_WRITE), >> +} 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..77c2682296 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint8_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + setter = property_set_uint8_ptr; >> + } >> + >> + object_property_add(obj, name, "uint8", >> + getter, setter, NULL, (void *)v, errp); > > Or just: > > object_property_add(obj, name, "uint8", > flags & OBJ_PROP_FLAG_READ ? property_get_uint8_ptr : NULL, > flags & OBJ_PROP_FLAG_WRITE ? property_set_uint8_ptr : NULL, > NULL, v, errp);
I had that in an earlier patchset and changed it. We briefly discussed the benefits of actually having "flags" (instead of the original "readonly"), which allows for more elaborate (yet generic) uint type getters/setters (like ones that don't accept zero). The expanded model will let us grow into that cleanly. F. > >> } >> >> 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint8_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint16_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint16_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint32_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint32_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint64_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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_READ) == OBJ_PROP_FLAG_READ) { >> + getter = property_get_uint64_ptr; >> + } >> + >> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) { >> + 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..bcbe65e696 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_READ, &error_abort); >> >> if (!active_console || ((active_console->console_type != >> GRAPHIC_CONSOLE) && >> (console_type == GRAPHIC_CONSOLE))) { >> -- >> 2.20.1 >> > > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > > -- > Marc-André Lureau