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

On Mon, Jul 31, 2017 at 08:58:41AM +0200, Martin Kletzander wrote: > On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote: > > 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 > > > > I agree with most of it, but I can't wrap my head around what you meant > by this paragraph, could you explain it to someone whose brain is just > not working yet, please? Currently we have: void virObjectLock(void *anyobj) { if (virObjectIsClass(anyobj, virObjectLockableClass)) { virObjectLockablePtr obj = anyobj; virMutexLock(&obj->lock); } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { virObjectRWLockablePtr obj = anyobj; virRWLockWrite(&obj->lock); } else { virObjectPtr obj = anyobj; VIR_WARN("Object %p (%s) is not a virObjectLockable " "nor virObjectRWLockable instance", anyobj, obj ? obj->klass->name : "(unknown)"); } } What I'm suggesting is void virObjectLock(void *anyobj) { virObjectLockablePtr obj = anyobj; virMutexLock(&obj->lock); } void virObjectRWLock(void *anyobj) { virObjectRWLockablePtr obj = anyobj; virRWLockWrite(&obj->lock); } eg just assume the caller has written code correctly and passing the right type of object.

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