[Qemu-devel] [PATCH 0/7] virtio-blk: use alias properties in transport devices

[Qemu-devel] [PATCH 0/7] virtio-blk: use alias properties in transport devices

[Stefan Hajnoczi]

Thanks for the feedback on the RFC.  This time around the alias property is
implemented at the QOM property level instead of at the qdev property level.

The virtio transport/device split is broken as follows:

1. The virtio-blk device is never finalized because the transport devices
   (virtio-blk-pci and friends) leak the refcount.

2. If we fix the refcount leak then we double-free the 'serial' string property
   upon hot unplug since its char* is copied into the virtio-blk device which
   has an identical 'serial' qdev property.

This series solves both of these problems as follows:

1. Introduce a QOM alias property that lets the transport device forward
   property accesses into the virtio device (the child).

2. Use alias properties in transport devices, instead of keeping a duplicate
   copy of the VirtIOBlkConf struct.

3. Fix the virtio-blk device refcount leak.  It's now safe to do this since the
   double-free has been resolved.

Stefan Hajnoczi (7):
  qom: add object_property_add_alias()
  virtio-blk: avoid qdev property definition duplication
  virtio-blk: move x-data-plane qdev property to virtio-blk.h
  virtio-blk: use aliases instead of duplicate qdev properties
  virtio-blk: drop virtio_blk_set_conf()
  virtio: fix virtio-blk child refcount in transports
  virtio-blk: move qdev properties into virtio-blk.c

 hw/block/virtio-blk.c          | 42 ++++++++++++++++++++++++++++------
 hw/s390x/s390-virtio-bus.c     | 10 ++------
 hw/s390x/s390-virtio-bus.h     |  1 -
 hw/s390x/virtio-ccw.c          |  7 ++----
 hw/s390x/virtio-ccw.h          |  1 -
 hw/virtio/virtio-pci.c         |  7 ++----
 hw/virtio/virtio-pci.h         |  1 -
 include/hw/virtio/virtio-blk.h | 19 +--------------
 include/qom/object.h           | 18 +++++++++++++++
 qom/object.c                   | 52 ++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 112 insertions(+), 46 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Stefan Hajnoczi]

Sometimes an object needs to present a property which is actually on
another object, or it needs to provide an alias name for an existing
property.

Examples:
  a.foo -> b.foo
  a.old_name -> a.new_name

