| Message ID | 1422541482-2839-2-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
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 <arei.gonglei@huawei.com> > Date: Tue Sep 30 14:10:35 2014 +0800 > virtio-balloon: fix virtio-balloon child refcount in transports > > Signed-off-by: Denis V. Lunev <den@openvz.org> > Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> > Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> > CC: Christian Borntraeger <borntraeger@de.ibm.com> > CC: Anthony Liguori <aliguori@amazon.com> > CC: Michael S. Tsirkin <mst@redhat.com> > --- > 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
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 <arei.gonglei@huawei.com> >> Date: Tue Sep 30 14:10:35 2014 +0800 >> virtio-balloon: fix virtio-balloon child refcount in transports >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> >> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> >> CC: Christian Borntraeger <borntraeger@de.ibm.com> >> CC: Anthony Liguori <aliguori@amazon.com> >> CC: Michael S. Tsirkin <mst@redhat.com> >> --- >> 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.
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 <arei.gonglei@huawei.com> > >> Date: Tue Sep 30 14:10:35 2014 +0800 > >> virtio-balloon: fix virtio-balloon child refcount in transports > >> > >>Signed-off-by: Denis V. Lunev <den@openvz.org> > >>Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> > >>Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> > >>CC: Christian Borntraeger <borntraeger@de.ibm.com> > >>CC: Anthony Liguori <aliguori@amazon.com> > >>CC: Michael S. Tsirkin <mst@redhat.com> > >>--- > >> 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?
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 <arei.gonglei@huawei.com> >>>> Date: Tue Sep 30 14:10:35 2014 +0800 >>>> virtio-balloon: fix virtio-balloon child refcount in transports >>>> >>>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>>> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> >>>> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>>> CC: Christian Borntraeger <borntraeger@de.ibm.com> >>>> CC: Anthony Liguori <aliguori@amazon.com> >>>> CC: Michael S. Tsirkin <mst@redhat.com> >>>> --- >>>> 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
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 <arei.gonglei@huawei.com> > >>>> Date: Tue Sep 30 14:10:35 2014 +0800 > >>>> virtio-balloon: fix virtio-balloon child refcount in transports > >>>> > >>>>Signed-off-by: Denis V. Lunev <den@openvz.org> > >>>>Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> > >>>>Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> > >>>>CC: Christian Borntraeger <borntraeger@de.ibm.com> > >>>>CC: Anthony Liguori <aliguori@amazon.com> > >>>>CC: Michael S. Tsirkin <mst@redhat.com> > >>>>--- > >>>> 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?
On 19/02/15 13:17, Michael S. Tsirkin wrote: > 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 <arei.gonglei@huawei.com> >>>>>> Date: Tue Sep 30 14:10:35 2014 +0800 >>>>>> virtio-balloon: fix virtio-balloon child refcount in transports >>>>>> >>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>>>>> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> >>>>>> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>>>>> CC: Christian Borntraeger <borntraeger@de.ibm.com> >>>>>> CC: Anthony Liguori <aliguori@amazon.com> >>>>>> CC: Michael S. Tsirkin <mst@redhat.com> >>>>>> --- >>>>>> 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? > why it should? virtio_instance_init_common is common for all virtio devices including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch move initialization of all common stuff into the common code. Initialization of virtio properties on the top of different transports is a bit different problem which should be touched differently. The same approach is used in the other devices. We can invent here new helper but this is a matter of independent changes set at my taste.
On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote: > >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? > > > why it should? > > virtio_instance_init_common is common for all virtio devices > including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch > move initialization of all common stuff into the common > code. The problem seems common enough, virtio_instance_init_common already works for all properties, why not for these ones?
On Thu, 19 Feb 2015 11:29:27 +0100 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote: > > >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? > > > > > why it should? > > > > virtio_instance_init_common is common for all virtio devices > > including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch > > move initialization of all common stuff into the common > > code. > > The problem seems common enough, virtio_instance_init_common > already works for all properties, why not for these ones? It only works for the properties that are common amongst transports and all devices. Adding a virtio_balloon_init_common() that adds the guest_stats (or a virtio_rng_init_common() that adds rng) is probably not a bad idea, but I'd prefer it as patches on top of these.
On 19/02/15 14:26, Cornelia Huck wrote: > On Thu, 19 Feb 2015 11:29:27 +0100 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote: >>>> 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? >>>> >>> why it should? >>> >>> virtio_instance_init_common is common for all virtio devices >>> including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch >>> move initialization of all common stuff into the common >>> code. >> The problem seems common enough, virtio_instance_init_common >> already works for all properties, why not for these ones? > It only works for the properties that are common amongst transports and > all devices. > > Adding a virtio_balloon_init_common() that adds the guest_stats (or a > virtio_rng_init_common() that adds rng) is probably not a bad idea, but > I'd prefer it as patches on top of these. > ping. Michael, what is the decision?
On Thu, Feb 26, 2015 at 08:01:08PM +0300, Denis V. Lunev wrote: > On 19/02/15 14:26, Cornelia Huck wrote: > >On Thu, 19 Feb 2015 11:29:27 +0100 > >"Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >>On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote: > >>>>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? > >>>> > >>>why it should? > >>> > >>>virtio_instance_init_common is common for all virtio devices > >>>including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch > >>>move initialization of all common stuff into the common > >>>code. > >>The problem seems common enough, virtio_instance_init_common > >>already works for all properties, why not for these ones? > >It only works for the properties that are common amongst transports and > >all devices. > > > >Adding a virtio_balloon_init_common() that adds the guest_stats (or a > >virtio_rng_init_common() that adds rng) is probably not a bad idea, but > >I'd prefer it as patches on top of these. > > > ping. Michael, what is the decision? Pls address this by sending patches on top.
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);
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 <arei.gonglei@huawei.com> Date: Tue Sep 30 14:10:35 2014 +0800 virtio-balloon: fix virtio-balloon child refcount in transports Signed-off-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com> CC: Christian Borntraeger <borntraeger@de.ibm.com> CC: Anthony Liguori <aliguori@amazon.com> CC: Michael S. Tsirkin <mst@redhat.com> --- hw/s390x/virtio-ccw.c | 5 ++--- hw/virtio/virtio-pci.c | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-)