| Message ID | 1369738859-16557-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <akong@redhat.com> > -- > 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 = { >
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 <akong@redhat.com> > -- > 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 <akong@redhat.com> 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 <akong@redhat.com> -- 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 >
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 <akong@redhat.com> > > -- > > 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
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 <akong@redhat.com> >>> -- >>> 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: > 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. 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 >
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 = {
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 <akong@redhat.com> -- 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(+)