Re: [Qemu-devel] [PATCH for-1.5 4/4] virtio-scsi: fix the command line compatibility.

[KONRAD Frédéric]

On 29/04/2013 17:44, Paolo Bonzini wrote:
> Il 29/04/2013 17:12, address@hidden ha scritto:
> > From: KONRAD Frederic <address@hidden>
> >
> > The bus name is wrong since the refactoring.
> >
> > This keep the behaviour of the command line.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> > ---
> >   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?

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?

Fred
> 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 */