[libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove

[Erik Skultety]

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