The new object_property_add_alias() API allows objects to alias a
property on another object.  The source and target names can be
different.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qom/object.h | 18 ++++++++++++++++++
 qom/object.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index a641dcd..70cbd13 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1203,6 +1203,24 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                                     const uint64_t *v, Error **Errp);
 
 /**
+ * object_property_add_alias:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @target_obj: the object to forward property access property to
+ * @target_name: the name of the property on the forwarded object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Add an alias for a property on an object.  This function will add a property
+ * of the same type as the forwarded property.
+ *
+ * The caller must ensure that <code>@target_obj</code> stays alive as long as
+ * this property exists.
+ */
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *target_obj, const char *target_name,
+                               Error **errp);
+
+/**
  * object_child_foreach:
  * @obj: the object whose children will be navigated
  * @fn: the iterator function to be called
diff --git a/qom/object.c b/qom/object.c
index e42b254..ff50f37 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1515,6 +1515,58 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+typedef struct
+{
+    Object *target_obj;
+    const char *target_name;
+} AliasProperty;
+
+static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    AliasProperty *prop = opaque;
+
+    object_property_get(prop->target_obj, v, prop->target_name, errp);
+}
+
+static void property_set_alias(Object *obj, struct Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    AliasProperty *prop = opaque;
+
+    object_property_set(prop->target_obj, v, prop->target_name, errp);
+}
+
+static void property_release_alias(Object *obj, const char *name, void *opaque)
+{
+    AliasProperty *prop = opaque;
+
+    g_free(prop);
+}
+
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *target_obj, const char *target_name,
+                               Error **errp)
+{
+    AliasProperty *prop;
+    ObjectProperty *target_prop;
+
+    target_prop = object_property_find(target_obj, target_name, errp);
+    if (!target_prop) {
+        return;
+    }
+
+    prop = g_malloc(sizeof(*prop));
+    prop->target_obj = target_obj;
+    prop->target_name = target_name;
+
+    object_property_add(obj, name, target_prop->type,
+                        property_get_alias,
+                        property_set_alias,
+                        property_release_alias,
+                        prop, errp);
+}
+
 static void object_instance_init(Object *obj)
 {
     object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
-- 
1.9.0

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Paolo Bonzini]

Il 21/05/2014 22:22, Stefan Hajnoczi ha scritto:
> Sometimes an object needs to present a property which is actually on
> another object, or it needs to provide an alias name for an existing
> property.
>
> Examples:
>   a.foo -> b.foo
>   a.old_name -> a.new_name
>
> The new object_property_add_alias() API allows objects to alias a
> property on another object.  The source and target names can be
> different.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/qom/object.h | 18 ++++++++++++++++++
>  qom/object.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index a641dcd..70cbd13 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1203,6 +1203,24 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>                                      const uint64_t *v, Error **Errp);
>
>  /**
> + * object_property_add_alias:
> + * @obj: the object to add a property to
> + * @name: the name of the property
> + * @target_obj: the object to forward property access property to
> + * @target_name: the name of the property on the forwarded object
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Add an alias for a property on an object.  This function will add a property
> + * of the same type as the forwarded property.
> + *
> + * The caller must ensure that <code>@target_obj</code> stays alive as long as
> + * this property exists.
> + */
> +void object_property_add_alias(Object *obj, const char *name,
> +                               Object *target_obj, const char *target_name,
> +                               Error **errp);
> +
> +/**
>   * object_child_foreach:
>   * @obj: the object whose children will be navigated
>   * @fn: the iterator function to be called
> diff --git a/qom/object.c b/qom/object.c
> index e42b254..ff50f37 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1515,6 +1515,58 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>                          NULL, NULL, (void *)v, errp);
>  }
>
> +typedef struct
> +{
> +    Object *target_obj;
> +    const char *target_name;
> +} AliasProperty;
> +
> +static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
> +                               const char *name, Error **errp)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    object_property_get(prop->target_obj, v, prop->target_name, errp);
> +}
> +
> +static void property_set_alias(Object *obj, struct Visitor *v, void *opaque,
> +                               const char *name, Error **errp)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    object_property_set(prop->target_obj, v, prop->target_name, errp);
> +}
> +
> +static void property_release_alias(Object *obj, const char *name, void *opaque)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    g_free(prop);
> +}
> +
> +void object_property_add_alias(Object *obj, const char *name,
> +                               Object *target_obj, const char *target_name,
> +                               Error **errp)
> +{
> +    AliasProperty *prop;
> +    ObjectProperty *target_prop;
> +
> +    target_prop = object_property_find(target_obj, target_name, errp);
> +    if (!target_prop) {
> +        return;
> +    }
> +
> +    prop = g_malloc(sizeof(*prop));
> +    prop->target_obj = target_obj;
> +    prop->target_name = target_name;
> +
> +    object_property_add(obj, name, target_prop->type,
> +                        property_get_alias,
> +                        property_set_alias,
> +                        property_release_alias,
> +                        prop, errp);
> +}

ref target_obj here, and unref in property_release_alias?

Paolo

>  static void object_instance_init(Object *obj)
>  {
>      object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
>

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Stefan Hajnoczi]

On Thu, May 22, 2014 at 12:05 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/05/2014 22:22, Stefan Hajnoczi ha scritto:
>> +void object_property_add_alias(Object *obj, const char *name,
>> +                               Object *target_obj, const char
>> *target_name,
>> +                               Error **errp)
>> +{
>> +    AliasProperty *prop;
>> +    ObjectProperty *target_prop;
>> +
>> +    target_prop = object_property_find(target_obj, target_name, errp);
>> +    if (!target_prop) {
>> +        return;
>> +    }
>> +
>> +    prop = g_malloc(sizeof(*prop));
>> +    prop->target_obj = target_obj;
>> +    prop->target_name = target_name;
>> +
>> +    object_property_add(obj, name, target_prop->type,
>> +                        property_get_alias,
>> +                        property_set_alias,
>> +                        property_release_alias,
>> +                        prop, errp);
>> +}
>
>
> ref target_obj here, and unref in property_release_alias?

No.  I originally did this but realized it is probably not a good idea.

1. If you want to alias a property on the same object instance (foo.a
-> foo.b) this would create a refcount leak (unless someone explicitly
deletes this property to bring the refcount back to 1).
2. The virtio callers already have a child reference and I suspect
other callers would too.  Therefore we don't need to do extra
refcounting.

I had an intermediate version of this patch with a flag so you could
tell object_property_add_alias() whether or not to ref the target
object.  But in the end it seems like overengineering since the
refcount case is rare or non-existent.  The refcount is a convenience
feature just for the case where you want to alias to a random object
that you don't otherwise hold a refcount on (I couldn't think of an
example).

Do you see what I mean?

Stefan

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Paolo Bonzini]

Il 22/05/2014 12:23, Stefan Hajnoczi ha scritto:
> I had an intermediate version of this patch with a flag so you could
> tell object_property_add_alias() whether or not to ref the target
> object.  But in the end it seems like overengineering since the
> refcount case is rare or non-existent.  The refcount is a convenience
> feature just for the case where you want to alias to a random object
> that you don't otherwise hold a refcount on (I couldn't think of an
> example).
>
> Do you see what I mean?

Yes, and it makes sense.  Perhaps add a comment for v2.

Paolo

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Peter Crosthwaite]

On Thu, May 22, 2014 at 6:22 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Sometimes an object needs to present a property which is actually on
> another object, or it needs to provide an alias name for an existing
> property.
>
> Examples:
>   a.foo -> b.foo
>   a.old_name -> a.new_name
>
> The new object_property_add_alias() API allows objects to alias a
> property on another object.  The source and target names can be
> different.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/qom/object.h | 18 ++++++++++++++++++
>  qom/object.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index a641dcd..70cbd13 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1203,6 +1203,24 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>                                      const uint64_t *v, Error **Errp);
>
>  /**
> + * object_property_add_alias:
> + * @obj: the object to add a property to
> + * @name: the name of the property
> + * @target_obj: the object to forward property access property to

-extra "property"

> + * @target_name: the name of the property on the forwarded object
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Add an alias for a property on an object.  This function will add a property
> + * of the same type as the forwarded property.
> + *
> + * The caller must ensure that <code>@target_obj</code> stays alive as long as
> + * this property exists.
> + */
> +void object_property_add_alias(Object *obj, const char *name,
> +                               Object *target_obj, const char *target_name,
> +                               Error **errp);
> +
> +/**
>   * object_child_foreach:
>   * @obj: the object whose children will be navigated
>   * @fn: the iterator function to be called
> diff --git a/qom/object.c b/qom/object.c
> index e42b254..ff50f37 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1515,6 +1515,58 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>                          NULL, NULL, (void *)v, errp);
>  }
>
> +typedef struct
> +{
> +    Object *target_obj;
> +    const char *target_name;
> +} AliasProperty;
> +
> +static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
> +                               const char *name, Error **errp)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    object_property_get(prop->target_obj, v, prop->target_name, errp);
> +}
> +
> +static void property_set_alias(Object *obj, struct Visitor *v, void *opaque,
> +                               const char *name, Error **errp)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    object_property_set(prop->target_obj, v, prop->target_name, errp);
> +}
> +
> +static void property_release_alias(Object *obj, const char *name, void *opaque)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    g_free(prop);
> +}
> +
> +void object_property_add_alias(Object *obj, const char *name,
> +                               Object *target_obj, const char *target_name,
> +                               Error **errp)
> +{
> +    AliasProperty *prop;
> +    ObjectProperty *target_prop;
> +
> +    target_prop = object_property_find(target_obj, target_name, errp);
> +    if (!target_prop) {
> +        return;

Not sure silent failure is right here. This should perhaps populate
errp and then caller can decide to assert/ignore/report as needed.

Regards,
Peter

> +    }
> +
> +    prop = g_malloc(sizeof(*prop));
> +    prop->target_obj = target_obj;
> +    prop->target_name = target_name;
> +
> +    object_property_add(obj, name, target_prop->type,
> +                        property_get_alias,
> +                        property_set_alias,
> +                        property_release_alias,
> +                        prop, errp);
> +}
> +
>  static void object_instance_init(Object *obj)
>  {
>      object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
> --
> 1.9.0
>
>

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Andreas Färber]

Am 22.05.2014 16:02, schrieb Peter Crosthwaite:
> On Thu, May 22, 2014 at 6:22 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index a641dcd..70cbd13 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1203,6 +1203,24 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>>                                      const uint64_t *v, Error **Errp);
>>
>>  /**
>> + * object_property_add_alias:
>> + * @obj: the object to add a property to
>> + * @name: the name of the property
>> + * @target_obj: the object to forward property access property to
> 
> -extra "property"

Maybe "properly" was meant? ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Stefan Hajnoczi]

On Fri, May 23, 2014 at 12:02:44AM +1000, Peter Crosthwaite wrote:
> On Thu, May 22, 2014 at 6:22 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > +void object_property_add_alias(Object *obj, const char *name,
> > +                               Object *target_obj, const char *target_name,
> > +                               Error **errp)
> > +{
> > +    AliasProperty *prop;
> > +    ObjectProperty *target_prop;
> > +
> > +    target_prop = object_property_find(target_obj, target_name, errp);
> > +    if (!target_prop) {
> > +        return;
> 
> Not sure silent failure is right here. This should perhaps populate
> errp and then caller can decide to assert/ignore/report as needed.

object_property_find() already populates errp so the error is not
silent.  Did you miss the errp argument to object_property_find()?

Stefan

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Peter Crosthwaite]

On Fri, May 23, 2014 at 12:38 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Fri, May 23, 2014 at 12:02:44AM +1000, Peter Crosthwaite wrote:
>> On Thu, May 22, 2014 at 6:22 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > +void object_property_add_alias(Object *obj, const char *name,
>> > +                               Object *target_obj, const char *target_name,
>> > +                               Error **errp)
>> > +{
>> > +    AliasProperty *prop;
>> > +    ObjectProperty *target_prop;
>> > +
>> > +    target_prop = object_property_find(target_obj, target_name, errp);
>> > +    if (!target_prop) {
>> > +        return;
>>
>> Not sure silent failure is right here. This should perhaps populate
>> errp and then caller can decide to assert/ignore/report as needed.
>
> object_property_find() already populates errp so the error is not
> silent.  Did you miss the errp argument to object_property_find()?
>

Yeh, so what's confused me here is using the function return to detect
error rather than the error mechanism. Its all good. The alternative
is the local_err = NULL / error_propagate pattern.

Out of scope of this work, but should we really be using return value
for error propagation when we have a usable errp argument?

Regards,
Peter

> Stefan
>

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Igor Mammedov]

On Wed, 21 May 2014 22:22:46 +0200
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Sometimes an object needs to present a property which is actually on
> another object, or it needs to provide an alias name for an existing
> property.
> 
> Examples:
>   a.foo -> b.foo
>   a.old_name -> a.new_name
this could be used for x86 cpu features where the same CPUID bit has
different names depending whether CPU is Intel or AMD based.

> 
> The new object_property_add_alias() API allows objects to alias a
> property on      another object.  The source and target names can be
              ^^^ the same or 
> different.

Re: [Qemu-devel] [PATCH 1/7] qom: add object_property_add_alias()

[Stefan Hajnoczi]

On Thu, May 22, 2014 at 04:05:46PM +0200, Igor Mammedov wrote:
> On Wed, 21 May 2014 22:22:46 +0200
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Sometimes an object needs to present a property which is actually on
> > another object, or it needs to provide an alias name for an existing
> > property.
> > 
> > Examples:
> >   a.foo -> b.foo
> >   a.old_name -> a.new_name
> this could be used for x86 cpu features where the same CPUID bit has
> different names depending whether CPU is Intel or AMD based.

Yes

> > 
> > The new object_property_add_alias() API allows objects to alias a
> > property on      another object.  The source and target names can be
>               ^^^ the same or 

Okay, will fix.

[Qemu-devel] [PATCH 2/7] virtio-blk: avoid qdev property definition duplication

[Stefan Hajnoczi]

It becomes unwiedly to duplicate all virtio-blk qdev property
definitions due to an #ifdef.  The C preprocessor syntax makes it a
little hard to resolve this cleanly but we can extract the #ifdef and
call a macro it defines later.

Avoiding duplication is important since it will only get worse when we
move the x-data-plane qdev property here too.  We'd have a combinatorial
explosion since x-data-plane has its own #ifdef.

Suggested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-blk.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index e4c41ff..fa416db 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -137,21 +137,19 @@ typedef struct VirtIOBlock {
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
 #ifdef __linux__
-#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
-        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
-        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
-        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
-        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
-        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),                \
-        DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
+#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field) \
+        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
 #else
+#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)
+#endif
+
 #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
+        DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)                    \
         DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
         DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
         DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
         DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
         DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
-#endif /* __linux__ */
 
 void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
 
