From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50072) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsdY-0002cO-Bx for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:17:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWsdX-000515-2g for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:17:16 -0400 Received: from greensocs.com ([87.106.252.221]:36178 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsdW-00050A-Mh for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:17:14 -0400 Message-ID: <517EB924.2070303@greensocs.com> Date: Mon, 29 Apr 2013 20:17:08 +0200 From: =?ISO-8859-15?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 References: <1367248365-27260-1-git-send-email-fred.konrad@greensocs.com> <1367248365-27260-5-git-send-email-fred.konrad@greensocs.com> <517E9552.5020303@redhat.com> <517E9F94.4010302@greensocs.com> <517EA0B7.4050503@redhat.com> <517EB04E.1060204@greensocs.com> <517EB42E.4060101@suse.de> In-Reply-To: <517EB42E.4060101@suse.de> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-1.5 4/4] virtio-scsi: fix the command line compatibility. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Andreas_F=E4rber?= Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, agraf@suse.de, amit.shah@redhat.com, cornelia.huck@de.ibm.com, Paolo Bonzini On 29/04/2013 19:55, Andreas F=E4rber wrote: > Am 29.04.2013 19:39, schrieb KONRAD Fr=E9d=E9ric: >> On 29/04/2013 18:32, Paolo Bonzini wrote: >>> Il 29/04/2013 18:28, KONRAD Fr=E9d=E9ric 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_real= ize? >>>> >>>> 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, DeviceS= tate >>>> *parent, const char *name) >>>> >>>> if (name) { >>>> bus->name =3D g_strdup(name); >>>> + } else if (bus->parent && bus->parent->bus_name) { >>>> + /* parent device has bus_name -> use it for bus name */ >>>> + len =3D strlen(bus->parent->bus_name) + 16; >>>> + buf =3D g_malloc(len); >>>> + snprintf(buf, len, "%s.%d", bus->parent->bus_name, >>>> + bus->parent->num_child_bus); >>>> + bus->name =3D buf; >>>> } else if (bus->parent && bus->parent->id) { >>>> /* parent device has id -> use it for bus name */ >>>> len =3D strlen(bus->parent->id) + 16; >>>> >>>> If so, change to scsi_bus_new is not needed and the two new function= s >>>> 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 =3D NULL; > } > >> + if (bus_name) { >> + dev->bus_name =3D 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, DeviceSta= te >> *parent, const char *name) >> >> if (name) { >> bus->name =3D 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=20 like the original series. > >> + len =3D strlen(bus->parent->bus_name) + 16; > Why 15 decimal digits? Is there any constant we could reuse? > >> + buf =3D g_malloc(len); >> + snprintf(buf, len, "%s.%d", bus->parent->bus_name, >> + bus->parent->num_child_bus); >> + bus->name =3D buf; >> } else if (bus->parent && bus->parent->id) { >> /* parent device has id -> use it for bus name */ >> len =3D 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 =3D VIRTIO_SCSI_PCI(vpci_dev); >> DeviceState *vdev =3D DEVICE(&dev->vdev); >> VirtIOSCSICommon *vs =3D VIRTIO_SCSI_COMMON(vdev); >> + DeviceState *proxy =3D DEVICE(dev); >> >> if (vpci_dev->nvectors =3D=3D DEV_NVECTORS_UNSPECIFIED) { >> vpci_dev->nvectors =3D vs->conf.num_queues + 3; >> } >> >> + /* >> + * For command line compatibility, this set the virtio-scsi-devic= e 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, con= st >> char *name); >> void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, i= nt 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 >