On 29/04/2013 19:55, Andreas Färber wrote:
Am 29.04.2013 19:39, schrieb KONRAD Frédéric:
On 29/04/2013 18:32, Paolo Bonzini wrote:
Il 29/04/2013 18:28, KONRAD Frédéric ha scritto:
Could this be simply a qdev property?
Yes, that can be a good idea.
What about adding a qdev property bus_name and using it in qbus_realize?
Like this:
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4eb0134..c5d5407 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -421,6 +421,13 @@ static void qbus_realize(BusState *bus, DeviceState
*parent, const char *name)
if (name) {
bus->name = g_strdup(name);
+ } else if (bus->parent && bus->parent->bus_name) {
+ /* parent device has bus_name -> use it for bus name */
+ len = strlen(bus->parent->bus_name) + 16;
+ buf = g_malloc(len);
+ snprintf(buf, len, "%s.%d", bus->parent->bus_name,
+ bus->parent->num_child_bus);
+ bus->name = buf;
} else if (bus->parent && bus->parent->id) {
/* parent device has id -> use it for bus name */
len = strlen(bus->parent->id) + 16;
If so, change to scsi_bus_new is not needed and the two new functions
are
not needed.
Is that making sense?
Ah, that's a bit more extreme. :)
I think I like it, but I need more input.
Paolo
Well, just for virtio-scsi-pci, something like that:
---
hw/core/qdev.c | 14 ++++++++++++++
hw/virtio/virtio-pci.c | 9 +++++++++
include/hw/qdev-core.h | 4 ++++
3 files changed, 27 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4eb0134..3aa0082 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -342,6 +342,13 @@ BusState *qdev_get_child_bus(DeviceState *dev,
const char *name)
return NULL;
}
+void qdev_set_bus_name(DeviceState *dev, const char *bus_name)
device_... for new functions please. :)
+{
Might be called multiple times, so better insert:
if (dev->bus_name) {
g_free(dev->bus_name);
dev->bus_name = NULL;
}
+ if (bus_name) {
+ dev->bus_name = g_strdup(bus_name);
+ }
+}
+
int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
qbus_walkerfn *busfn, void *opaque)
{
@@ -421,6 +428,13 @@ static void qbus_realize(BusState *bus, DeviceState
*parent, const char *name)
if (name) {
bus->name = g_strdup(name);
+ } else if (bus->parent && bus->parent->bus_name) {
+ /* parent device has bus_name -> use it for bus name */
This seems backwards to me. qdev had made sure to have a 1:n
relationship between device and bus, whereas you are making the name
template 1:1 here. Don't you rather want a qbus_set_name() mechanism
operating on the bus?
What do you mean with the relationship 1:n / 1:1 I don't understand?
Note: I did that quickly to ask Paolo if it was worth doing like that or
like the original
series.
+ len = strlen(bus->parent->bus_name) + 16;
Why 15 decimal digits? Is there any constant we could reuse?
+ buf = g_malloc(len);
+ snprintf(buf, len, "%s.%d", bus->parent->bus_name,
+ bus->parent->num_child_bus);
+ bus->name = buf;
} else if (bus->parent && bus->parent->id) {
/* parent device has id -> use it for bus name */
len = strlen(bus->parent->id) + 16;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 070df44..8d5d146 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1106,11 +1106,20 @@ static int
virtio_scsi_pci_init_pci(VirtIOPCIProxy *vpci_dev)
VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+ DeviceState *proxy = DEVICE(dev);
if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
vpci_dev->nvectors = vs->conf.num_queues + 3;
}
+ /*
+ * For command line compatibility, this set the virtio-scsi-device bus
"sets"
+ * name as before.
+ */
+ if (proxy->id) {
+ qdev_set_bus_name(vdev, proxy->id);
+ }
+
qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
if (qdev_init(vdev) < 0) {
return -1;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index cf83d54..332e11f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -125,6 +125,7 @@ struct DeviceState {
int num_child_bus;
int instance_id_alias;
int alias_required_for_version;
+ const char *bus_name;
};
#define TYPE_BUS "bus"
@@ -224,6 +225,9 @@ BusState *qdev_get_child_bus(DeviceState *dev, const
char *name);
void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
+/* Set the bus_name property. */
+void qdev_set_bus_name(DeviceState *dev, const char *bus_name);
+
BusState *qdev_get_parent_bus(DeviceState *dev);
/*** BUS API. ***/
Andreas
