On Mon, Aug 17, 2009 at 03:10:20PM +0100, Mark McLoughlin wrote:
> The qemuPrepareHostDevices() and qemuDomainReAttachHostDevices()
> functions are clutter with a bunch of calls to pciGetDevice() and
> pciFreeDevice() obscuring the basic logic.
>
> Add a pciDeviceList type and add a qemuGetPciHostDeviceList() function
> to build a list from a domain definition. Use this in prepare/re-attach
> fto simplify things and eliminate the multiple pciGetDevice calls.
>
> This is especially useful because in the next patch we need to iterate
> the hostdevs list a third time and we also need a list type for keeping
> track of active devices.
>
> * src/pci.[ch]: add pciDeviceList type and also a per-device 'managed'
> property
>
> * src/libvirt_private.syms: export the new functions
>
> * src/qemu_driver.c: add qemuGetPciHostDeviceList() and re-write
> qemuPrepareHostDevices() and qemuDomainReAttachHostDevices() to use it
> ---
> src/libvirt_private.syms | 7 ++-
> src/pci.c | 109 ++++++++++++++++++++++++++++++
> src/pci.h | 20 ++++++
> src/qemu_driver.c | 167 +++++++++++++++++++--------------------------
> 4 files changed, 206 insertions(+), 97 deletions(-)
Yeah that's nicer readability in the QEMU driver.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 642c2bc..2bf4e15 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -285,7 +285,12 @@ pciFreeDevice;
> pciDettachDevice;
> pciReAttachDevice;
> pciResetDevice;
> -
> +pciDeviceSetManaged;
> +pciDeviceGetManaged;
> +pciDeviceListNew;
> +pciDeviceListFree;
> +pciDeviceListAdd;
> +pciDeviceListDel;
>
> # qparams.h
> qparam_get_query;
> diff --git a/src/pci.c b/src/pci.c
> index 74f7ef0..a37eaf7 100644
> --- a/src/pci.c
> +++ b/src/pci.c
> @@ -63,6 +63,7 @@ struct _pciDevice {
> unsigned pci_pm_cap_pos;
> unsigned has_flr : 1;
> unsigned has_pm_reset : 1;
> + unsigned managed : 1;
> };
>
> /* For virReportOOMError() and virReportSystemError() */
> @@ -890,8 +891,116 @@ pciGetDevice(virConnectPtr conn,
> void
> pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)
> {
> + if (!dev)
> + return;
> VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
> if (dev->fd >= 0)
> close(dev->fd);
> VIR_FREE(dev);
> }
> +
> +void pciDeviceSetManaged(pciDevice *dev, unsigned managed)
> +{
> + dev->managed = !!managed;
> +}
> +
> +unsigned pciDeviceGetManaged(pciDevice *dev)
> +{
> + return dev->managed;
> +}
> +
> +pciDeviceList *
> +pciDeviceListNew(virConnectPtr conn)
> +{
> + pciDeviceList *list;
> +
> + if (VIR_ALLOC(list) < 0) {
> + virReportOOMError(conn);
> + return NULL;
> + }
> +
> + return list;
> +}
> +
> +void
> +pciDeviceListFree(virConnectPtr conn,
> + pciDeviceList *list)
> +{
> + int i;
> +
> + if (!list)
> + return;
> +
> + for (i = 0; i < list->count; i++) {
> + pciFreeDevice(conn, list->devs[i]);
> + list->devs[i] = NULL;
> + }
> +
> + list->count = 0;
> + VIR_FREE(list->devs);
> + VIR_FREE(list);
> +}
> +
> +int
> +pciDeviceListAdd(virConnectPtr conn,
> + pciDeviceList *list,
> + pciDevice *dev)
> +{
> + if (pciDeviceListFind(list, dev)) {
> + pciReportError(conn, VIR_ERR_INTERNAL_ERROR,
> + _("Device %s is already in use"), dev->name);
> + return -1;
> + }
> +
> + if (VIR_REALLOC_N(list->devs, list->count+1) < 0) {
> + virReportOOMError(conn);
> + return -1;
> + }
> +
> + list->devs[list->count++] = dev;
> +
> + return 0;
> +}
> +
> +void
> +pciDeviceListDel(virConnectPtr conn ATTRIBUTE_UNUSED,
> + pciDeviceList *list,
> + pciDevice *dev)
> +{
> + int i;
> +
> + for (i = 0; i < list->count; i++) {
> + if (list->devs[i]->domain != dev->domain ||
> + list->devs[i]->bus != dev->bus ||
> + list->devs[i]->slot != dev->slot ||
> + list->devs[i]->function != dev->function)
> + continue;
> +
> + pciFreeDevice(conn, list->devs[i]);
> +
> + if (i != --list->count)
> + memmove(&list->devs[i],
> + &list->devs[i+1],
> + sizeof(*list->devs) * (list->count-i));
> +
> + if (VIR_REALLOC_N(list->devs, list->count) < 0) {
> + ; /* not fatal */
> + }
> +
> + break;
> + }
> +}
> +
> +pciDevice *
> +pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
> +{
> + int i;
> +
> + for (i = 0; i < list->count; i++)
> + if (list->devs[i]->domain == dev->domain &&
> + list->devs[i]->bus == dev->bus &&
> + list->devs[i]->slot == dev->slot &&
> + list->devs[i]->function == dev->function)
> + return list->devs[i];
> + return NULL;
> +}
> diff --git a/src/pci.h b/src/pci.h
> index 47882ef..75fbd51 100644
> --- a/src/pci.h
> +++ b/src/pci.h
> @@ -27,6 +27,11 @@
>
> typedef struct _pciDevice pciDevice;
>
> +typedef struct {
> + unsigned count;
> + pciDevice **devs;
> +} pciDeviceList;
> +
> pciDevice *pciGetDevice (virConnectPtr conn,
> unsigned domain,
> unsigned bus,
> @@ -40,5 +45,20 @@ int pciReAttachDevice (virConnectPtr conn,
> pciDevice *dev);
> int pciResetDevice (virConnectPtr conn,
> pciDevice *dev);
> +void pciDeviceSetManaged(pciDevice *dev,
> + unsigned managed);
> +unsigned pciDeviceGetManaged(pciDevice *dev);
> +
> +pciDeviceList *pciDeviceListNew (virConnectPtr conn);
> +void pciDeviceListFree (virConnectPtr conn,
> + pciDeviceList *list);
> +int pciDeviceListAdd (virConnectPtr conn,
> + pciDeviceList *list,
> + pciDevice *dev);
> +void pciDeviceListDel (virConnectPtr conn,
> + pciDeviceList *list,
> + pciDevice *dev);
> +pciDevice * pciDeviceListFind (pciDeviceList *list,
> + pciDevice *dev);
>
> #endif /* __VIR_PCI_H__ */
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index a9da387..f2b807b 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -1329,48 +1329,16 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
> return -1;
> }
>
> -static int qemuPrepareHostDevices(virConnectPtr conn,
> - virDomainDefPtr def) {
> +static pciDeviceList *
> +qemuGetPciHostDeviceList(virConnectPtr conn,
> + virDomainDefPtr def)
> +{
> + pciDeviceList *list;
> int i;
>
> - /* We have to use 2 loops here. *All* devices must
> - * be detached before we reset any of them, because
> - * in some cases you have to reset the whole PCI,
> - * which impacts all devices on it
> - */
> -
> - for (i = 0 ; i < def->nhostdevs ; i++) {
> - virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> -
> - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> - continue;
> - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> - continue;
> -
> - if (hostdev->managed) {
> - pciDevice *dev = pciGetDevice(conn,
> - hostdev->source.subsys.u.pci.domain,
> - hostdev->source.subsys.u.pci.bus,
> - hostdev->source.subsys.u.pci.slot,
> - hostdev->source.subsys.u.pci.function);
> - if (!dev)
> - goto error;
> -
> - if (pciDettachDevice(conn, dev) < 0) {
> - pciFreeDevice(conn, dev);
> - goto error;
> - }
> -
> - pciFreeDevice(conn, dev);
> - } /* else {
> - XXX validate that non-managed device isn't in use, eg
> - by checking that device is either un-bound, or bound
> - to pci-stub.ko
> - } */
> - }
> + if (!(list = pciDeviceListNew(conn)))
> + return NULL;
>
> - /* Now that all the PCI hostdevs have be dettached, we can safely
> - * reset them */
> for (i = 0 ; i < def->nhostdevs ; i++) {
> virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> pciDevice *dev;
> @@ -1385,95 +1353,102 @@ static int qemuPrepareHostDevices(virConnectPtr conn,
> hostdev->source.subsys.u.pci.bus,
> hostdev->source.subsys.u.pci.slot,
> hostdev->source.subsys.u.pci.function);
> - if (!dev)
> - goto error;
> + if (!dev) {
> + pciDeviceListFree(conn, list);
> + return NULL;
> + }
>
> - if (pciResetDevice(conn, dev) < 0) {
> + if (pciDeviceListAdd(conn, list, dev) < 0) {
> pciFreeDevice(conn, dev);
> - goto error;
> + pciDeviceListFree(conn, list);
> + return NULL;
> }
>
> - pciFreeDevice(conn, dev);
> + pciDeviceSetManaged(dev, hostdev->managed);
> }
>
> + return list;
> +}
> +
> +static int
> +qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def)
> +{
> + pciDeviceList *pcidevs;
> + int i;
> +
> + if (!def->nhostdevs)
> + return 0;
> +
> + if (!(pcidevs = qemuGetPciHostDeviceList(conn, def)))
> + return -1;
> +
> + /* We have to use 2 loops here. *All* devices must
> + * be detached before we reset any of them, because
> + * in some cases you have to reset the whole PCI,
> + * which impacts all devices on it
> + */
> +
> + /* XXX validate that non-managed device isn't in use, eg
> + * by checking that device is either un-bound, or bound
> + * to pci-stub.ko
> + */
> +
> + for (i = 0; i < pcidevs->count; i++)
> + if (pciDeviceGetManaged(pcidevs->devs[i]) &&
> + pciDettachDevice(conn, pcidevs->devs[i]) < 0)
> + goto error;
> +
> + /* Now that all the PCI hostdevs have be dettached, we can safely
> + * reset them */
> + for (i = 0; i < pcidevs->count; i++)
> + if (pciResetDevice(conn, pcidevs->devs[i]) < 0)
> + goto error;
> +
> + pciDeviceListFree(conn, pcidevs);
> return 0;
>
> error:
> + pciDeviceListFree(conn, pcidevs);
> return -1;
> }
>
> static void
> qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def)
> {
> + pciDeviceList *pcidevs;
> int i;
>
> - /* Again 2 loops; reset all the devices before re-attach */
> -
> - for (i = 0 ; i < def->nhostdevs ; i++) {
> - virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> - pciDevice *dev;
> -
> - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> - continue;
> - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> - continue;
> -
> - dev = pciGetDevice(conn,
> - hostdev->source.subsys.u.pci.domain,
> - hostdev->source.subsys.u.pci.bus,
> - hostdev->source.subsys.u.pci.slot,
> - hostdev->source.subsys.u.pci.function);
> - if (!dev) {
> - virErrorPtr err = virGetLastError();
> - VIR_ERROR(_("Failed to allocate pciDevice: %s\n"),
> - err ? err->message : "");
> - virResetError(err);
> - continue;
> - }
> -
> - if (pciResetDevice(conn, dev) < 0) {
> - virErrorPtr err = virGetLastError();
> - VIR_ERROR(_("Failed to reset PCI device: %s\n"),
> - err ? err->message : "");
> - virResetError(err);
> - }
> + if (!def->nhostdevs)
> + return;
>
> - pciFreeDevice(conn, dev);
> + if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) {
> + virErrorPtr err = virGetLastError();
> + VIR_ERROR(_("Failed to allocate pciDeviceList: %s\n"),
> + err ? err->message : "");
> + virResetError(err);
> + return;
> }
>
> - for (i = 0 ; i < def->nhostdevs ; i++) {
> - virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> - pciDevice *dev;
> -
> - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> - continue;
> - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> - continue;
> - if (!hostdev->managed)
> - continue;
> + /* Again 2 loops; reset all the devices before re-attach */
>
> - dev = pciGetDevice(conn,
> - hostdev->source.subsys.u.pci.domain,
> - hostdev->source.subsys.u.pci.bus,
> - hostdev->source.subsys.u.pci.slot,
> - hostdev->source.subsys.u.pci.function);
> - if (!dev) {
> + for (i = 0; i < pcidevs->count; i++)
> + if (pciResetDevice(conn, pcidevs->devs[i]) < 0) {
> virErrorPtr err = virGetLastError();
> - VIR_ERROR(_("Failed to allocate pciDevice: %s\n"),
> + VIR_ERROR(_("Failed to reset PCI device: %s\n"),
> err ? err->message : "");
> virResetError(err);
> - continue;
> }
>
> - if (pciReAttachDevice(conn, dev) < 0) {
> + for (i = 0; i < pcidevs->count; i++)
> + if (pciDeviceGetManaged(pcidevs->devs[i]) &&
> + pciReAttachDevice(conn, pcidevs->devs[i]) < 0) {
> virErrorPtr err = virGetLastError();
> VIR_ERROR(_("Failed to re-attach PCI device: %s\n"),
> err ? err->message : "");
> virResetError(err);
> }
>
> - pciFreeDevice(conn, dev);
> - }
> + pciDeviceListFree(conn, pcidevs);
> }
>
> static const char *const defaultDeviceACL[] = {
> --
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list