[libvirt] [PATCH] nodedev: update netdev feature bits before each dumpxml

[Laine Stump]

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 at 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!