[PATCH v2] qdev: don't add typename to fw_dev_path when get_fw_dev_path isn't implemented

[Amos Kong]

Recent virtio refactoring in QEMU made virtio-bus become the parent bus
of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
so redundant typename will be added to fw_dev_path. It causes that
bootindex parameter of scsi device doesn't work.

This patch changes qdev_get_fw_dev_path_helper() to add nothing if
implemented get_fw_dev_path() returns NULL.

Signed-off-by: Amos Kong 
--
v2: only add nothing to fw_dev_path when get_fw_dev_path() is
    implemented and return NULL. then it will not effect other devices
    don't have get_fw_dev_path() implementation.
---
 hw/core/qdev.c         | 5 +++++
 hw/virtio/virtio-bus.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6985ad8..2a08368 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -509,11 +509,16 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)

if (dev && dev->parent_bus) {
         char *d;
+        BusClass *parent_bc = BUS_GET_CLASS(dev->parent_bus);
+
         l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
         d = bus_get_fw_dev_path(dev->parent_bus, dev);
+
         if (d) {
             l += snprintf(p + l, size - l, "%s", d);
             g_free(d);
+        } else if (parent_bc->get_fw_dev_path) {
+            return l;
         } else {
             l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
         }
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index ea2e11a..6849a01 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -161,10 +161,16 @@ static char *virtio_bus_get_dev_path(DeviceState *dev)
     return qdev_get_dev_path(proxy);
 }

+static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
+{
+    return NULL;
+}
+
 static void virtio_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *bus_class = BUS_CLASS(klass);
     bus_class->get_dev_path = virtio_bus_get_dev_path;
+    bus_class->get_fw_dev_path = virtio_bus_get_fw_dev_path;
 }

static const TypeInfo virtio_bus_info = {

[PATCH v2] qdev: don't add typename to fw_dev_path when get_fw_dev_path isn't implemented

[Paolo Bonzini]

Il 28/05/2013 13:00, Amos Kong ha scritto:
> Recent virtio refactoring in QEMU made virtio-bus become the parent bus
> of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
> so redundant typename will be added to fw_dev_path. It causes that
> bootindex parameter of scsi device doesn't work.
>
> This patch changes qdev_get_fw_dev_path_helper() to add nothing if
> implemented get_fw_dev_path() returns NULL.
> Signed-off-by: Amos Kong
>
> v2: only add nothing to fw_dev_path when get_fw_dev_path() is
>     implemented and return NULL. then it will not effect other devices
>     don't have get_fw_dev_path() implementation.

To achieve the same result, you can simply ensure that all classes
implement the method.  The default implementation is just
g_strdup(object_get_typename(OBJECT(dev))).

Remember that if you do not specify a method in a BusClass's class_init
function, it is inherited from the superclass.

Paolo
> hw/core/qdev.c         | 5 +++++
>  hw/virtio/virtio-bus.c | 6 ++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6985ad8..2a08368 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -509,11 +509,16 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>  if (dev && dev->parent_bus) {
>      char *d;
>    BusClass *parent_bc = BUS_GET_CLASS(dev->parent_bus);
>    l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>    d = bus_get_fw_dev_path(dev->parent_bus, dev);
>    if (d) {
>        l += snprintf(p + l, size - l, "%s", d);
>        g_free(d);
>    } else if (parent_bc->get_fw_dev_path) {
>        return l;
>    } else {
>        l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>    }
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index ea2e11a..6849a01 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -161,10 +161,16 @@ static char *virtio_bus_get_dev_path(DeviceState *dev)
>      return qdev_get_dev_path(proxy);
>  }
>
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> +{
> return NULL;
>
> +}
>
> static void virtio_bus_class_init(ObjectClass *klass, void *data)
>  {
>      BusClass *bus_class = BUS_CLASS(klass);
>      bus_class->get_dev_path = virtio_bus_get_dev_path;
> bus_class->get_fw_dev_path = virtio_bus_get_fw_dev_path;

This hunk is correct.
> }
>
> static const TypeInfo virtio_bus_info = {

[PATCH v2] qdev: don't add typename to fw_dev_path when get_fw_dev_path isn't implemented

[Amos Kong]

On Tue, May 28, 2013 at 01:15:57PM +0200, Paolo Bonzini wrote:
> Il 28/05/2013 13:00, Amos Kong ha scritto:
> > Recent virtio refactoring in QEMU made virtio-bus become the parent bus
> > of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
> > so redundant typename will be added to fw_dev_path. It causes that
> > bootindex parameter of scsi device doesn't work.
> >
> > This patch changes qdev_get_fw_dev_path_helper() to add nothing if
> > implemented get_fw_dev_path() returns NULL.
> > Signed-off-by: Amos Kong
> >
> > v2: only add nothing to fw_dev_path when get_fw_dev_path() is
> >     implemented and return NULL. then it will not effect other devices
> >     don't have get_fw_dev_path() implementation.

Hi Paolo,
> To achieve the same result, you can simply ensure that all classes
> implement the method.

Do you mean if some classes didn't have an implement, I need to add
a implement (just returns NULL) for all of them, then nothing will be
added to fw_dev_path for those classes?

Currently I'm sure 'typename' is redundant for virio-bus. 'typename'
might be needed for other class (have no get_fw_dev_path implementation)

Applied patch v2, there are three conditions: 
 1) implement get_fw_dev_path, return specific string, add to
    fw_dev_path (same as original)
 2) don't implement get_fw_dev_path, add typename to fw_dev_path.
    (same as original)
 3) implement get_fw_dev_path and return NULL, nothing is added
    to fw_dev_path (new behavior)

