Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
On 06/29/2017 08:12 AM, Erik Skultety wrote:
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.
[...]
The main motivation is that in the previous review pass there was a
"dislike" of passing the pointer to a pointer for something else I
changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
So I took the approach that this code could/should follow other API's. I
used cscope and found other similar functions via "vir.*Obj.*Remove" on
the "Find this global definition:" - that led me to 7 functions.
Of those Interface, NWFilter, and StoragePool each used this forward
linked list in a similar manner - so that's what I changed this model to
be. As an aside, nwfilterDefineXML can call virNWFilterObjListRemove,
but not clear @obj when done which means the subsequent if (obj)
virNWFilterObjUnlock(obj) will fail, but I digress.
As you figured out, there's this issue w/ the hal code in dev_refresh()
which *never* would have called dev_create() because dev would be NULL
*always* if virNodeDeviceObjRemove was called.
In hindsight, perhaps I should have solved this by setting a boolean to
force calling dev_create(udi) rather than having/forcing each caller to
handle calling virNodeDeviceObjFree.
Perhaps this gives a flavor for the impetus behind this whole effort -
there are perhaps some hidden bugs I'm finding along the way because the
existing code (in many places) isn't consistent. I have 8 different
models I'm trying to converge - each doing something slightly different.
Luckily most issues I've run into have been in error paths or in this
case in older and hopefully less used callers.
> 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.
I'd like to ignore or get rid of the hal code ;-)
I think (now) the better option would be to have virNodeDeviceObjRemove
make the virNodeDeviceObjFree call as well and have this one caller just
know that it ran through this code path in order to force calling
dev_create() after releasing the node device lock.
Does that seem like a better option?
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
I suppose that's possible, but the comment above the call to
virNodeDeviceObjRemove similar scares the sh*t out of me ;-)
Tks -
John