-- 
1.9.0

[Qemu-devel] [PATCH 3/7] virtio-blk: move x-data-plane qdev property to virtio-blk.h

[Stefan Hajnoczi]

Move the x-data-plane property.  Originally it was outside since not
every transport may wish to support dataplane.  But that makes little
sense when we have a dedicated CONFIG_VIRTIO_BLK_DATA_PLANE ifdef
already.

This move makes it easier to switch to property aliases in the next
patch.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/s390x/virtio-ccw.c          | 3 ---
 hw/virtio/virtio-pci.c         | 3 ---
 include/hw/virtio/virtio-blk.h | 8 ++++++++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 1cb4e2c..082bb42 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1129,9 +1129,6 @@ static Property virtio_ccw_blk_properties[] = {
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkCcw, blk.data_plane, 0, false),
-#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..0751a1e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1067,9 +1067,6 @@ static Property virtio_blk_pci_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false),
-#endif
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index fa416db..78e7f81 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -143,8 +143,16 @@ typedef struct VirtIOBlock {
 #define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)
 #endif
 
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field) \
+    DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
+#else
+#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)
+#endif
+
 #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
         DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)                    \
+        DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)               \
         DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
         DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
         DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
-- 
1.9.0

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

[Stefan Hajnoczi]

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 <stefanha@redhat.com>
---
 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
