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

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

From my perspective, if it's the caller who's responsible to free it, they'll

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