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

On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote: > > > 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}; That's probably buggy if you use existing goto's, because many of those cleanup/error locations will call virObjectUnlock(obj), so you'll need to introduce another set of gotoo labels to optionally skip the unlock step. This is why I think it makes the code more complex for dubious benefit. > The only place this doesn't work properly is the vir*Remove() calls > which are void functions. We'd still be "stuck" with them. Yes that's another scenario I imagined - there are case where it simply isn't practical to do cleanup when locking fails. > Well I can propose the abort() on error if so desired. I agree w/r/t > some awful things that could happen... If we separate virObjectLock vs virObjectRWLockWrite() then, we can just unconditionally reference the object in the virObjectLock method and just let the abort happen naturally, without needing explicit abort

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