Message ID | 1367248365-27260-5-git-send-email-fred.konrad@greensocs.com |
---|---|
State | New |
Headers | show |
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 */ >
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 */ >>
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
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?
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 --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 */