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

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;

+ cleanup: VIR_FREE(linkdev); + VIR_FREE(MAC); + VIR_FREE(adminMAC); + virNetDevVlanFree(vlan); return ret; }