[2/6] qom: Allow to make integer qom properties writeable

We have helper functions to easily expose integers as QOM object properties.
However, these are read only. 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.

Signed-off-by: Alexander Graf <agraf@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(-)

On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@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 <agraf@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
>
>

On 02.07.14 05:48, Peter Crosthwaite wrote:
> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@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 was trying to think of a nice middle ground between flexibility that's 
needed and flexibility that's required. I think it's very common to have

   * read-only properties
   * read-write properties
   * read-write properties with special handling for the write path 
(checks mostly, boundary or permission)

> 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.

That'd be an alternative approach - I would just open codedly call 
object_property_add with the global helpers then. I'm not sure that's 
incredibly nicer though.

In the code flow you describe above, a read-only accessor would be 
declared the same way it does today. Read-write ones would already have 
to go via object_property_add.

So we either add clumsy code in the actual user files (which we try to 
reduce) or we end up adding another set of _rw functions (which is what 
I did at first - it's ugly) or we add an enum as parameter (which I did 
then, but it's also ugly, see next email).

I'm not passionate about the approach I ended up taking, but I couldn't 
think of a better middle ground. If you are passionate about it though I 
can easily declare the getters public as well and just use 
object_property_add() explicitly.

> 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).

Yeah, I can't really think of too many properties that really should 
have read side effects ;).


Alex