[libvirt] [PATCH 0/7] Misc improvements

[Martin Kletzander]

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?

>
>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/39a147eb/attachment-0001.sig>