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

[John Ferlan]

On 06/30/2017 07:41 AM, Erik Skultety wrote:
> On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote:
>>
>>
>> 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.
> 
> I don't think so. Looking at other examples, what happens in most cases is that
> caller already gets the ref'd object, calls Remove() which calls Unref(),
> returns to the caller who then calls EndAPI() which in turn destroys the last
> reference. Therefore in here, to actually get rid of the object all the callers
> would need to do essentially the same thing - keep their Unref() in the function
> block, solely because GetObj() returns an already locked and ref'd object.
> 

Doh...  true - of course I have in mind the end goal which is making
this whole process awfully difficult. Going step by step hasn't really
helped as much as it should have /-(.  Adjustments to later patches will
be necessary to either Unref or EndAPI.

Without getting into specifics of each EndAPI model (domain, network,
and secret) - I'm assuming you can agree that each does things a bit
differently and gives a sense for the impetus behind this overall
effort. I generally sit here looking at code keeping track on my fingers
of the RefCnt and whether the lock is held ;-)

>> 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
> 
> I wanted to get rid of this^ completely....in a separate patch...
> 
>>    Unlock
>>    dev_create
> 
> and call ^this directly, the remove there doesn't serve a purpose anymore.
> 

I'm not sure I follow... Like noted previously trying not to disturb the
logic here - just applying a different band-aid

The purpose of the Remove AIUI is to allow the code to rediscover some
device because no one wanted to handle the messy situation of some
capability being removed or some property being modified. Instead it was
deemed a better option to just Remove the device and allow dev_create
reinstantiate.

Go back to the original implementation, commit id '620d4be7a'... The
locks were added as part of commit id 'd48717054c7' and the dev_refresh
in commit id '6244c173b' (to fix a deadlock).

I think the Remove is necessary.

>> 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
> 
> I know it would - so why introduce it in the first place if we can achieve the
> same thing in a different way, especially because its removal would be only a
> couple of patches apart (I know...there are exceptions - this does not qualify
> as one).
> 
> Erik
> 

I can remove the bool, IDC, but unless I can understand why you don't
think the Remove is necessary - it's either a bool or a move
dev_create() inside the if, after an Unlock and a return in the if
condition (at least temporarily).

John