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

 src/bhyve/bhyve_driver.c               |  1 +
 src/conf/virnetworkobj.c               | 42 ++++++++++----------------------
 src/conf/virnetworkobj.h               |  8 -------
 src/conf/virnodedeviceobj.c            | 16 ++++++-------
 src/conf/virstorageobj.h               |  2 +-
 src/datatypes.c                        |  6 +++--
 src/datatypes.h                        |  6 ++---
 src/libvirt_private.syms               |  2 --
 src/lxc/lxc_driver.c                   |  1 +
 src/lxc/lxc_fuse.c                     |  4 +++-
 src/network/bridge_driver.c            |  1 +
 src/node_device/node_device_hal.c      |  1 +
 src/nwfilter/nwfilter_dhcpsnoop.c      | 12 +++++++---
 src/nwfilter/nwfilter_driver.c         |  5 +++-
 src/nwfilter/nwfilter_gentech_driver.c |  4 +++-
 src/secret/secret_driver.c             |  2 ++
 src/storage/storage_driver.c           | 44 +++++++++++++++++++---------------
 src/uml/uml_driver.c                   |  1 +
 src/util/virerror.c                    |  2 +-
 src/util/virnetlink.c                  |  1 +
 src/util/virthreadpool.c               |  4 +++-
 src/vmware/vmware_driver.c             |  5 +++-
 src/vz/vz_driver.c                     |  4 +++-
 tools/virsh-console.c                  |  4 +++-
 24 files changed, 94 insertions(+), 84 deletions(-)

-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

> src/bhyve/bhyve_driver.c               |  1 +
> src/conf/virnetworkobj.c               | 42 ++++++++++----------------------
> src/conf/virnetworkobj.h               |  8 -------
> src/conf/virnodedeviceobj.c            | 16 ++++++-------
> src/conf/virstorageobj.h               |  2 +-
> src/datatypes.c                        |  6 +++--
> src/datatypes.h                        |  6 ++---
> src/libvirt_private.syms               |  2 --
> src/lxc/lxc_driver.c                   |  1 +
> src/lxc/lxc_fuse.c                     |  4 +++-
> src/network/bridge_driver.c            |  1 +
> src/node_device/node_device_hal.c      |  1 +
> src/nwfilter/nwfilter_dhcpsnoop.c      | 12 +++++++---
> src/nwfilter/nwfilter_driver.c         |  5 +++-
> src/nwfilter/nwfilter_gentech_driver.c |  4 +++-
> src/secret/secret_driver.c             |  2 ++
> src/storage/storage_driver.c           | 44 +++++++++++++++++++---------------
> src/uml/uml_driver.c                   |  1 +
> src/util/virerror.c                    |  2 +-
> src/util/virnetlink.c                  |  1 +
> src/util/virthreadpool.c               |  4 +++-
> src/vmware/vmware_driver.c             |  5 +++-
> src/vz/vz_driver.c                     |  4 +++-
> tools/virsh-console.c                  |  4 +++-
> 24 files changed, 94 insertions(+), 84 deletions(-)
>
>--
>2.13.0
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 think virObjectLock should not handle either RWLockable or
Lockable. It should just handle Lockable.
 * There could be a virObjectRWUnlock, but virObjectUnlock should be
"OK" to not be specific since theoretically one would have already
locked and got something valid. I think through this common object
series I've found a few instances where Unlock was called with an
Unlocked object which is a different can-o-worms. I have not come across
any instance where Unlock was called with NULL or invalid parameter. And
if it was, the worse thing that could happen is we wouldn't unlock the
resource and that'd be found relatively quickly by the next locker.
Debugging it would be a beachball though.

I do have some patches I put together which I'll post some time today
with any luck...

John

FWIW: As noted in my responses to the RWLock series, consider if
virObjectLock(obj) is called with an invalid @obj, then we really don't
get the lock. All that's done is a VIR_WARN() and return. So if someone
passes the wrong thing we have all sorts of problems. That's been true
of virObjectLock for a long time, but we have (ahem) well behaved code
so less of a problem.

Introducing virObjectRWLockable gives us the opportunity to not only
have concurrency in locks, but also to allow error checking which should
have been done in virObjectLock, but there's way too many callers now to
undo that and using abort() or something similar to that void function
is considered bad form. Also IMO having a virObjectLockRead could lead
someone to believe that passing a virObjectLockable object would work
just fine.  Which, well it would, but not really. Passing a
virObjectLockable would cause a VIR_WARN message and return without the
resource really locked, which isn't good. Since LockRead and Lock can be
used in the same code path it means we need to be more careful when
reviewing to know which passed argument was a RWLock style lock. Right
now that's @doms or @domlist for RWLocks and @obj, @dom, and @vm for
existing locks. Not look closely enough and you'll miss that @dom cannot
be use for RWLock in the same code path that's using @doms that can be
used that way.

If the goal is to eventually convert to using RWLocks more, then take
the plunge now to check the status or forever be shackled with the
problem that virObjectLock has. When converting over a lock to be an
RWLock that means any code that touches the lock will need to use either
a Read or Write API, but to me that's goodness.

