Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by
value
and then let the caller decide whether or not the object should be free'd.
This function should just handle the remove of the object from the list
for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path
since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm
wondering why do we actually have to remove the device from the list when
handling 'change'/'update' for hal instead of just replacing the ->def
with a
new instance but it's perfectly fine to do that for udev...I don't see the
point in doing what we currently do for hal.
[...]
diff --git a/src/node_device/node_device_hal.c
b/src/node_device/node_device_hal.c
index f468e42..c354cd3 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -514,7 +514,7 @@ dev_refresh(const char *udi)
/* Simply "rediscover" device -- incrementally handling changes
* to sub-capabilities (like net.80203) is nasty ... so avoid it.
*/
- virNodeDeviceObjRemove(&driver->devs, &dev);
+ virNodeDeviceObjRemove(&driver->devs, dev);
I guess now that freeing '@dev' is caller's responsibility, you want to free
it
on function exit after you checked that you actually want to recreate the
device - I already expressed my opinion about this above.
ACK with @dev also freed in hal backend. I'd also like to hear your opinion on
calling *AssignDef directly from hal's dev_refresh rather than first removing
the device from the list completely.
Erik