[libvirt] [PATCH] nodedev: update netdev feature bits before each dumpxml
As with several other attributes of devices (link status, sriov VF list, IOMMU group list), the detdev feature bits aren't automatically updated in the nodedev driver's cache when they change. In order to get a properly up-to-date list when getting the XML of a device, we must reget them in update-caps prior to each dumpxml. Reported-By: Moshe Levi <moshele@mellanox.com> --- I dislike needing to put in the virBitmapFree and set the pointer to NULL before re-getting the features, but leaving it out would lead to a leak of the old bitmap, and I'm not sure I want virNetDevGetFeatures() to assume a valid pointer when it starts (it currently assumes the pointer contents is junk, and overwrites it with a newly allocated *virBitmap). src/node_device/node_device_driver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 768db7f..31741b9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -58,6 +58,10 @@ static int update_caps(virNodeDeviceObjPtr dev) case VIR_NODE_DEV_CAP_NET: if (virNetDevGetLinkInfo(cap->data.net.ifname, &cap->data.net.lnk) < 0) return -1; + virBitmapFree(cap->data.net.features); + cap->data.net.features = NULL; + if (virNetDevGetFeatures(cap->data.net.ifname, &cap->data.net.features) < 0) + return -1; break; case VIR_NODE_DEV_CAP_PCI_DEV: if (nodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path, -- 2.1.0
On Tue, Jun 16, 2015 at 12:03:55PM -0400, Laine Stump wrote:
As with several other attributes of devices (link status, sriov VF list, IOMMU group list), the detdev feature bits aren't automatically
s/det/net/
updated in the nodedev driver's cache when they change. In order to get a properly up-to-date list when getting the XML of a device, we must reget them in update-caps prior to each dumpxml.
Reported-By: Moshe Levi <moshele@mellanox.com> ---
I dislike needing to put in the virBitmapFree and set the pointer to NULL before re-getting the features, but leaving it out would lead to a leak of the old bitmap, and I'm not sure I want virNetDevGetFeatures() to assume a valid pointer when it starts (it currently assumes the pointer contents is junk, and overwrites it with a newly allocated *virBitmap).
Setting it to NULL is not strictly required, since the GetFeatures() call will always overwrite it. Maybe the code would be nicer if virBitmapFree also cleared the pointer and virNetDevGetFeatures returned the bitmap instead of an int?
src/node_device/node_device_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 768db7f..31741b9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -58,6 +58,10 @@ static int update_caps(virNodeDeviceObjPtr dev) case VIR_NODE_DEV_CAP_NET: if (virNetDevGetLinkInfo(cap->data.net.ifname, &cap->data.net.lnk) < 0) return -1; + virBitmapFree(cap->data.net.features); + cap->data.net.features = NULL; + if (virNetDevGetFeatures(cap->data.net.ifname, &cap->data.net.features) < 0) + return -1; break; case VIR_NODE_DEV_CAP_PCI_DEV: if (nodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path,
ACK with the typo fixed. Jan
On 06/17/2015 04:35 AM, Ján Tomko wrote:
On Tue, Jun 16, 2015 at 12:03:55PM -0400, Laine Stump wrote:
As with several other attributes of devices (link status, sriov VF list, IOMMU group list), the detdev feature bits aren't automatically s/det/net/
updated in the nodedev driver's cache when they change. In order to get a properly up-to-date list when getting the XML of a device, we must reget them in update-caps prior to each dumpxml.
Reported-By: Moshe Levi <moshele@mellanox.com> ---
I dislike needing to put in the virBitmapFree and set the pointer to NULL before re-getting the features, but leaving it out would lead to a leak of the old bitmap, and I'm not sure I want virNetDevGetFeatures() to assume a valid pointer when it starts (it currently assumes the pointer contents is junk, and overwrites it with a newly allocated *virBitmap).
Setting it to NULL is not strictly required, since the GetFeatures() call will always overwrite it.
Yes, you are correct. I was being overly paranoid and assuming that an error return from that function might leave the pointer unmodified, but looking at it now I can verify it will always be valid on exit. So I'm going to remove the blah = NULL before I push.
Maybe the code would be nicer if virBitmapFree also cleared the pointer and virNetDevGetFeatures returned the bitmap instead of an int?
Having virBitmapFree() would make it cleaner to use and would make it more like the VIR_FREE() macro, but would make it behave differently from all the other vir*Free() functions in libvirt. So in the name of consistency, I thin we should leave it as it is. (and as you pointed out, it's not necessary to clear it in this case).
src/node_device/node_device_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 768db7f..31741b9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -58,6 +58,10 @@ static int update_caps(virNodeDeviceObjPtr dev) case VIR_NODE_DEV_CAP_NET: if (virNetDevGetLinkInfo(cap->data.net.ifname, &cap->data.net.lnk) < 0) return -1; + virBitmapFree(cap->data.net.features); + cap->data.net.features = NULL; + if (virNetDevGetFeatures(cap->data.net.ifname, &cap->data.net.features) < 0) + return -1; break; case VIR_NODE_DEV_CAP_PCI_DEV: if (nodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path, ACK with the typo fixed.
Thanks!
participants (2)
-
Ján Tomko -
Laine Stump