If some other class wants to add nothing to path, just define a method
to return NULL.
> The default implementation is just
> g_strdup(object_get_typename(OBJECT(dev))).

If we implement the method for all classes, the default implementation
in qdev_get_fw_dev_path_helper() will not be called.
> Remember that if you do not specify a method in a BusClass's class_init
> function, it is inherited from the superclass.
>
> Paolo

[PATCH v2] qdev: don't add typename to fw_dev_path when get_fw_dev_path isn't implemented

[Paolo Bonzini]

Il 29/05/2013 06:41, Amos Kong ha scritto:
> On Tue, May 28, 2013 at 01:15:57PM +0200, Paolo Bonzini wrote:
> > Il 28/05/2013 13:00, Amos Kong ha scritto:
> > > Recent virtio refactoring in QEMU made virtio-bus become the parent bus
> > > of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
> > > so redundant typename will be added to fw_dev_path. It causes that
> > > bootindex parameter of scsi device doesn't work.
> > >
> > > This patch changes qdev_get_fw_dev_path_helper() to add nothing if
> > > implemented get_fw_dev_path() returns NULL.
> > > Signed-off-by: Amos Kong
> > >
> > > v2: only add nothing to fw_dev_path when get_fw_dev_path() is
> > >     implemented and return NULL. then it will not effect other devices
> > >     don't have get_fw_dev_path() implementation.
>
> Hi Paolo,
> > To achieve the same result, you can simply ensure that all classes
> > implement the method.
>
> Do you mean if some classes didn't have an implement, I need to add
> a implement (just returns NULL) for all of them, then nothing will be
> added to fw_dev_path for those classes?

No, you can have an implementation in BusClass.  See how
device_class_init provides a default implementation for
realize/unrealize, and do the same in bus_class_init.
> Currently I'm sure 'typename' is redundant for virio-bus. 'typename'
> might be needed for other class (have no get_fw_dev_path implementation)
>
> Applied patch v2, there are three conditions:
> implement get_fw_dev_path, return specific string, add to
> fw_dev_path (same as original)
> don't implement get_fw_dev_path, add typename to fw_dev_path.
> (same as original)
> implement get_fw_dev_path and return NULL, nothing is added
> to fw_dev_path (new behavior)
>
> If some other class wants to add nothing to path, just define a method
> to return NULL.
> > The default implementation is just
> > g_strdup(object_get_typename(OBJECT(dev))).
>
> If we implement the method for all classes, the default implementation
> in qdev_get_fw_dev_path_helper() will not be called.

