[libvirt] [PATCH] hostdev: fix net config restore error

[John Ferlan]

On 04/15/2015 01:29 PM, Huanle Han wrote:
> Fix for such a case:
> 1. Domain A and B xml contain the same SRIOV net hostdev(<interface
> type='hostdev' /> with same pci address).
> 2. virsh start A (Successfully, and configure the SRIOV net with
> custom mac)
> 3. virsh start B (Fail because of the hostdev used by domain A or other
> reason.)
> In step 3, 'virHostdevNetConfigRestore' is called for the hostdev
> which is still used by domain A. It makes the mac/vlan of the SRIOV net
> change.
> 
> Code Change in this fix:
> 1. As the pci used by other domain have been removed from
> 'pcidevs' in previous loop, we only restore the nic config for
> the hostdev still in 'pcidevs'(used by this domain)
> 2. wrap a function 'virHostdevIsPCINetDevice', which detect whether the
> hostdev is a pci net device or not.
> 3. update the comments to make it more clear
> 
> Signed-off-by: Huanle Han <hanxueluo at gmail.com>
> ---
>  src/util/virhostdev.c | 70 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 47 insertions(+), 23 deletions(-)
> 

This seems reasonable - although it should have been two patches - the
first just to create virHostdevIsPCINetDevice and the second to fix the
issue and update the comments...  Something I can take care of for you
before pushing...

Hopefully Laine can also take a quick look at it and verify the actions
since he's most familiar with the SRIOV code path...

John
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index f583e54..a2719d3 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -350,6 +350,14 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, char **linkdev,
>      return ret;
>  }
>  
> +static int
> +virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev)
> +{
> +    return 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;
> +}
>  
>  static int
>  virHostdevNetConfigVirtPortProfile(const char *linkdev, int vf,
> @@ -481,10 +489,7 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev,
>      /* This is only needed for PCI devices that have been defined
>       * using <interface type='hostdev'>. For all others, it is a NOP.
>       */
> -    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)
> +    if (!virHostdevIsPCINetDevice(hostdev))
>         return 0;
>  
>      isvf = virHostdevIsVirtualFunction(hostdev);
> @@ -604,16 +609,11 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
>       * the network device, set the netdev config */
>      for (i = 0; i < nhostdevs; i++) {
>           virDomainHostdevDefPtr hostdev = hostdevs[i];
> -         if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> +         if (!virHostdevIsPCINetDevice(hostdev))
>               continue;
> -         if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> -             continue;
> -         if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET &&
> -             hostdev->parent.data.net) {
> -             if (virHostdevNetConfigReplace(hostdev, uuid,
> -                                            hostdev_mgr->stateDir) < 0) {
> -                 goto resetvfnetconfig;
> -             }
> +         if (virHostdevNetConfigReplace(hostdev, uuid,
> +                                        hostdev_mgr->stateDir) < 0) {
> +             goto resetvfnetconfig;
>           }
>           last_processed_hostdev_vf = i;
>      }
> @@ -781,19 +781,20 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
>          goto cleanup;
>      }
>  
> -    /* Again 4 loops; mark all devices as inactive before reset
> +    /* Here are 4 loops; mark all devices as inactive before reset
>       * them and reset all the devices before re-attach.
>       * Attach mac and port profile parameters to devices
>       */
> +
> +    /* Loop 1: delete the copy of the dev from pcidevs if it's used by
> +     * other domain. Or delete it from activePCIHostDevs if it had
> +     * been used by this domain.
> +     */
>      i = 0;
>      while (i < virPCIDeviceListCount(pcidevs)) {
>          virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>          virPCIDevicePtr activeDev = NULL;
>  
> -        /* delete the copy of the dev from pcidevs if it's used by
> -         * other domain. Or delete it from activePCIHostDevs if it had
> -         * been used by this domain.
> -         */
>          activeDev = virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev);
>          if (activeDev) {
>              const char *usedby_drvname;
> @@ -815,13 +816,33 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
>       */
>  
>      /*
> -     * For SRIOV net host devices, unset mac and port profile before
> -     * reset and reattach device
> +     * Loop 2: For SRIOV net host devices used by this domain, unset mac and port profile before
> +     * resetting and reattaching device
>       */
> -    for (i = 0; i < nhostdevs; i++)
> -        virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir,
> -                                   oldStateDir);
> +    for (i = 0; i < nhostdevs; i++) {
> +        virDomainHostdevDefPtr hostdev = hostdevs[i];
>  
> +        if (virHostdevIsPCINetDevice(hostdev)) {
> +            virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
> +            virPCIDevicePtr dev = NULL;
> +            dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
> +                                  pcisrc->addr.slot, pcisrc->addr.function);
> +            if (dev) {
> +                if (virPCIDeviceListFind(pcidevs, dev)) {
> +                    virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir,
> +                                               oldStateDir);
> +                }
> +            } else {
> +                virErrorPtr err = virGetLastError();
> +                VIR_ERROR(_("Failed to new PCI device: %s"),
> +                          err ? err->message : _("unknown error"));
> +                virResetError(err);
> +            }
> +            virPCIDeviceFree(dev);
> +        }
> +    }
> +
> +    /* Loop 3: reset pci device used by this domain */
>      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>          virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>  
> @@ -834,6 +855,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
>          }
>      }
>  
> +    /* Loop 4: reattach pci devices used by this domain
> +     * and steal all the devices from pcidevs
> +     */
>      while (virPCIDeviceListCount(pcidevs) > 0) {
>          virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0);
>          virHostdevReattachPCIDevice(dev, hostdev_mgr);
>