On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> These comments explain the difference between a virPCIDevice
> instance used for lookups and an actual device instance; some
> information is also provided for specific uses.
> ---
> src/util/virhostdev.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 10d1c1a..a431f0a 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -60,6 +60,27 @@ struct virHostdevIsPCINodeDeviceUsedData {
> const bool usesVFIO;
> };
>
> +/* This module makes heavy use of bookkeeping lists, contained inside a
^
no comma.
> + * virHostdevManager instance, to keep track of the devices' status. To make
^
same
> + * it easy to spot potential ownership errors when moving devices from one
> + * list to the other, variable names should comply with the following
> + * conventions when it comes to virPCIDevice and virPCIDeviceList instances:
> + *
> + * pci - a short-lived virPCIDevice whose purpose is usually just to look
> + * up the actual PCI device in one of the bookkeeping lists; basically
> + * little more than a fancy virPCIDeviceAddress
> + *
> + * pcidevs - a list containing a bunch of the above
> + *
> + * actual - a virPCIDevice instance that has either been retrieved from one
> + * of the bookkeeping lists, or is intended to be added or copied
> + * to one at some point
> + *
> + * Passing an 'actual' to a function that requires a 'pci' is fine, but the
> + * opposite is usually not true; as a rule of thumb, functions in the virpci
> + * module usually expect an 'actual'. Even with these conventions in place,
> + * adding comments to highlight ownership-related issues is recommended */
> +
s//Free beer for anyone that reads this and adheres to it. You will need
it./
> static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque)
> {
> virPCIDevicePtr actual;
> @@ -544,6 +565,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>
> if (virPCIDeviceGetManaged(pci)) {
Just for consistency/readability - add an extra line between the if and
comment open.
ACK
John
> + /* We can't look up the actual device because it has not been
> + * created yet: virPCIDeviceDetach() will insert a copy of 'pci'
> + * into the list of inactive devices, and that copy will be the
> + * actual device going forward */
> VIR_DEBUG("Detaching managed PCI device %s",
> virPCIDeviceGetName(pci));
> if (virPCIDeviceDetach(pci,
> @@ -564,6 +589,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>
> + /* We can avoid looking up the actual device here, because performing
> + * a PCI reset on a device doesn't require any information other than
> + * the address, which 'pci' already contains */
> VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
> if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
> mgr->inactivePCIHostdevs) < 0)
> @@ -608,6 +636,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr pci, actual;
>
> + /* We need to look up the actual device and set the information
> + * there because 'pci' only contain address information and will
> + * be released at the end of the function */
> pci = virPCIDeviceListGet(pcidevs, i);
> actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
>
> @@ -775,6 +806,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> virPCIDevicePtr actual = NULL;
>
> + /* We need to look up the actual device, which is the one containing
> + * information such as by which domain and driver it is used. As a
> + * side effect, by looking it up we can also tell whether it was
> + * really active in the first place */
> actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
> if (actual) {
> const char *actual_drvname;
> @@ -830,6 +865,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>
> + /* We can avoid looking up the actual device here, because performing
> + * a PCI reset on a device doesn't require any information other than
> + * the address, which 'pci' already contains */
> VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
> if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
> mgr->inactivePCIHostdevs) < 0) {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list