[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_propert
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init |
Date: |
Thu, 19 Feb 2015 11:17:32 +0100 |
On Thu, Feb 19, 2015 at 12:46:34PM +0300, Denis V. Lunev wrote:
> On 19/02/15 12:39, Michael S. Tsirkin wrote:
> >On Thu, Feb 19, 2015 at 12:36:37PM +0300, Denis V. Lunev wrote:
> >>On 19/02/15 12:25, Michael S. Tsirkin wrote:
> >>>On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
> >>>>The idea is that all other virtio devices are calling this helper
> >>>>to merge properties of the proxy device. This is the only difference
> >>>>in between this helper and code in inside virtio_instance_init_common.
> >>>>The patch should not cause any harm as property list in generic balloon
> >>>>code is empty.
> >>>>
> >>>>This also allows to avoid some dummy errors like fixed by this
> >>>> commit 91ba21208839643603e7f7fa5864723c3f371ebe
> >>>> Author: Gonglei <address@hidden>
> >>>> Date: Tue Sep 30 14:10:35 2014 +0800
> >>>> virtio-balloon: fix virtio-balloon child refcount in transports
> >>>>
> >>>>Signed-off-by: Denis V. Lunev <address@hidden>
> >>>>Signed-off-by: Raushaniya Maksudova <address@hidden>
> >>>>Revieved-by: Cornelia Huck <address@hidden>
> >>>>CC: Christian Borntraeger <address@hidden>
> >>>>CC: Anthony Liguori <address@hidden>
> >>>>CC: Michael S. Tsirkin <address@hidden>
> >>>>---
> >>>> hw/s390x/virtio-ccw.c | 5 ++---
> >>>> hw/virtio/virtio-pci.c | 5 ++---
> >>>> 2 files changed, 4 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >>>>index ea236c9..82da894 100644
> >>>>--- a/hw/s390x/virtio-ccw.c
> >>>>+++ b/hw/s390x/virtio-ccw.c
> >>>>@@ -899,9 +899,8 @@ static void
> >>>>balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
> >>>> static void virtio_ccw_balloon_instance_init(Object *obj)
> >>>> {
> >>>> VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
> >>>>- object_initialize(&dev->vdev, sizeof(dev->vdev),
> >>>>TYPE_VIRTIO_BALLOON);
> >>>>- object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev),
> >>>>NULL);
> >>>>- object_unref(OBJECT(&dev->vdev));
> >>>>+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>>>+ TYPE_VIRTIO_BALLOON);
> >>>> object_property_add(obj, "guest-stats", "guest statistics",
> >>>> balloon_ccw_stats_get_all, NULL, NULL, dev,
> >>>> NULL);
> >>>>diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>>>index dde1d73..745324b 100644
> >>>>--- a/hw/virtio/virtio-pci.c
> >>>>+++ b/hw/virtio/virtio-pci.c
> >>>>@@ -1316,9 +1316,8 @@ static void
> >>>>virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
> >>>> static void virtio_balloon_pci_instance_init(Object *obj)
> >>>> {
> >>>> VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
> >>>>- object_initialize(&dev->vdev, sizeof(dev->vdev),
> >>>>TYPE_VIRTIO_BALLOON);
> >>>>- object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev),
> >>>>NULL);
> >>>>- object_unref(OBJECT(&dev->vdev));
> >>>>+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>>>+ TYPE_VIRTIO_BALLOON);
> >>>> object_property_add(obj, "guest-stats", "guest statistics",
> >>>> balloon_pci_stats_get_all, NULL, NULL, dev,
> >>>> NULL);
> >>>OK, but what about this guest-stats property?
> >>>Should it get the same treatment?
> >>>
> >>>>--
> >>>>1.9.1
> >>hmm, IMHO no. init_common is actually do the following
> >>
> >>void virtio_instance_init_common(Object *proxy_obj, void *data,
> >> size_t vdev_size, const char *vdev_name)
> >>{
> >> DeviceState *vdev = data;
> >>
> >> object_initialize(vdev, vdev_size, vdev_name);
> >> object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev),
> >>NULL);
> >> object_unref(OBJECT(vdev));
> >> qdev_alias_all_properties(vdev, proxy_obj);
> >>}
> >>
> >>on the other hand there is the following code in s390
> >>
> >>static void s390_virtio_net_instance_init(Object *obj)
> >>{
> >> VirtIONetS390 *dev = VIRTIO_NET_S390(obj);
> >>
> >> virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >> TYPE_VIRTIO_NET);
> >> object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
> >> "bootindex", &error_abort);
> >>}
> >>
> >>which does not contain guest-stats property.
> >But why doesn't it?
> >Seems like an obvious omission?
> >
> no it is not
>
> cfind . | xargs fgrep "guest-stats"
> ./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats", errp);
> ./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/virtio/virtio-pci.c: object_property_set(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/virtio/virtio-pci.c: object_property_add(obj, "guest-stats", "guest
> statistics",
> ./hw/virtio/virtio-pci.c: object_property_add(obj,
> "guest-stats-polling-interval", "int",
> ./hw/virtio/virtio-balloon.c: visit_start_struct(v, NULL, "guest-stats",
> name, 0, &err);
> ./hw/virtio/virtio-balloon.c: object_property_add(OBJECT(dev),
> "guest-stats", "guest statistics",
> ./hw/virtio/virtio-balloon.c: object_property_add(OBJECT(dev),
> "guest-stats-polling-interval", "int",
> ./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats", errp);
> ./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/s390x/virtio-ccw.c: object_property_set(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/s390x/virtio-ccw.c: object_property_add(obj, "guest-stats", "guest
> statistics",
> ./hw/s390x/virtio-ccw.c: object_property_add(obj,
> "guest-stats-polling-interval", "int",
>
> looking into details this property is registered and defined for balloon
> only
> and provides information about guest memory subsystem. May be the name
> is toooo generic, but it is private to baloon code.
>
> Thus no cure us needed at my opinion
The problem is code duplication: all transports need to know
about these balloon-specific property.
Why isn't it handled by virtio_instance_init_common?
--
MST
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Michael S. Tsirkin, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Denis V. Lunev, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Michael S. Tsirkin, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Denis V. Lunev, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Denis V. Lunev, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Michael S. Tsirkin, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Cornelia Huck, 2015/02/19
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Denis V. Lunev, 2015/02/26
- Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init, Michael S. Tsirkin, 2015/02/26