diff mbox

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

Message ID 1367248365-27260-5-git-send-email-fred.konrad@greensocs.com
State New
Headers show

Commit Message

fred.konrad@greensocs.com April 29, 2013, 3:12 p.m. UTC 
From: KONRAD Frederic <fred.konrad@greensocs.com>

The bus name is wrong since the refactoring.

This keep the behaviour of the command line.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/s390x/s390-virtio-bus.c      |  9 +++++++++
 hw/s390x/virtio-ccw.c           |  9 +++++++++
 hw/scsi/virtio-scsi.c           | 14 +++++++++++++-
 hw/virtio/virtio-pci.c          |  9 +++++++++
 include/hw/virtio/virtio-scsi.h |  7 +++++++
 5 files changed, 47 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini April 29, 2013, 3:44 p.m. UTC | #1 
Il 29/04/2013 17:12, fred.konrad@greensocs.com ha scritto:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
> 
> The bus name is wrong since the refactoring.
> 
> This keep the behaviour of the command line.
> 
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/s390x/s390-virtio-bus.c      |  9 +++++++++
>  hw/s390x/virtio-ccw.c           |  9 +++++++++
>  hw/scsi/virtio-scsi.c           | 14 +++++++++++++-
>  hw/virtio/virtio-pci.c          |  9 +++++++++
>  include/hw/virtio/virtio-scsi.h |  7 +++++++
>  5 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 6620d29..e1fd937 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -232,6 +232,15 @@ static int s390_virtio_scsi_init(VirtIOS390Device *s390_dev)
>  {
>      VirtIOSCSIS390 *dev = VIRTIO_SCSI_S390(s390_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> +    DeviceState *qdev = DEVICE(s390_dev);
> +
> +    /*
> +     * For command line compatibility, this set the virtio-scsi-device bus
> +     * name as before.
> +     */
> +    if (qdev->id) {
> +        set_virtio_scsi_bus_name(vdev, qdev->id);
> +    }

Could this be simply a qdev property?

Consider that qdev will strdup any bus name you pass, so it is perfectly
ok to do:

   bus_name = g_strdup_printf("%s.0", vs->bus_name);
   scsi_bus_new(..., bus_name);
   g_free(bus_name);

> +void set_virtio_scsi_bus_name(DeviceState *dev, const char *bus_name)
> +{
> +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> +    if (bus_name) {
> +        vs->bus_name = g_malloc(strlen(bus_name) + 3);
> +        snprintf(vs->bus_name, strlen(bus_name) + 3, "%s.0", bus_name);

This would use g_strdup_printf, as above.

Paolo

> +    }
> +}
> +
>  int virtio_scsi_common_init(VirtIOSCSICommon *s)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -624,7 +633,7 @@ static int virtio_scsi_device_init(VirtIODevice *vdev)
>          return ret;
>      }
>  
> -    scsi_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info);
> +    scsi_named_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info, vs->bus_name);
>      if (!qdev->hotplugged) {
>          scsi_bus_legacy_handle_cmdline(&s->bus);
>      }
> @@ -639,6 +648,9 @@ int virtio_scsi_common_exit(VirtIOSCSICommon *vs)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(vs);
>  
> +    if (vs->bus_name) {
> +        g_free(vs->bus_name);
> +    }
>      g_free(vs->cmd_vqs);
>      virtio_cleanup(vdev);
>      return 0;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 598876f..14fb8dd 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(vpci_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) {
> +        set_virtio_scsi_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/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 4db346b..c356d54 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -164,6 +164,9 @@ typedef struct VirtIOSCSICommon {
>      VirtQueue *ctrl_vq;
>      VirtQueue *event_vq;
>      VirtQueue **cmd_vqs;
> +
> +    /* bus_name, for command line compatibility */
> +    char *bus_name;
>  } VirtIOSCSICommon;
>  
>  typedef struct {
> @@ -189,5 +192,9 @@ typedef struct {
>  int virtio_scsi_common_init(VirtIOSCSICommon *vs);
>  int virtio_scsi_common_exit(VirtIOSCSICommon *vs);
>  
> +/*
> + * For command line back compatibility, set the bus name before initialisation.
> + */
> +void set_virtio_scsi_bus_name(DeviceState *dev, const char *bus_name);
>  
>  #endif /* _QEMU_VIRTIO_SCSI_H */
>
fred.konrad@greensocs.com April 30, 2013, 1:06 p.m. UTC | #2 
On 29/04/2013 17:44, Paolo Bonzini wrote:
> Il 29/04/2013 17:12, fred.konrad@greensocs.com ha scritto:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> The bus name is wrong since the refactoring.
>>
>> This keep the behaviour of the command line.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   hw/s390x/s390-virtio-bus.c      |  9 +++++++++
>>   hw/s390x/virtio-ccw.c           |  9 +++++++++
>>   hw/scsi/virtio-scsi.c           | 14 +++++++++++++-
>>   hw/virtio/virtio-pci.c          |  9 +++++++++
>>   include/hw/virtio/virtio-scsi.h |  7 +++++++
>>   5 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index 6620d29..e1fd937 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -232,6 +232,15 @@ static int s390_virtio_scsi_init(VirtIOS390Device *s390_dev)
>>   {
>>       VirtIOSCSIS390 *dev = VIRTIO_SCSI_S390(s390_dev);
>>       DeviceState *vdev = DEVICE(&dev->vdev);
>> +    DeviceState *qdev = DEVICE(s390_dev);
>> +
>> +    /*
>> +     * For command line compatibility, this set the virtio-scsi-device bus
>> +     * name as before.
>> +     */
>> +    if (qdev->id) {
>> +        set_virtio_scsi_bus_name(vdev, qdev->id);
>> +    }
> Could this be simply a qdev property?

Well, maybe my solution is too extreme.

Maybe I can put bus_name in struct VirtIODevice instead to avoid the
set_virtio_x_bus_name code duplication?
>
> Consider that qdev will strdup any bus name you pass, so it is perfectly
> ok to do:
>
>     bus_name = g_strdup_printf("%s.0", vs->bus_name);
>     scsi_bus_new(..., bus_name);
>     g_free(bus_name);
>
>> +void set_virtio_scsi_bus_name(DeviceState *dev, const char *bus_name)
>> +{
>> +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>> +    if (bus_name) {
>> +        vs->bus_name = g_malloc(strlen(bus_name) + 3);
>> +        snprintf(vs->bus_name, strlen(bus_name) + 3, "%s.0", bus_name);
> This would use g_strdup_printf, as above.
>
> Paolo
>
>> +    }
>> +}
>> +
>>   int virtio_scsi_common_init(VirtIOSCSICommon *s)
>>   {
>>       VirtIODevice *vdev = VIRTIO_DEVICE(s);
>> @@ -624,7 +633,7 @@ static int virtio_scsi_device_init(VirtIODevice *vdev)
>>           return ret;
>>       }
>>   
>> -    scsi_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info);
>> +    scsi_named_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info, vs->bus_name);
>>       if (!qdev->hotplugged) {
>>           scsi_bus_legacy_handle_cmdline(&s->bus);
>>       }
>> @@ -639,6 +648,9 @@ int virtio_scsi_common_exit(VirtIOSCSICommon *vs)
>>   {
>>       VirtIODevice *vdev = VIRTIO_DEVICE(vs);
>>   
>> +    if (vs->bus_name) {
>> +        g_free(vs->bus_name);
>> +    }
>>       g_free(vs->cmd_vqs);
>>       virtio_cleanup(vdev);
>>       return 0;
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 598876f..14fb8dd 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(vpci_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) {
>> +        set_virtio_scsi_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/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
>> index 4db346b..c356d54 100644
>> --- a/include/hw/virtio/virtio-scsi.h
>> +++ b/include/hw/virtio/virtio-scsi.h
>> @@ -164,6 +164,9 @@ typedef struct VirtIOSCSICommon {
>>       VirtQueue *ctrl_vq;
>>       VirtQueue *event_vq;
>>       VirtQueue **cmd_vqs;
>> +
>> +    /* bus_name, for command line compatibility */
>> +    char *bus_name;
>>   } VirtIOSCSICommon;
>>   
>>   typedef struct {
>> @@ -189,5 +192,9 @@ typedef struct {
>>   int virtio_scsi_common_init(VirtIOSCSICommon *vs);
>>   int virtio_scsi_common_exit(VirtIOSCSICommon *vs);
>>   
>> +/*
>> + * For command line back compatibility, set the bus name before initialisation.
>> + */
>> +void set_virtio_scsi_bus_name(DeviceState *dev, const char *bus_name);
>>   
>>   #endif /* _QEMU_VIRTIO_SCSI_H */
>>
Paolo Bonzini April 30, 2013, 2:01 p.m. UTC | #3 
Il 30/04/2013 15:06, KONRAD Frédéric ha scritto:
>>>
>> Could this be simply a qdev property?
> 
> Well, maybe my solution is too extreme.
> 
> Maybe I can put bus_name in struct VirtIODevice instead to avoid the
> set_virtio_x_bus_name code duplication?

No, I think it should be either DeviceState or single devices.

Paolo
fred.konrad@greensocs.com April 30, 2013, 2:15 p.m. UTC | #4 
On 30/04/2013 16:01, Paolo Bonzini wrote:
> Il 30/04/2013 15:06, KONRAD Frédéric ha scritto:
>>> Could this be simply a qdev property?
>> Well, maybe my solution is too extreme.
>>
>> Maybe I can put bus_name in struct VirtIODevice instead to avoid the
>> set_virtio_x_bus_name code duplication?
> No, I think it should be either DeviceState or single devices.
>
> Paolo
>
oops, just sent the fix with bus_name in VirtIODevice..

I think it's not necessary to put bus_name in DeviceState as it is only 
needed for these two devices.

But it may be better for it to be in VirtIODevice no?
Paolo Bonzini April 30, 2013, 2:17 p.m. UTC | #5 
Il 30/04/2013 16:15, KONRAD Frédéric ha scritto:
> On 30/04/2013 16:01, Paolo Bonzini wrote:
>> Il 30/04/2013 15:06, KONRAD Frédéric ha scritto:
>>>> Could this be simply a qdev property?
>>> Well, maybe my solution is too extreme.
>>>
>>> Maybe I can put bus_name in struct VirtIODevice instead to avoid the
>>> set_virtio_x_bus_name code duplication?
>> No, I think it should be either DeviceState or single devices.
>>
>> Paolo
>>
> oops, just sent the fix with bus_name in VirtIODevice..
> 
> I think it's not necessary to put bus_name in DeviceState as it is only
> needed for these two devices.
> 
> But it may be better for it to be in VirtIODevice no?

Yeah, in the end it doesn't look bad... I should let other people who
followed the refactoring more closely decide.

Paolo
diff mbox

Patch

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 6620d29..e1fd937 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -232,6 +232,15 @@  static int s390_virtio_scsi_init(VirtIOS390Device *s390_dev)
 {
     VirtIOSCSIS390 *dev = VIRTIO_SCSI_S390(s390_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    DeviceState *qdev = DEVICE(s390_dev);
+
+    /*
+     * For command line compatibility, this set the virtio-scsi-device bus
+     * name as before.
+     */
+    if (qdev->id) {
+        set_virtio_scsi_bus_name(vdev, qdev->id);
+    }
 
     qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
     if (qdev_init(vdev) < 0) {
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index daa94d6..9d69df5 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -677,6 +677,15 @@  static int virtio_ccw_scsi_init(VirtioCcwDevice *ccw_dev)
 {
     VirtIOSCSICcw *dev = VIRTIO_SCSI_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    DeviceState *qdev = DEVICE(ccw_dev);
+
+    /*
+     * For command line compatibility, this set the virtio-scsi-device bus
+     * name as before.
+     */
+    if (qdev->id) {
+        set_virtio_scsi_bus_name(vdev, qdev->id);
+    }
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     if (qdev_init(vdev) < 0) {
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 84b3ac7..fc27131 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -587,6 +587,15 @@  static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .load_request = virtio_scsi_load_request,
 };
 
+void set_virtio_scsi_bus_name(DeviceState *dev, const char *bus_name)
+{
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+    if (bus_name) {
+        vs->bus_name = g_malloc(strlen(bus_name) + 3);
+        snprintf(vs->bus_name, strlen(bus_name) + 3, "%s.0", bus_name);
+    }
+}
+
 int virtio_scsi_common_init(VirtIOSCSICommon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -624,7 +633,7 @@  static int virtio_scsi_device_init(VirtIODevice *vdev)
         return ret;
     }
 
-    scsi_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info);
+    scsi_named_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info, vs->bus_name);
     if (!qdev->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus);
     }
@@ -639,6 +648,9 @@  int virtio_scsi_common_exit(VirtIOSCSICommon *vs)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(vs);
 
+    if (vs->bus_name) {
+        g_free(vs->bus_name);
+    }
     g_free(vs->cmd_vqs);
     virtio_cleanup(vdev);
     return 0;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 598876f..14fb8dd 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(vpci_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) {
+        set_virtio_scsi_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/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 4db346b..c356d54 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -164,6 +164,9 @@  typedef struct VirtIOSCSICommon {
     VirtQueue *ctrl_vq;
     VirtQueue *event_vq;
     VirtQueue **cmd_vqs;
+
+    /* bus_name, for command line compatibility */
+    char *bus_name;
 } VirtIOSCSICommon;
 
 typedef struct {
@@ -189,5 +192,9 @@  typedef struct {
 int virtio_scsi_common_init(VirtIOSCSICommon *vs);
 int virtio_scsi_common_exit(VirtIOSCSICommon *vs);
 
+/*
+ * For command line back compatibility, set the bus name before initialisation.
+ */
+void set_virtio_scsi_bus_name(DeviceState *dev, const char *bus_name);
 
 #endif /* _QEMU_VIRTIO_SCSI_H */