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

[John Ferlan]

On 06/30/2017 04:40 AM, Erik Skultety wrote:
> On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
>>
>>
>> On 06/29/2017 10:28 AM, Erik Skultety wrote:
>>> On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
>>>>
>>>>
>>>> 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
>>>>
>>>
>>> Well, that comment is true, but the commit message of this patch says that it
>>> drops the requirement of passing by reference, thus leaving the responsibility
>>> to free the obj to the caller. Now, the way I see it what we should aim at
>>> achieving here is reference counted objects, so the vir*ObjFree in the caller
>>> would become and *Unref. Therefore IMHO it's not the best approach to introduce
>>> another boolean for HAL and leave the vir*ObjFree in the Remove routine, you
>>> wouldn't be clearing the object for the caller anyway, so I don't think that is
>>> the way to go.
>>>
>>
>> I actually think the better course of action is to be more like the
>> other functions. I believe virNodeDeviceObjRemove should do the
>> virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the
>> alloc and list insertion and *Remove is the antecedent doing the list
> 
> Well, eventually we'll hopefully end up with reference-counted objects so doing
> it with having *ObjFree in ObjRemove or in the caller won't matter in the end.

Keeping the Free/Unref as part of the callers responsibility would mean
that once we ended up with common object code doing the remove, then
each of the nodedev callers would need to remove their extra Free/Unref.
Which like you point out below is essentially equal work as long as one
remembers that when it comes time ;-).

> From my perspective, if it's the caller who's responsible to free it, they'll
> know they cannot touch the pointer afterwards (yeah...I assumed there are no
> bugs) whereas if freeing the object is done as part of ObjRemove, it's the
> caller who's the one to assume if the object was both freed and cleared or just
> freed or neither - which I don't like that much, but again, matter of
> perspective, I see your reasoning though.
> You'll have to replace frees with unrefs anyway, thus you won't make it any
> easier by this approach, the amount of work in both cases we're discussing here
> is equal, so I'll be probably fine with that adjustment as well.
> 
>> removal and free. I will adjust the commit message and can even add
>> comments prior to the function (if desired; however, it'll eventually be
>> just a wrapper to a more generic object function).
>>
>> Also, light has dawned over marble head and for hal's dev_refresh the
>> logic will then work correctly anyway since &dev wouldn't be set to
>> NULL. The code doesn't reference data in @dev. All that is important is
> 
> Relying on a dangling pointer just for the sole purpose of checking 'yep, there
> used to be something some time ago' would be very very very poor and fragile
> design. Adding a bool there is still only a workaround of a previous workaround
> (the commit you referenced), which doesn't necessarily make the 'more nicely
> packaged' fix a better solution. Per [1] we should call dev_create(udi) right
> away, dropping the driver lock just before the call - so much more readable.
> So I don't agree with the boolean part of the patch attached.

Personally I was more in favor of the pass by reference logic because it
leaves no question. I'm not as much of a fan of the (sometimes hidden)
knowledge that a function "consumes" a callers pointer. We have quite a
few of those sprinkled throughout the code. The few code analyzers I've
used over time prefer logic that doesn't require some caller to set a
pointer they passed to a function to NULL after the function
successfully completes. Opinions in this team are different though.

Anyway, from your comment to drop the boolean the logic is either:

Lock
dev = FindByName
if (dev)
   Remove
   Unlock
   dev_create
else
   DEBUG
   Unlock

Or adding a return after the dev_create to avoid having a second Unlock
in the else.  IDC either way, just want to be clear.

FWIW: The boolean would be short lived... Once the ObjList is a self
locking virLockableObject (patch 13), the boolean would disappear in
patch 14 with the dev_create moving inside the "if" part. I already have
that in my branch, but I'm sure it wouldn't apply due to merge conflicts
as a result of changes made as part of the review process.

John

> 
> [1] https://www.redhat.com/archives/libvir-list/2017-June/msg01328.html
>