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

[Erik Skultety]

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.
>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.

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