[libvirt] [PATCH 0/7] Misc improvements

[John Ferlan]

On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
> 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 agree w/ the screw up part. Obviously for me it's the RW vs non-RW
usage that sent me down this path...

Still, I'm not sure what you mean by massive amount of code to deal with
failures. I was considering only the RW lock mgmt.  Currently only
virdomainobjlist was modified to add virObjectLockRead and only done
within the last week. There's 9 virObjectLockRead calls and would be 4
virObjectLockWrite calls.

    if (virObjectLock{Read|Write}(obj) < 0)
        {goto {cleanup|error}|return -1|return NULL};

The only place this doesn't work properly is the vir*Remove() calls
which are void functions. We'd still be "stuck" with them.

Within virObjectLock{Read|Write}() in the failure path:

    virReportError(VIR_ERR_INTERNAL_ERROR,
                   _("unable to obtain rwlock for object=%p"), anyobj);

The code would return 0 on success and -1 on failure.

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

That's fine - I'll add that to my patches before sending...

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

Well I can propose the abort() on error if so desired. I agree w/r/t
some awful things that could happen...

John