From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOOB1-0007FO-Ut for qemu-devel@nongnu.org; Thu, 19 Feb 2015 05:17:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YOOAt-0004KC-P1 for qemu-devel@nongnu.org; Thu, 19 Feb 2015 05:17:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44630) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOOAt-0004K8-Hd for qemu-devel@nongnu.org; Thu, 19 Feb 2015 05:17:39 -0500 Date: Thu, 19 Feb 2015 11:17:32 +0100 From: "Michael S. Tsirkin" Message-ID: <20150219101732.GA24499@redhat.com> References: <1422541482-2839-1-git-send-email-den@openvz.org> <1422541482-2839-2-git-send-email-den@openvz.org> <20150219092541.GA19380@redhat.com> <54E5AEA5.8030407@openvz.org> <20150219093941.GB19380@redhat.com> <54E5B0FA.3010505@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54E5B0FA.3010505@openvz.org> Subject: Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: cornelia.huck@de.ibm.com, Christian Borntraeger , qemu-devel@nongnu.org, Raushaniya Maksudova 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 > >>>> Date: Tue Sep 30 14:10:35 2014 +0800 > >>>> virtio-balloon: fix virtio-balloon child refcount in transports > >>>> > >>>>Signed-off-by: Denis V. Lunev > >>>>Signed-off-by: Raushaniya Maksudova > >>>>Revieved-by: Cornelia Huck > >>>>CC: Christian Borntraeger > >>>>CC: Anthony Liguori > >>>>CC: Michael S. Tsirkin > >>>>--- > >>>> 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