Re: [Qemu-devel] [PATCH v3 5/8] virtio-blk: use aliases instead of duplicate qdev properties

[Peter Crosthwaite]

On Fri, May 30, 2014 at 9:17 PM, 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>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
> v3:
>  * Split qdev_alias_all_properties() into its own patch [Peter Crosthwaite]
>
> v2:
>  * Add qdev_alias_all_properties() instead of virtio-blk-specific
>    function [Paolo]
> ---
>  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 -
>  6 files changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 9c71afa..041d4e2 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -179,7 +179,6 @@ static int s390_virtio_blk_init(VirtIOS390Device 
> *s390_dev)
>  {
>      VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> -    virtio_blk_set_conf(vdev, &(dev->blk));
>      qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
>      if (qdev_init(vdev) < 0) {
>          return -1;
> @@ -192,6 +191,7 @@ static void s390_virtio_blk_instance_init(Object *obj)
>      VirtIOBlkS390 *dev = VIRTIO_BLK_S390(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), 
> NULL);
> +    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
>  static int s390_virtio_serial_init(VirtIOS390Device *s390_dev)
> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
>      .class_init    = s390_virtio_net_class_init,
>  };
>
> -static Property s390_virtio_blk_properties[] = {
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
>
>      k->init = s390_virtio_blk_init;
> -    dc->props = s390_virtio_blk_properties;
>  }
>
>  static const TypeInfo s390_virtio_blk = {
> diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h
> index ac81bd8..ffd0df7 100644
> --- a/hw/s390x/s390-virtio-bus.h
> +++ b/hw/s390x/s390-virtio-bus.h
> @@ -124,7 +124,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev);
>  typedef struct VirtIOBlkS390 {
>      VirtIOS390Device parent_obj;
>      VirtIOBlock vdev;
> -    VirtIOBlkConf blk;
>  } VirtIOBlkS390;
>
>  /* virtio-scsi-s390 */
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index f3f53dc..0cc8132 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -800,7 +800,6 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev)
>  {
>      VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> -    virtio_blk_set_conf(vdev, &(dev->blk));
>      qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
>      if (qdev_init(vdev) < 0) {
>          return -1;
> @@ -814,6 +813,7 @@ static void virtio_ccw_blk_instance_init(Object *obj)
>      VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), 
> NULL);
> +    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
>  static int virtio_ccw_serial_init(VirtioCcwDevice *ccw_dev)
> @@ -1309,7 +1309,6 @@ static const TypeInfo virtio_ccw_net = {
>  static Property virtio_ccw_blk_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index b8b8a8a..5a1f16e 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -144,7 +144,6 @@ typedef struct VHostSCSICcw {
>  typedef struct VirtIOBlkCcw {
>      VirtioCcwDevice parent_obj;
>      VirtIOBlock vdev;
> -    VirtIOBlkConf blk;
>  } VirtIOBlkCcw;
>
>  /* virtio-balloon-ccw */
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 0751a1e..3bb782f 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1068,7 +1068,6 @@ static Property virtio_blk_pci_properties[] = {
>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -1076,7 +1075,6 @@ static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev)
>  {
>      VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> -    virtio_blk_set_conf(vdev, &(dev->blk));
>      qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
>      if (qdev_init(vdev) < 0) {
>          return -1;
> @@ -1104,6 +1102,7 @@ static void virtio_blk_pci_instance_init(Object *obj)
>      VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), 
> NULL);
> +    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
>  static const TypeInfo virtio_blk_pci_info = {
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index dc332ae..1cea157 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -131,7 +131,6 @@ struct VHostSCSIPCI {
>  struct VirtIOBlkPCI {
>      VirtIOPCIProxy parent_obj;
>      VirtIOBlock vdev;
> -    VirtIOBlkConf blk;
>  };
>
>  /*
> --
> 1.9.3
>
>