-- 
1.9.0

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

[Paolo Bonzini]

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 <stefanha@redhat.com>
> ---
>  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
>

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

[Stefan Hajnoczi]

On Thu, May 22, 2014 at 12:04 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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.

I think your idea is something like:

def qdev_add_alias_properties(obj, dev):
    for qdev_prop in dev:
        object_property_add_alias(obj, qdev_prop.name, dev, qdev_prop.name)

That way we don't explicitly list all properties and duplicate code
when fixing up net, rng, serial, scsi, etc.

I took a quick look at net, rng, and serial.  This approach should
work because the VirtIODevice qdev properties need to be exposed
wholesale on the transport device.

If that's what you're hinting at, I'll try it in v2.

Stefan

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

[Paolo Bonzini]

Il 22/05/2014 12:17, Stefan Hajnoczi ha scritto:
> I took a quick look at net, rng, and serial.  This approach should
> work because the VirtIODevice qdev properties need to be exposed
> wholesale on the transport device.
>
> If that's what you're hinting at, I'll try it in v2.

Yes, exactly.  scsi too.

Paolo

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

[Andreas Färber]

Am 22.05.2014 00:04, schrieb Paolo Bonzini:
> 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.

"type", "realized" and the child<> property for VirtIODevice come to
mind, possibly one or two more.