>> src/bhyve/bhyve_driver.c               |  1 +
>> src/conf/virnetworkobj.c               | 42
>> ++++++++++----------------------
>> src/conf/virnetworkobj.h               |  8 -------
>> src/conf/virnodedeviceobj.c            | 16 ++++++-------
>> src/conf/virstorageobj.h               |  2 +-
>> src/datatypes.c                        |  6 +++--
>> src/datatypes.h                        |  6 ++---
>> src/libvirt_private.syms               |  2 --
>> src/lxc/lxc_driver.c                   |  1 +
>> src/lxc/lxc_fuse.c                     |  4 +++-
>> src/network/bridge_driver.c            |  1 +
>> src/node_device/node_device_hal.c      |  1 +
>> src/nwfilter/nwfilter_dhcpsnoop.c      | 12 +++++++---
>> src/nwfilter/nwfilter_driver.c         |  5 +++-
>> src/nwfilter/nwfilter_gentech_driver.c |  4 +++-
>> src/secret/secret_driver.c             |  2 ++
>> src/storage/storage_driver.c           | 44
>> +++++++++++++++++++---------------
>> src/uml/uml_driver.c                   |  1 +
>> src/util/virerror.c                    |  2 +-
>> src/util/virnetlink.c                  |  1 +
>> src/util/virthreadpool.c               |  4 +++-
>> src/vmware/vmware_driver.c             |  5 +++-
>> src/vz/vz_driver.c                     |  4 +++-
>> tools/virsh-console.c                  |  4 +++-
>> 24 files changed, 94 insertions(+), 84 deletions(-)
>>
>> -- 
>> 2.13.0
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 think virObjectLock should not handle either RWLockable or
> Lockable. It should just handle Lockable.

I think that made sense as an intermediate step, to avoid having
to bulk-convert all code at once, but agree that it would be
reasonable to add a virObjectRWLockWrite() method, convert code
over, and then finally remove the code stuff in virObjectLock

>  * There could be a virObjectRWUnlock, but virObjectUnlock should be
> "OK" to not be specific since theoretically one would have already
> locked and got something valid. I think through this common object
> series I've found a few instances where Unlock was called with an
> Unlocked object which is a different can-o-worms. I have not come across
> any instance where Unlock was called with NULL or invalid parameter. And
> if it was, the worse thing that could happen is we wouldn't unlock the
> resource and that'd be found relatively quickly by the next locker.
> Debugging it would be a beachball though.

I think it would make sense to have a virObjectRWUnlock too.

Both of these changes would make it clearer which bit of code is
using a plain lockable object, vs a rwlockable object.

> FWIW: As noted in my responses to the RWLock series, consider if
> virObjectLock(obj) is called with an invalid @obj, then we really don't
> get the lock. All that's done is a VIR_WARN() and return. So if someone
> passes the wrong thing we have all sorts of problems. That's been true
> of virObjectLock for a long time, but we have (ahem) well behaved code
> so less of a problem.

As above, trying to detect these errors & carry on & cleanup is
just giving a false sense of security. I think this is probably a
case where it is reasonable to abort() immediately, because if you
are in this messed up state, the chances are that the code is going
to abort due to memory corrupton shortly anyway.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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};

The only place this doesn't work properly is the vir*Remove() calls
which are void functions. We'd still be "stuck" with them.

Within virObjectLock{Read|Write}() in the failure path:

    virReportError(VIR_ERR_INTERNAL_ERROR,
                   _("unable to obtain rwlock for object=%p"), anyobj);

The code would return 0 on success and -1 on failure.

>>  * I think virObjectLock should not handle either RWLockable or
>> Lockable. It should just handle Lockable.
> 
> I think that made sense as an intermediate step, to avoid having
> to bulk-convert all code at once, but agree that it would be
> reasonable to add a virObjectRWLockWrite() method, convert code
> over, and then finally remove the code stuff in virObjectLock
> 
>>  * There could be a virObjectRWUnlock, but virObjectUnlock should be
>> "OK" to not be specific since theoretically one would have already
>> locked and got something valid. I think through this common object
>> series I've found a few instances where Unlock was called with an
>> Unlocked object which is a different can-o-worms. I have not come across
>> any instance where Unlock was called with NULL or invalid parameter. And
>> if it was, the worse thing that could happen is we wouldn't unlock the
>> resource and that'd be found relatively quickly by the next locker.
>> Debugging it would be a beachball though.
> 
> I think it would make sense to have a virObjectRWUnlock too.
> 
> Both of these changes would make it clearer which bit of code is
> using a plain lockable object, vs a rwlockable object.
> 

That's fine - I'll add that to my patches before sending...

>> FWIW: As noted in my responses to the RWLock series, consider if
>> virObjectLock(obj) is called with an invalid @obj, then we really don't
>> get the lock. All that's done is a VIR_WARN() and return. So if someone
>> passes the wrong thing we have all sorts of problems. That's been true
>> of virObjectLock for a long time, but we have (ahem) well behaved code
>> so less of a problem.
> 
> As above, trying to detect these errors & carry on & cleanup is
> just giving a false sense of security. I think this is probably a
> case where it is reasonable to abort() immediately, because if you
> are in this messed up state, the chances are that the code is going
> to abort due to memory corrupton shortly anyway.
> 
> Regards,
> Daniel
> 

Well I can propose the abort() on error if so desired. I agree w/r/t
some awful things that could happen...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list