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