From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsJ0-00035k-Aq for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:56:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWsIy-0006DX-Lz for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:56:02 -0400 Received: from cantor2.suse.de ([195.135.220.15]:58143 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsIy-0006DO-CM for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:56:00 -0400 Message-ID: <517EB42E.4060101@suse.de> Date: Mon, 29 Apr 2013 19:55:58 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= 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> In-Reply-To: <517EB04E.1060204@greensocs.com> Content-Type: text/plain; charset=ISO-8859-15 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?KONRAD_Fr=E9d=E9ric?= 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 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_reali= ze? >>> >>> 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, DeviceSt= ate >>> *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 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: >=20 > --- > hw/core/qdev.c | 14 ++++++++++++++ > hw/virtio/virtio-pci.c | 9 +++++++++ > include/hw/qdev-core.h | 4 ++++ > 3 files changed, 27 insertions(+) >=20 > 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; > } >=20 > +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, DeviceStat= e > *parent, const char *name) >=20 > 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? > + 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); >=20 > if (vpci_dev->nvectors =3D=3D DEV_NVECTORS_UNSPECIFIED) { > vpci_dev->nvectors =3D vs->conf.num_queues + 3; > } >=20 > + /* > + * 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; > }; >=20 > #define TYPE_BUS "bus" > @@ -224,6 +225,9 @@ BusState *qdev_get_child_bus(DeviceState *dev, cons= t > 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); >=20 > +/* Set the bus_name property. */ > +void qdev_set_bus_name(DeviceState *dev, const char *bus_name); > + > BusState *qdev_get_parent_bus(DeviceState *dev); >=20 > /*** BUS API. ***/ Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg