On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <ag...@suse.de> wrote:
> We have helper functions to easily expose integers as QOM object properties.
> However, these are read only.
I think this is a good idea, and _ptr properties should have some write-ability.
> Let's introduce some easy helpers that would
> just write to the values.
>
> We also add a new parameter to the property registration that allows us to
> specify the setter function. If we need special magic to happen, we can make
> it happen in our own setters. Otherwise we can just use one of the provided
> helper setters that implement generic value writes for integers.
>
> With this we can easily create read-write qom variables, but maintain the
> flexibility to diverge slightly from the default case.
>
But I think it's inconsistent that the read path is automatic and
write path required an open-coded callback to be added. If you need
open-codedness then it can be achieved with just raw
object_property_set. The real issue is that the other side (e.g. the
read handler for a side-effected write) will need to be open coded too
leading to mass boiler plate (is this the problem you are trying to
solve?).
I think we can minimise the number of core QOM API use cases while
achieving your goal by:
1: Leaving the _ptr APIs as minimal having no open-coded capability.
2: Adding global trivial getter and setter fns that can be passed to
the lower level object_property_add.
When one side of a property (either read or write) has a side effect,
go open-coded (you can also call the trivial setter/getter from your
open coded fn for the actual ptr read/write to avoid device code
having to manage visitors). Then add the trivial setter/getter for the
other side. LOC should be roughly the same as this soln.
This also supports the rarer case of a property with read side effects
(can't think of a use case yet but i sure it's out there).
Regards,
Peter
> Signed-off-by: Alexander Graf <ag...@suse.de>
> ---
> hw/acpi/ich9.c | 4 +--
> hw/acpi/pcihp.c | 2 +-
> hw/acpi/piix4.c | 12 ++++----
> hw/i386/acpi-build.c | 2 +-
> hw/isa/lpc_ich9.c | 4 +--
> include/qom/object.h | 84
> +++++++++++++++++++++++++++++++++++++++++++++++++---
> qom/object.c | 13 +++++++-
> ui/console.c | 3 +-
> 8 files changed, 105 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e7d6c77..36a3998 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -286,12 +286,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs
> *pm, Error **errp)
> pm->acpi_memory_hotplug.is_enabled = true;
>
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> - &pm->pm_io_base, errp);
> + &pm->pm_io_base, NULL, 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, NULL, 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 fae663a..978b785 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -312,7 +312,7 @@ void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
>
> *bus_bsel = ACPI_PCIHP_BSEL_DEFAULT;
> object_property_add_uint32_ptr(OBJECT(root_bus),
> ACPI_PCIHP_PROP_BSEL,
> - bus_bsel, NULL);
> + bus_bsel, NULL, NULL);
> }
>
> memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index b72b34e..76191fc 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -405,17 +405,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, NULL, NULL);
> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - &acpi_disable_cmd, NULL);
> + &acpi_disable_cmd, NULL, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> - &gpe0_blk, NULL);
> + &gpe0_blk, NULL, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> - &gpe0_blk_len, NULL);
> + &gpe0_blk_len, NULL, NULL);
> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> - &sci_int, NULL);
> + &sci_int, NULL, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> - &s->io_base, NULL);
> + &s->io_base, NULL, NULL);
> }
>
> static int piix4_pm_initfn(PCIDevice *dev)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebc5f03..f2a58ab 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -750,7 +750,7 @@ 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, NULL);
> + bus_bsel, NULL, NULL);
> }
>
> return bsel_alloc;
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index b846d81..8e598d7 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -556,9 +556,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, NULL, NULL);
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - &acpi_disable_cmd, NULL);
> + &acpi_disable_cmd, NULL, NULL);
>
> ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
> }
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8a05a81..deae138 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1207,52 +1207,128 @@ void object_property_add_bool(Object *obj, const
> char *name,
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @set: setter function for this value, pass NULL for read-only
> * @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,
> + ObjectPropertyAccessor *set,
> + Error **errp);
> +
> +/**
> + * property_set_uint8_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint8' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint8_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint8_ptr(Object *obj, struct Visitor *v,
> + void *opaque, const char *name,
> + 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
> + * @set: setter function for this value, pass NULL for read-only
> * @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,
> + ObjectPropertyAccessor *set,
> + Error **errp);
> +
> +/**
> + * property_set_uint16_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint16' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint16_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint16_ptr(Object *obj, struct Visitor *v,
> + void *opaque, const char *name,
> + 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
> + * @set: setter function for this value, pass NULL for read-only
> * @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,
> + ObjectPropertyAccessor *set,
> + Error **errp);
> +
> +/**
> + * property_set_uint32_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint32' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint32_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint32_ptr(Object *obj, struct Visitor *v,
> + void *opaque, const char *name,
> + 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
> + * @set: setter function for this value, pass NULL for read-only
> * @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,
> + ObjectPropertyAccessor *set,
> + Error **Errp);
> +
> +/**
> + * property_set_uint64_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint64' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint64_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint64_ptr(Object *obj, struct Visitor *v,
> + void *opaque, const char *name,
> + Error **errp);
>
> /**
> * object_property_add_alias:
> diff --git a/qom/object.c b/qom/object.c
> index 73cd717..ff13b68 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1518,12 +1518,23 @@ static void
> glue(glue(property_get_,size),_ptr)(Object *obj, Visitor *v, \
> glue(visit_type_,size)(v, &value, name, errp);
> \
> }
> \
>
> \
> +void glue(glue(object_property_set_,size),_ptr)(Object *obj, Visitor *v,
> \
> + void *opaque,
> \
> + const char *name,
> \
> + Error **errp)
> \
> +{
> \
> + glue(size,_t) value;
> \
> + glue(visit_type_,size)(v, &value, name, errp);
> \
> + *(glue(size,_t) *)opaque = value;
> \
> +}
> \
> +
> \
> void glue(glue(object_property_add_,size),_ptr)(Object *obj, const char
> *name, \
> const glue(size,_t) *v,
> \
> + ObjectPropertyAccessor *set,
> \
> Error **errp)
> \
> {
> \
> ObjectPropertyAccessor *get = glue(glue(property_get_,size),_ptr);
> \
> - object_property_add(obj, name, stringify(size), get, NULL, NULL, (void
> *)v,\
> + object_property_add(obj, name, stringify(size), get, set, NULL, (void
> *)v, \
> errp);
> \
> }
> \
>
> diff --git a/ui/console.c b/ui/console.c
> index ab84549..af61ac8 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1195,8 +1195,7 @@ static QemuConsole *new_console(DisplayState *ds,
> console_type_t console_type,
> object_property_allow_set_link,
> OBJ_PROP_LINK_UNREF_ON_RELEASE,
> &error_abort);
> - object_property_add_uint32_ptr(obj, "head",
> - &s->head, &error_abort);
> + object_property_add_uint32_ptr(obj, "head", &s->head, NULL,
> &error_abort);
>
> if (!active_console || ((active_console->console_type !=
> GRAPHIC_CONSOLE) &&
> (console_type == GRAPHIC_CONSOLE))) {
> --
> 1.8.1.4
>
>
