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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/05/2014 22:22, Stefan Hajnoczi ha scritto:
> 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.

Which properties are _not_ being added?  This is probably needed for all 
other virtio devices so a generic solution would be nice.

Paolo

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 24 ++++++++++++++++++++++++
>  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/virtio/virtio-blk.h |  2 ++
>  8 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8a568e5..52cd3c9 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -754,6 +754,30 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
>      virtio_cleanup(vdev);
>  }
>
> +void virtio_blk_add_child_aliases(Object *obj, VirtIOBlock *s)
> +{
> +    const char *aliases[] = {"drive", "logical_block_size",
> +        "physical_block_size", "min_io_size", "opt_io_size", "bootindex",
> +        "discard_granularity", "cyls", "heads", "secs", "serial", "config-wce",
> +        "x-iothread",
> +#ifdef __linux__
> +        "scsi",
> +#endif
> +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> +        "x-data-plane",
> +#endif
> +        NULL
> +    };
> +    Object *child = OBJECT(s);
> +    int i;
> +
> +    for (i = 0; aliases[i]; i++) {
> +        object_property_add_alias(obj, aliases[i],
> +                                  child, aliases[i],
> +                                  &error_abort);
> +    }
> +}
> +
>  static Property virtio_blk_properties[] = {
>      DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlock, blk),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 9c71afa..e7efcdb 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);
> +    virtio_blk_add_child_aliases(obj, &dev->vdev);
>  }
>
>  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 082bb42..9e98713 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -725,7 +725,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;
> @@ -739,6 +738,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);
> +    virtio_blk_add_child_aliases(obj, &dev->vdev);
>  }
>
>  static int virtio_ccw_serial_init(VirtioCcwDevice *ccw_dev)
> @@ -1126,7 +1126,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 4393e44..7eea6b9 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -134,7 +134,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..c13fb09 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);
> +    virtio_blk_add_child_aliases(obj, &dev->vdev);
>  }
>
>  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;
>  };
>
>  /*
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 78e7f81..bea540c 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -161,4 +161,6 @@ typedef struct VirtIOBlock {
>
>  void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>
> +void virtio_blk_add_child_aliases(Object *obj, VirtIOBlock *s);
> +
>  #endif
>