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

[Stefan Hajnoczi]

On Thu, May 29, 2014 at 07:03:42PM +1000, Peter Crosthwaite wrote:
> On Fri, May 23, 2014 at 1:40 AM, Stefan Hajnoczi <address@hidden> wrote:
> > 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.
> >
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> > v2:
> >  * Add qdev_alias_all_properties() instead of virtio-blk-specific
> >    function [Paolo]
> > ---
> >  hw/core/qdev.c               | 19 +++++++++++++++++++
> 
> The commit message proper makes no reference to the new qedev core
> feature for alias properties. That said, I think the qdev change would
> work nicely as a separate patch and this ones commit message would
> stand nicely as-is without the qdev patch content.

Good point, I'll split qdev_alias_all_properties() into a separate
patch.

> >  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/qdev-properties.h |  2 ++
> >  8 files changed, 24 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 936eae6..e41f5b8 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -724,6 +724,25 @@ void qdev_property_add_static(DeviceState *dev, 
> > Property *prop,
> >      }
> >  }
> >
> > +/* @qdev_alias_all_properties - Add alias properties to the source object 
> > for
> > + * all qdev properties on the target DeviceState.
> > + */
> > +void qdev_alias_all_properties(DeviceState *target, Object *source)
> > +{
> > +    ObjectClass *class;
> > +    Property *prop;
> > +
> > +    class = object_get_class(OBJECT(target));
> > +    do {
> > +        for (prop = DEVICE_CLASS(class)->props; prop && prop->name; 
> > prop++) {
> 
> Don't de-reference an inline QOM cast macro. DEVICE_CLASS(class) will
> need its own local variable. Andreas, can we update QOM conventions
> with this finer point? I think we already have "Avoid using cast
> macros other than OBJECT() inline" but I remember a thread a while
> back along the lines of no de-referencing being the main intention of
> this rule.

Sure, this is easy to change.  Could you explain the reasoning behind it
though?

By the way, this is copied from hw/core/qdev.c:device_initfn() where we
also dereference inline.

Stefan