Re: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly
On 07/13/2017 07:19 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
>> after the call virSecretObjListRemove, be more explicit by calling
>> virObjectUnref and setting @obj to NULL for secretUndefine and in
>> the error path of secretDefineXML.
>>
>> This also fixes a leak during virSecretLoad if the virSecretLoadValue
>> fails the code jumps to cleanup without setting @ret = obj, thus calling
>> virSecretObjListRemove which only accounts for the object reference
>> related to adding the object to the list during virSecretObjListAdd,
>> but does not account for the reference to the object itself as the
>> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
>> on the object recently added thus reducing the refcnt to zero. Thus
>> cleaning up the virSecretLoadValue error path to make it clearer what
>> needs to be done on failure.
>
> I think the real reason is that we cannot call virSecretObjEndAPI()
> because that *unlocks* the secret object. However, the object is already
> unlocked at that point by virSecretObjListRemove() and thus we would
> unlock twice while locking just once. Honestly, I'd rather see that
> explanation in the commit message. But it's your call.
>
Partially true - although calling Unlock on something already Unlocked
by the same thread IIRC doesn't do much other than cause an error, but
we don't fail on that error.
I don't think this is true. Just consider the following general example:
Thread A | Thread B
------------------------------
1) lock(X) |
2) | lock(x)
3) unlock(X) |
4) unlock(X) |
5) | work(X)
In this scenario, thread B starts working on X thinking it's locked
while it isn't. Also, man-page for pthread_mutex_lock says that
unlocking an unlocked lock leads to undefined behaviour (we use NORMAL
non-robust mutexes. And yes, I had to search for what does it mean
non-robust mutex).
It's ironic this relates to something Erik and I discussed during the
virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now
has a reference on the object and would always need to Unref it even
after removing it from the List.
About that. I think that if we do our ref counting right, we don't need
to do any special unref(). I mean:
undefine(X) {
obj = lookup(X); // obj now has refcnt = 2, one beacuse it's in the
// list, one for the undefine()
listRemove(obj); // obj.refcnt = 1, the list reference is gone
endApi(&obj); // undefine() reference is gone, obj is freed
}
Of course, if list uses two hash tables (one for UUIDs one for names) it
holds two references, but then listRemove() also unref()-s twice.
This just happened to be where I had that oh sh*t moment and realized
that when calling Remove we were essentially unlocking and thus yes
calling EndAPI would unlock and unlocked object. I'll add something in
about that..
John
As an aside, this is exactly why I started down the path of common
objects. Consider how the callers to virDomainObjListAdd go through
great lengths to managed the returned object some quite differently
based on what they know about how many refs are returned and whether the
calling function calls virDomainObjEndAPI. If it does, there's always a
virObjectRef done on the object after the Add returns. It's a subtle
thing, but confusing nonetheless.
The thing is the *Remove API will call virHashRemoveEntry which
decrements a refcnt for each table; however, the *Add API only
increments the refcnt once for adding into each table. Callers are very
careful to understand and manage that.
I haven't looked into the code, but if this is true then listAdd() and
listRemove() functions are broken because they mangle refcount. It
should be able to do the following and having the refcount of an object
unchanged at the end:
listAdd(obj);
listRemove(obj);
If the refcount is not the same at the end as it was at the beginning,
the list functions are broken and need to be fixed
I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj
the @obj regardless if Add or Lookup was used. The callers shouldn't
have to know they need to either use *EndAPI or ObjUnlock. In any case,
I digress and that's a different issue for another day
I totally agree on this!
Michal