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

[John Ferlan]

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
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
that @dev was found and removed. Still adding a bool rediscover just
makes it more obvious for the next poor soul tripping across this code.

If you search through history, you'll find that commit '611480747'
introduced this hal dev_refresh anomaly in order to fix a problem in
testNodeDeviceDestroy that @obj would not be present. Instead of setting
obj = NULL; after calls, the usage of &dev was introduced. I'm undoing
that and taking the other approach to set the removed @dev = NULL;  (all
that during a freeze, too!).

See the quagmire this convergence creates ;-)

Anyway, I've attached a patch that should be able to be git am'd on top
of this one. It will cause merge conflicts in the rest of the series...


John

>> 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.
> 
> I went for basically every module equivalent of this. So the unnecessary lock
> dance just to compare pointers could be a fairly straightforward follow-up
> patch across multiple modules if they're consistent.
> 
>>
>> 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,
> 
> I won't comment on NWFilter as I'm not familiar with it at all.
> 
>>
>> 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.
> 
> See my comment above, I think that would be a step back in what we're trying to
> achieve here (comparing it also with other OO languages' practice in this
> aspect).
> 
>>>>          /* 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 ;-)
> 
> Who wouldn't, honestly...
> 
>>
>> 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?
> 
> From my perspective, not really (but I might be wrong of course, wouldn't be
> the first nor the last time).
> 
>>
>>> 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 ;-)
> 
> It just needs a bit of love....and a bunch of cups of coffee ;)...and a
> platform to test.
> 
> Erik
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-merge-and-alter-commit-message-to.patch
Type: text/x-patch
Size: 3974 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170629/4501d8cd/attachment-0001.bin>