If we follow a generic scheme, we could add an .instance_post_init hook
for VirtIOPCIProxy iterating over all properties and blacklisting some.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Stefan Hajnoczi]

On Thu, May 22, 2014 at 12:18 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 22.05.2014 00:04, schrieb Paolo Bonzini:
>> 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.
>
> "type", "realized" and the child<> property for VirtIODevice come to
> mind, possibly one or two more.
>
> If we follow a generic scheme, we could add an .instance_post_init hook
> for VirtIOPCIProxy iterating over all properties and blacklisting some.

I think the trick is to alias all the qdev properties, not the QOM
ones.  That way we get all the explicitly declared properties and none
of the implicit ones.

Stefan

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

[Andreas Färber]

Am 22.05.2014 12:24, schrieb Stefan Hajnoczi:
> On Thu, May 22, 2014 at 12:18 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 22.05.2014 00:04, schrieb Paolo Bonzini:
>>> 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.
>>
>> "type", "realized" and the child<> property for VirtIODevice come to
>> mind, possibly one or two more.
>>
>> If we follow a generic scheme, we could add an .instance_post_init hook
>> for VirtIOPCIProxy iterating over all properties and blacklisting some.
> 
> I think the trick is to alias all the qdev properties, not the QOM
> ones.  That way we get all the explicitly declared properties and none
> of the implicit ones.

I wouldn't oppose that, but you then need to iterate over parent classes
until you hit VirtioDeviceClass (or DeviceClass?), to avoid properties
falling through the cracks.

I just figured it easier and in line with your QMP patch to avoid
distinguishing them in new code. But a quick solution is more important
than futureproofness here, so I'll take or ack whatever works here.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Peter Crosthwaite]

On Thu, May 22, 2014 at 8:32 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 22.05.2014 12:24, schrieb Stefan Hajnoczi:
>> On Thu, May 22, 2014 at 12:18 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 22.05.2014 00:04, schrieb Paolo Bonzini:
>>>> 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.
>>>
>>> "type", "realized" and the child<> property for VirtIODevice come to
>>> mind, possibly one or two more.
>>>

link<> ?

