[libvirt] [PATCH 0/7] Misc improvements
Martin Kletzander
mkletzan at redhat.com
Mon Jul 31 21:34:14 UTC 2017
On Mon, Jul 31, 2017 at 10:26:30AM +0100, Daniel P. Berrange wrote:
>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.
>
So no error checking, not aborts, nothing. I liked the possibility of
gradual changes from Mutexes to RWLocks when Lock() handled both. I
understand we don't want to have any abort()s in our code, but I'm not
really sure for this one. I also think we're missing lot of error
handling in virthread (merely due to multiple implementations in the
past?).
Anyway, there will always be room for improvement.
>
>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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170731/5d1f3fb2/attachment-0001.sig>
More information about the libvir-list
mailing list