[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]