External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-07-30 13:37:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

util: restructure virNetDevReadNetConfig() to eliminate false error logs

Browse Source 
virHostdevRestoreNetConfig() calls virNetDevReadNetConfig() to try and
read the "original config" of a netdev, and if that fails, it tries
again with a different directory/netdev name. This achieves the
desired effect (we end up finding the config wherever it may be), but
for each failure, virNetDevReadNetConfig() places a nice error message
in the system logs. Experience has shown that false-positive error
logs like this lead to erroneous bug reports, and can often mislead
those searching for *real* bugs.

This patch changes virNetDevReadNetConfig() to explicitly check if the
file exists before calling virFileReadAll(); if it doesn't exist,
virNetDevReadNetConfig() returns a success, but leaves all the
variables holding the results as NULL. (This makes sense if you define
the purpose of the function as "read a netdev's config from its config
file *if that file exists*).

To take advantage of that change, the caller,
virHostdevRestoreNetConfig() is modified to fail immediately if
virNetDevReadNetConfig() returns an error, and otherwise to try the
different directory/netdev name if adminMAC & vlan & MAC are all NULL
after the preceding attempt.
This commit is contained in:
Laine Stump 2017-08-09 20:49:26 -04:00
parent b67eaa6351
commit 9a08168301
3 changed files with 94 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

122
src/util/virhostdev.c
View File

@ -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;
/* 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.
*/
ret = virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC);
if (ret < 0 && oldStateDir)
ret = virNetDevReadNetConfig(linkdev, vf, oldStateDir,
&adminMAC, &vlan, &MAC);
/* 1) standard location */
if (virNetDevReadNetConfig(linkdev, vf, stateDir,
&adminMAC, &vlan, &MAC) < 0) {
goto cleanup;
}
if (ret < 0) {
/* see if the config was saved using the PF's "port 2"
* netdev for the file name.
*/
/* 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) {
ret = virNetDevReadNetConfig(linkdev, vf, stateDir,
&adminMAC, &vlan, &MAC);
if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 ||
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;
}
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;
}

16
src/util/virnetdev.c
View File

@ -1971,7 +1971,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* @linkdev:@vf from a file in @stateDir. (see virNetDevSaveNetConfig
* for details of file name and format).
*
* Returns 0 on success, -1 on failure.
* Returns 0 on success, -1 on failure. It is *NOT* considered failure
* if no file is found to read. In that case, adminMAC, vlan, and MAC
* are set to NULL, and success is returned.
*
* The caller MUST free adminMAC, vlan, and MAC when it is finished
* with them (they will be NULL if they weren't found in the file)
@ -2036,8 +2038,8 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
if (linkdev && !virFileExists(filePath)) {
/* the device may have been stored in a file named for the
* VF due to saveVlan == false (or an older version of
* libvirt), so reset filePath so we'll try the other
* filename before failing.
* libvirt), so reset filePath and pfDevName so we'll try
* the other filename.
*/
VIR_FREE(filePath);
pfDevName = NULL;
@ -2049,6 +2051,14 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
goto cleanup;
}
if (!virFileExists(filePath)) {
/* having no file to read is not necessarily an error, so we
* just return success, but with MAC, adminMAC, and vlan set to NULL
*/
ret = 0;
goto cleanup;
}
if (virFileReadAll(filePath, 128, &fileStr) < 0)
goto cleanup;

5
src/util/virnetdevmacvlan.c
View File

@ -1187,8 +1187,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
virMacAddrPtr adminMAC = NULL;
virNetDevVlanPtr vlan = NULL;
if (virNetDevReadNetConfig(linkdev, -1, stateDir,
&adminMAC, &vlan, &MAC) == 0) {
if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
&adminMAC, &vlan, &MAC) == 0) &&
(adminMAC || vlan || MAC)) {
ignore_value(virNetDevSetNetConfig(linkdev, -1,
adminMAC, vlan, MAC, !!vlan));