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.