[libvirt] [PATCH] hostdev: fix two bugs in virHostdevReAttachPCIDevices

[John Ferlan]

On 03/22/2015 09:59 AM, Huanle Han wrote:
> 
> Bug 1: The the next element in the pcidevs is skipped after we
> virPCIDeviceListDel the previous element.
> 
> Bug 2: virHostdevNetConfigRestore is called for store the hostdevs
> which may be used by other domain.
> 
> Signed-off-by: Huanle Han <hanxueluo at gmail.com <mailto:hanxueluo at gmail.com>>
> ---
>  src/util/virhostdev.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
Should this be two separate patches? IOW: Are you fixing two separate
issues or essentially the same issue? Makes it easier to cherry-pick and
choose what may need to be backported elsewhere...

If there's a bug associated that would have been "good" to list as well
as perhaps an example or two...  That is - what XML and command(s)
sequence(s) prompted you to write the patch(es)...

Also, my attempt to git am -3 your patch fails - perhaps it's your
mailer configuration.  Did you use git send-email?  or something else...
I'm seeing html format... So I'm only reviewing based on what I read.

> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 9678e2b..ecbe5d8 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -785,7 +785,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> hostdev_mgr,
>       * them and reset all the devices before re-attach.
>       * Attach mac and port profile parameters to devices
>       */

^^^
I think the comment here could be updated to make it clear what's being
done. To just say "Again 4 loops;..." is a bit misleading since the only
loops I found in the code were in virHostdevPreparePCIDevices where
there are (currently) 9 such loops!

> -    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> +    for (i = 0; i < virPCIDeviceListCount(pcidevs);) {
>          virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>          virPCIDevicePtr activeDev = NULL;
>  
> @@ -806,6 +806,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> hostdev_mgr,
>          }
>  
>          virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev);
> +        i++;
>      }


Not sure I'm seeing why your fix works... Are you claiming that the
remove from other domain check (e.g. where the code "continue;"'s)
shouldn't be updating the 'i' value?  All it seems you did was move the
i++ from the for line to after the second virPCIDeviceListDel in the
block... Which yes, does reduce the count of devices and I would think
should be the case that doesn't increment i...

If we have 4 devices [0, 1, 2, 3] and we remove [1], then there are 3
devices left and i would be 2, but the new array would be [0, 2, 3] and
thus we don't check [2].

Perhaps this would work/read better as a while loop.

>  
>      /* At this point, any device that had been used by the guest is in
> @@ -816,9 +817,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> hostdev_mgr,
>       * For SRIOV net host devices, unset mac and port profile before
>       * reset and reattach device
>       */
> -    for (i = 0; i < nhostdevs; i++)
> -        virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir,
> -                                   oldStateDir);
> +    for (i = 0; i < nhostdevs; i++) {
> +        virPCIDevicePtr dev;
> +        virDomainHostdevDefPtr hostdev = hostdevs[i];
> +        virDomainHostdevSubsysPCIPtr pcisrc =
> &hostdev->source.subsys.u.pci;
> +
> +        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> +            hostdev->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI ||
> +            hostdev->parent.type != VIR_DOMAIN_DEVICE_NET ||
> +            !hostdev->parent.data.net <http://parent.data.net>)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^
This is what makes me believe you sent in html format

It's also directly taken from virHostdevNetConfigRestore... and could be
considered partially from virHostdevPreparePCIDevices

Suggestion - create a patch that has a new static bool function
("isHostdevPCINetDevice") that does the same check passing "hostdev"...
Then adjust the (currently) two places that make that check in order to
use the bool function to decide to continue or not. Then this patch
would use that function here instead.

> +                continue;
> +
> +        dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
> +                              pcisrc->addr.slot, pcisrc->addr.function);
> +        if (virPCIDeviceListFind(pcidevs, dev)) {
> +            virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir,
> +                                       oldStateDir);
> +        }
> +        virPCIDeviceFree(dev);
> +   }

It's still not quite clear to me from your description why we have to
search the pcidevs for our device before calling the Restore function.
Does it matter if it's in or not in the activePCIHostdevs list (the
comment just before this section).  I think this is one of those cases
where the code perhaps isn't self documenting and that by adding a few
more comments along the way we can better describe what we're trying to
do and why it has to be done.


John
>  
>      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>          virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> -- 
> 2.1.0
> 
> 
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>