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

[KONRAD Frédéric]

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