Patchwork [for-1.5,4/4] virtio-scsi: fix the command line compatibility.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date April 29, 2013, 5:39 p.m.
Message ID <517EB04E.1060204@greensocs.com>
Download mbox | patch
Permalink /patch/240425/
State New
Headers show

Comments

fred.konrad@greensocs.com - April 29, 2013, 5:39 p.m.
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(+)

  /*** BUS API. ***/
Andreas Färber - April 29, 2013, 5:55 p.m.
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?

> +        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
fred.konrad@greensocs.com - April 29, 2013, 6:17 p.m.
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
>

Patch

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)
+{
+    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 */
+        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;
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
+     * 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);