Re: [libvirt] [PATCH 0/7] Misc improvements

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.

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