[PATCH RESEND 17/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFind()

[Laine Stump]

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
>   src/hypervisor/virhostdev.c | 12 ++++++++----
>   src/util/virpci.c           | 16 ++++++++--------
>   src/util/virpci.h           |  2 +-
>   3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
> index 402d7be42d..3bfb04c674 100644
> --- a/src/hypervisor/virhostdev.c
> +++ b/src/hypervisor/virhostdev.c
> @@ -657,7 +657,8 @@ virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr,
>   
>           /* We need to look up the actual device because that's what
>            * virPCIDeviceReattach() expects as its argument */
> -        if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
> +        if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs,
> +                                            virPCIDeviceGetAddress(pci))))
>               continue;
>   
>           if (virPCIDeviceGetManaged(actual)) {
> @@ -777,7 +778,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
>   
>               /* Unmanaged devices should already have been marked as
>                * inactive: if that's the case, we can simply move on */
> -            if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) {
> +            if (virPCIDeviceListFind(mgr->inactivePCIHostdevs,
> +                                     virPCIDeviceGetAddress(pci))) {

Wondered at first why you were using a access function to get the 
pointer rather than just referencing it directly. I'd forgotten that 
virPCIDevice is one of those rare "private structures" in libvirt - 
defined only in a single .c file.

Anyway,

Reviewed-by: Laine Stump <laine at redhat.com>


>                   VIR_DEBUG("Not detaching unmanaged PCI device %s",
>                             virPCIDeviceGetName(pci));
>                   continue;
> @@ -860,7 +862,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
>            * 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);
> +        actual = virPCIDeviceListFind(mgr->activePCIHostdevs,
> +                                      virPCIDeviceGetAddress(pci));
>   
>           VIR_DEBUG("Setting driver and domain information for PCI device %s",
>                     virPCIDeviceGetName(pci));
> @@ -992,7 +995,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr,
>            * 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);
> +        actual = virPCIDeviceListFind(mgr->activePCIHostdevs,
> +                                      virPCIDeviceGetAddress(pci));
>           if (actual) {
>               const char *actual_drvname;
>               const char *actual_domname;
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 1554acffb6..9544275c31 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -705,7 +705,7 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void
>           return 0;
>   
>       /* same bus, but inactive, i.e. about to be assigned to guest */
> -    if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, check))
> +    if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, &check->address))
>           return 0;
>   
>       return 1;
> @@ -1022,7 +1022,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
>           return -1;
>       }
>   
> -    if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
> +    if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("Not resetting active device %s"), dev->name);
>           return -1;
> @@ -1294,7 +1294,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
>       if (virPCIProbeStubDriver(dev->stubDriver) < 0)
>           return -1;
>   
> -    if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
> +    if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("Not detaching active device %s"), dev->name);
>           return -1;
> @@ -1306,7 +1306,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
>       /* Add *a copy of* the dev into list inactiveDevs, if
>        * it's not already there.
>        */
> -    if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) {
> +    if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, &dev->address)) {
>           VIR_DEBUG("Adding PCI device %s to inactive list", dev->name);
>           if (virPCIDeviceListAddCopy(inactiveDevs, dev) < 0)
>               return -1;
> @@ -1324,7 +1324,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
>                        virPCIDeviceListPtr activeDevs,
>                        virPCIDeviceListPtr inactiveDevs)
>   {
> -    if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
> +    if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("Not reattaching active device %s"), dev->name);
>           return -1;
> @@ -1684,7 +1684,7 @@ int
>   virPCIDeviceListAdd(virPCIDeviceListPtr list,
>                       virPCIDevicePtr dev)
>   {
> -    if (virPCIDeviceListFind(list, dev)) {
> +    if (virPCIDeviceListFind(list, &dev->address)) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("Device %s is already in use"), dev->name);
>           return -1;
> @@ -1795,11 +1795,11 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list,
>   
>   
>   virPCIDevicePtr
> -virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDevicePtr dev)
> +virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDeviceAddressPtr devAddr)
>   {
>       int idx;
>   
> -    if ((idx = virPCIDeviceListFindIndex(list, &dev->address)) >= 0)
> +    if ((idx = virPCIDeviceListFindIndex(list, devAddr)) >= 0)
>           return list->devs[idx];
>       else
>           return NULL;
> diff --git a/src/util/virpci.h b/src/util/virpci.h
> index 8c6776da21..628a293972 100644
> --- a/src/util/virpci.h
> +++ b/src/util/virpci.h
> @@ -167,7 +167,7 @@ virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
>   void virPCIDeviceListDel(virPCIDeviceListPtr list,
>                            virPCIDevicePtr dev);
>   virPCIDevicePtr virPCIDeviceListFind(virPCIDeviceListPtr list,
> -                                     virPCIDevicePtr dev);
> +                                     virPCIDeviceAddressPtr devAddr);
>   virPCIDevicePtr
>   virPCIDeviceListFindByIDs(virPCIDeviceListPtr list,
>                             unsigned int domain,
>