Re: [libvirt] [PATCH 0/7] Misc improvements
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
> On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
>> As I started to turn more object into using RW locks, I've found
>> couple of
>> areas for improvement too.
>>
>> Michal Privoznik (7):
>> virConnect: Update comment for @privateData
>> Report error if virMutexInit fails
>> virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static
>> again
>> virNetworkObjList: Derive from virObjectRWLockable
>> virNodeDeviceObjList: Derive from virObjectRWLockable
>> virConnect: Derive from virObjectRWLockable
>> storageDriver: Use RW locks
>>
>
> The patches I have not replied to look fine, but I think it would be
> easier to modify the common object after John's patches. Are any of
> those non-conflicting with those series? If yes, I can review those
> into more detail.
>
I had contacted Michal via IRC about this when I saw these hit the list.
I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so
quickly, but what's done is done. I have a couple of other thoughts in
this area:
* I think virObjectLockableRead should return 0/-1 and have the caller
handle it.
* I think there should be a virObjectLockableWrite w/ same return value
checking.
I rather disagree with that - it just adds a massive amount more
code to deal with failures from the lock apis that should never
happen unless you've already screwed up somewhere else in your
code. If the object you've passed into the methods has already
been freed, then you're already doomed and trying to recover from
that is never going to be reliable - in fact it could cause more
trouble. The memory for the object passed in is either in the free
pool (and so shouldn't be touched at all), or has been reused and
allocated for some other object now (and so again touching it is
a bad idea). Trying to detect & handle these situatuons is just
doomed to be racy or dangerous or both
* I think virObjectLock should not handle either RWLockable or
Lockable. It should just handle Lockable.
I think that made sense as an intermediate step, to avoid having
to bulk-convert all code at once, but agree that it would be
reasonable to add a virObjectRWLockWrite() method, convert code
over, and then finally remove the code stuff in virObjectLock
* There could be a virObjectRWUnlock, but virObjectUnlock should be
"OK" to not be specific since theoretically one would have already
locked and got something valid. I think through this common object
series I've found a few instances where Unlock was called with an
Unlocked object which is a different can-o-worms. I have not come across
any instance where Unlock was called with NULL or invalid parameter. And
if it was, the worse thing that could happen is we wouldn't unlock the
resource and that'd be found relatively quickly by the next locker.
Debugging it would be a beachball though.
I think it would make sense to have a virObjectRWUnlock too.
Both of these changes would make it clearer which bit of code is
using a plain lockable object, vs a rwlockable object.
FWIW: As noted in my responses to the RWLock series, consider if
virObjectLock(obj) is called with an invalid @obj, then we really don't
get the lock. All that's done is a VIR_WARN() and return. So if someone
passes the wrong thing we have all sorts of problems. That's been true
of virObjectLock for a long time, but we have (ahem) well behaved code
so less of a problem.
As above, trying to detect these errors & carry on & cleanup is
just giving a false sense of security. I think this is probably a
case where it is reasonable to abort() immediately, because if you
are in this messed up state, the chances are that the code is going
to abort due to memory corrupton shortly anyway.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|