From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44350) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnEcu-0004Tu-UC for qemu-devel@nongnu.org; Wed, 21 May 2014 18:04:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WnEcl-0003oh-Qe for qemu-devel@nongnu.org; Wed, 21 May 2014 18:04:44 -0400 Received: from mail-ee0-x22e.google.com ([2a00:1450:4013:c00::22e]:42897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnEcl-0003oK-Fd for qemu-devel@nongnu.org; Wed, 21 May 2014 18:04:35 -0400 Received: by mail-ee0-f46.google.com with SMTP id t10so1946621eei.5 for ; Wed, 21 May 2014 15:04:34 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <537D22EC.1090409@redhat.com> Date: Thu, 22 May 2014 00:04:28 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1400703772-3324-1-git-send-email-stefanha@redhat.com> <1400703772-3324-5-git-send-email-stefanha@redhat.com> In-Reply-To: <1400703772-3324-5-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/7] virtio-blk: use aliases instead of duplicate qdev properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , qemu-devel@nongnu.org Cc: Peter Crosthwaite , Andreas Faerber , Frederic Konrad 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 > --- > 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 >