Yes, but we shouldn't implement the method for all classes.  We should
implement it just once in BusClass.  Then the default implementation in
qdev_get_fw_dev_path_helper() can be removed.  It will live in BusClass
instead

Paolo
> > Remember that if you do not specify a method in a BusClass's class_init
> > function, it is inherited from the superclass.
> >
> > Paolo

[Qemu-devel] [PATCH v2] qdev: don't add typename to fw_dev_path when get_fw_dev_path isn't implemented

[Amos Kong]

On Tue, May 28, 2013 at 07:00:59PM +0800, Amos Kong wrote:
> Recent virtio refactoring in QEMU made virtio-bus become the parent bus
> of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
> so redundant typename will be added to fw_dev_path. It causes that
> bootindex parameter of scsi device doesn't work.
>
> This patch changes qdev_get_fw_dev_path_helper() to add nothing if
> implemented get_fw_dev_path() returns NULL.
> Signed-off-by: Amos Kong
>
> v2: only add nothing to fw_dev_path when get_fw_dev_path() is
>     implemented and return NULL. then it will not effect other devices
>     don't have get_fw_dev_path() implementation.

[Reply to myself]

Subject should be fixed, will send v3 later.

commit 3643ed5aad5446c7e3eaf1958ae5fc5dbe8ed2d1
Author: Amos Kong 
Date:   Tue May 28 18:07:19 2013 +0800

qdev: add nothing to fw_dev_path when implemented get_fw_dev_path returns NULL

Recent virtio refactoring in QEMU made virtio-bus become the parent bus
    of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
    so redundant typename will be added to fw_dev_path. It causes that
    bootindex parameter of scsi device doesn't work.

This patch changes qdev_get_fw_dev_path_helper() to add nothing if
    implemented get_fw_dev_path() returns NULL, and implement
    virtio_bus_get_fw_dev_path() to return NULL. Then QEMU will still
    pass original style of fw_dev_path to seabios.

Signed-off-by: Amos Kong 
    --
    v2: only add nothing to fw_dev_path when get_fw_dev_path() is
        implemented and returns NULL. then it will not effect other devices
        don't have get_fw_dev_path() implementation.
    v3: fix subject
> hw/core/qdev.c         | 5 +++++
>  hw/virtio/virtio-bus.c | 6 ++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6985ad8..2a08368 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -509,11 +509,16 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>  if (dev && dev->parent_bus) {
>      char *d;
>    BusClass *parent_bc = BUS_GET_CLASS(dev->parent_bus);
>    l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>    d = bus_get_fw_dev_path(dev->parent_bus, dev);
>    if (d) {
>        l += snprintf(p + l, size - l, "%s", d);
>        g_free(d);
>    } else if (parent_bc->get_fw_dev_path) {
>        return l;
>    } else {
>        l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>    }
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index ea2e11a..6849a01 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -161,10 +161,16 @@ static char *virtio_bus_get_dev_path(DeviceState *dev)
>      return qdev_get_dev_path(proxy);
>  }
>
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> +{
> return NULL;
>
> +}
>
> static void virtio_bus_class_init(ObjectClass *klass, void *data)
>  {
>      BusClass *bus_class = BUS_CLASS(klass);
>      bus_class->get_dev_path = virtio_bus_get_dev_path;
> bus_class->get_fw_dev_path = virtio_bus_get_fw_dev_path;
>
> }
> static const TypeInfo virtio_bus_info = {
>
> 1.8.1.4