[libvirt] [PATCH v2 6/7] util: restructure virNetDevReadNetConfig() to eliminate false error logs

[John Ferlan]

[...]
> 
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 102fd85c1..cca9d81a4 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -534,6 +534,10 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
>      int ret = -1;
>      int vf = -1;
>      bool port_profile_associate = false;
> +    virMacAddrPtr MAC = NULL;
> +    virMacAddrPtr adminMAC = NULL;
> +    virNetDevVlanPtr vlan = NULL;
> +
>  
>      /* This is only needed for PCI devices that have been defined
>       * using <interface type='hostdev'>. For all others, it is a NOP.
> @@ -559,62 +563,92 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
>                                                   NULL,
>                                                   port_profile_associate);
>      } else {
> -        virMacAddrPtr MAC = NULL;
> -        virMacAddrPtr adminMAC = NULL;
> -        virNetDevVlanPtr vlan = NULL;
> -
> -        ret = virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC);
> -        if (ret < 0 && oldStateDir)
> -            ret = virNetDevReadNetConfig(linkdev, vf, oldStateDir,
> -                                         &adminMAC, &vlan, &MAC);
> -
> -        if (ret < 0) {
> -            /* see if the config was saved using the PF's "port 2"
> -             * netdev for the file name.
> -             */
> -            VIR_FREE(linkdev);
> +        /* we need to try 3 different places for the config file:
> +         * 1) ${stateDir}/${PF}_vf${vf}
> +         *    This is almost always where the saved config is
> +         *
> +         * 2) ${oldStateDir/${PF}_vf${vf}
> +         *    saved config is only here if this machine was running a
> +         *    (by now *very*) old version of libvirt that saved the
> +         *    file in a different directory
> +         *
> +         * 3) ${stateDir}${PF[1]}_vf${VF}
> +         *    PF[1] means "the netdev for port 2 of the PF device", and
> +         *    is only valid when the PF is a Mellanox dual port NIC with
> +         *    a VF that was created in "single port" mode.
> +         *
> +         *  NB: if virNetDevReadNetConfig() returns < 0, then it found
> +         *  the file, but there was a problem, so we should
> +         *  immediately return an error to our caller. If it returns
> +         *  0, but all of the interesting stuff is NULL, that means
> +         *  the file wasn't found, so we can/should check other
> +         *  locations for it.
> +         */
>  
> -            if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) >= 0) {
> -                ret = virNetDevReadNetConfig(linkdev, vf, stateDir,
> -                                             &adminMAC, &vlan, &MAC);
> -            }
> +        /* 1) standard location */
> +        if (virNetDevReadNetConfig(linkdev, vf, stateDir,
> +                                   &adminMAC, &vlan, &MAC) < 0) {
> +            goto cleanup;
>          }
>  
> -        if (ret == 0) {
> -            /* if a MAC was stored for the VF, we should now restore
> -             * that as the adminMAC. We have to do it this way because
> -             * the VF is still not bound to the host's net driver, so
> -             * we can't directly set its MAC (and even after it is
> -             * re-bound to the host net driver, it will still have its
> -             * "administratively set" flag on, and that prohibits the
> -             * VF's net driver from directly setting the MAC
> -             * anyway). But it we set the desired VF MAC as the "admin
> -             * MAC" *now*, then when the VF is re-bound to the host
> -             * net driver (which will happen soon after returning from
> -             * this function), that adminMAC will be set (by the PF)
> -             * as the VF's new initial MAC.
> -             *
> -             * If no MAC was stored for the VF, that means it wasn't
> -             * bound to a net driver before we used it anyway, so the
> -             * adminMAC is all we have, and we can just restore it
> -             * directly.
> -             */
> -            if (MAC) {
> -                VIR_FREE(adminMAC);
> -                adminMAC = MAC;
> -                MAC = NULL;
> +        /* 2) "old" (pre-1.2.3 circa 2014) location - whenever we get
> +        *  to the point that nobody will ever upgrade directly from
> +        *  1.2.3 (or older) directly to current libvirt, we can
> +        *  eliminate this clause
> +        **/
> +        if (!(adminMAC || vlan || MAC) && oldStateDir &&
> +            virNetDevReadNetConfig(linkdev, vf, oldStateDir,
> +                                   &adminMAC, &vlan, &MAC) < 0) {
> +            goto cleanup;
> +        }
> +
> +        /* 3) try using the PF's "port 2" netdev as the name of the
> +         * config file
> +         */
> +        if (!(adminMAC || vlan || MAC)) {
> +            VIR_FREE(linkdev);
> +
> +            if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 ||
> +                virNetDevReadNetConfig(linkdev, vf, stateDir,
> +                                       &adminMAC, &vlan, &MAC) < 0) {
> +                goto cleanup;
>              }
> +        }
>  
> -            ignore_value(virNetDevSetNetConfig(linkdev, vf,
> -                                               adminMAC, vlan, MAC, true));
> +        /* if a MAC was stored for the VF, we should now restore
> +         * that as the adminMAC. We have to do it this way because
> +         * the VF is still not bound to the host's net driver, so
> +         * we can't directly set its MAC (and even after it is
> +         * re-bound to the host net driver, it will still have its
> +         * "administratively set" flag on, and that prohibits the
> +         * VF's net driver from directly setting the MAC
> +         * anyway). But it we set the desired VF MAC as the "admin
> +         * MAC" *now*, then when the VF is re-bound to the host
> +         * net driver (which will happen soon after returning from
> +         * this function), that adminMAC will be set (by the PF)
> +         * as the VF's new initial MAC.
> +         *
> +         * If no MAC was stored for the VF, that means it wasn't
> +         * bound to a net driver before we used it anyway, so the
> +         * adminMAC is all we have, and we can just restore it
> +         * directly.
> +         */
> +        if (MAC) {
> +            VIR_FREE(adminMAC);
> +            adminMAC = MAC;
> +            MAC = NULL;
>          }
>  
> -        VIR_FREE(MAC);
> -        VIR_FREE(adminMAC);
> -        virNetDevVlanFree(vlan);
> +        ignore_value(virNetDevSetNetConfig(linkdev, vf,
> +                                           adminMAC, vlan, MAC, true));
>      }
>  
> +    ret = 0;

Coverity notes that in the first half of the if statement :

ret = virHostdevNetConfigVirtPortProfile(...)

However, this overwrites that.

John

> + cleanup:
>      VIR_FREE(linkdev);
> +    VIR_FREE(MAC);
> +    VIR_FREE(adminMAC);
> +    virNetDevVlanFree(vlan);
>  
>      return ret;
>  }
[...]