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@xxxxxxxxxxxx> >> --- >> >> 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! -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list