Re: [Qemu-devel] [PATCH v2 4/7] virtio-blk: use aliases instead of duplicate qdev properties

[Peter Crosthwaite]

On Fri, May 23, 2014 at 1:40 AM, Stefan Hajnoczi <address@hidden> wrote:
> virtio-blk-pci, virtio-blk-s390, and virtio-blk-ccw all duplicate the
> qdev properties of their VirtIOBlock child.  This approach does not work
> well with string or pointer properties since we must be careful about
> leaking or double-freeing them.
>
> Use the QOM alias property to forward property accesses to the
> VirtIOBlock child.  This way no duplication is necessary.
>
> Remember to stop calling virtio_blk_set_conf() so that we don't clobber
> the values already set on the VirtIOBlock instance.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> v2:
>  * Add qdev_alias_all_properties() instead of virtio-blk-specific
>    function [Paolo]
> ---
>  hw/core/qdev.c               | 19 +++++++++++++++++++

The commit message proper makes no reference to the new qedev core
feature for alias properties. That said, I think the qdev change would
work nicely as a separate patch and this ones commit message would
stand nicely as-is without the qdev patch content.

>  hw/s390x/s390-virtio-bus.c   |  9 +--------
>  hw/s390x/s390-virtio-bus.h   |  1 -
>  hw/s390x/virtio-ccw.c        |  3 +--
>  hw/s390x/virtio-ccw.h        |  1 -
>  hw/virtio/virtio-pci.c       |  3 +--
>  hw/virtio/virtio-pci.h       |  1 -
>  include/hw/qdev-properties.h |  2 ++
>  8 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 936eae6..e41f5b8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -724,6 +724,25 @@ void qdev_property_add_static(DeviceState *dev, Property 
> *prop,
>      }
>  }
>
> +/* @qdev_alias_all_properties - Add alias properties to the source object for
> + * all qdev properties on the target DeviceState.
> + */
> +void qdev_alias_all_properties(DeviceState *target, Object *source)
> +{
> +    ObjectClass *class;
> +    Property *prop;
> +
> +    class = object_get_class(OBJECT(target));
> +    do {
> +        for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {

Don't de-reference an inline QOM cast macro. DEVICE_CLASS(class) will
need its own local variable. Andreas, can we update QOM conventions
with this finer point? I think we already have "Avoid using cast
macros other than OBJECT() inline" but I remember a thread a while
back along the lines of no de-referencing being the main intention of
this rule.

Regards,
Peter