>>> If we follow a generic scheme, we could add an .instance_post_init hook
>>> for VirtIOPCIProxy iterating over all properties and blacklisting some.
>>
>> I think the trick is to alias all the qdev properties, not the QOM
>> ones.  That way we get all the explicitly declared properties and none
>> of the implicit ones.
>
> I wouldn't oppose that, but you then need to iterate over parent classes
> until you hit VirtioDeviceClass (or DeviceClass?),

IMO DeviceClass. Makes this whole alias concept non-virtio specific
and it will be more consistent with the existing qdev multi-level
property system. Add it to the qdev core for all to use:

qdev_alias_all_properties(DeviceState *target, Object *forwarder);

Or some such.

Regards,
Peter

 to avoid properties
> falling through the cracks.
>
> I just figured it easier and in line with your QMP patch to avoid
> distinguishing them in new code. But a quick solution is more important
> than futureproofness here, so I'll take or ack whatever works here.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

[Qemu-devel] [PATCH 5/7] virtio-blk: drop virtio_blk_set_conf()

[Stefan Hajnoczi]

This function is no longer used since parent objects now use child
aliases to set the VirtIOBlkConf directly.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c          | 6 ------
 include/hw/virtio/virtio-blk.h | 2 --
 2 files changed, 8 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 52cd3c9..469e2b4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -642,12 +642,6 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
-{
-    VirtIOBlock *s = VIRTIO_BLK(dev);
-    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
-}
-
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
 /* Disable dataplane thread during live migration since it does not
  * update the dirty memory bitmap yet.
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index bea540c..794801c 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -159,8 +159,6 @@ typedef struct VirtIOBlock {
         DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
         DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
-
 void virtio_blk_add_child_aliases(Object *obj, VirtIOBlock *s);
 
 #endif
-- 
1.9.0

[Qemu-devel] [PATCH 6/7] virtio: fix virtio-blk child refcount in transports

[Stefan Hajnoczi]

object_initialize() leaves the object with a refcount of 1.
object_property_add_child() adds its own reference which is dropped
again when the property is deleted.

The upshot of this is that we always have a refcount >= 1.  Upon hot
unplug the virtio-blk child is not finalized!

Drop our reference after the child property has been added to the
parent.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/s390x/s390-virtio-bus.c | 1 +
 hw/s390x/virtio-ccw.c      | 1 +
 hw/virtio/virtio-pci.c     | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index e7efcdb..79744b5 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -191,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);
+    object_unref(OBJECT(&dev->vdev));
     virtio_blk_add_child_aliases(obj, &dev->vdev);
 }
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 9e98713..7cc149e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -738,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);
+    object_unref(OBJECT(&dev->vdev));
     virtio_blk_add_child_aliases(obj, &dev->vdev);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c13fb09..1576786 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1102,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);
+    object_unref(OBJECT(&dev->vdev));
     virtio_blk_add_child_aliases(obj, &dev->vdev);
 }
 
-- 
1.9.0

[Qemu-devel] [PATCH 7/7] virtio-blk: move qdev properties into virtio-blk.c

[Stefan Hajnoczi]

There is no need to make DEFINE_VIRTIO_BLK_PROPERTIES() public.  Inline
it into virtio-blk.c so it cannot be used by mistake from other source
files.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c          | 12 +++++++++++-
 include/hw/virtio/virtio-blk.h | 23 -----------------------
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 469e2b4..9aa86fe 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -773,7 +773,17 @@ void virtio_blk_add_child_aliases(Object *obj, VirtIOBlock *s)
 }
 
 static Property virtio_blk_properties[] = {
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlock, blk),
+    DEFINE_BLOCK_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOBlock, blk.serial),
+    DEFINE_PROP_BIT("config-wce", VirtIOBlock, blk.config_wce, 0, true),
+    DEFINE_PROP_IOTHREAD("x-iothread", VirtIOBlock, blk.iothread),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOBlock, blk.scsi, 0, true),
+#endif
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    DEFINE_PROP_BIT("x-data-plane", VirtIOBlock, blk.data_plane, 0, false),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 794801c..14c1ac0 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -136,29 +136,6 @@ typedef struct VirtIOBlock {
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
-#ifdef __linux__
-#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field) \
-        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
-#else
-#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)
-#endif
-
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field) \
-    DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
-#else
-#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)
-#endif
-
-#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
-        DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)                    \
-        DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)               \
-        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
-        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
-        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
-        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
-        DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
-
 void virtio_blk_add_child_aliases(Object *obj, VirtIOBlock *s);
 
 #endif
-- 
1.9.0