[libvirt] [PATCH v2 1/8] util: Rename virObjectLockRead to virObjectRWLockRead

[John Ferlan]

On 08/01/2017 09:24 AM, Michal Privoznik wrote:
> On 08/01/2017 02:05 AM, John Ferlan wrote:
>> Since the class it represents is based on virObjectRWLockableClass
>> and in order to make sure we diffentiate lest anyone somehow
> 
> ^^ couple of typos
> 

Just differentiate from my dictionary.

'lest' is someone colloquial - I can alter it though.

>> believes they could use virObjectLockRead for a virObjectLockableClass,
>> let's rename the API to use the RW in the name. Besides the RW locks
>> refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the
>> other locks refer to pthread_mutex_{init|lock|unlock|destroy}.
> 
> Firstly, this is just an internal implementation. We often rename APIs
> for us to use. Secondly, this is because pthreads are C API with no
> 'object' reference. So they have to have two unlock functions for two
> different objects.
> 

Well function naming guidelines weren't in place when virObjectLock (and
Unlock) were added, but they are now.

>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/virdomainobjlist.c | 18 +++++++++---------
>>  src/libvirt_private.syms    |  2 +-
>>  src/util/virobject.c        | 11 ++++++++---
>>  src/util/virobject.h        |  2 +-
>>  4 files changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>> index 1d027a4..9bc6603 100644
>> --- a/src/conf/virdomainobjlist.c
>> +++ b/src/conf/virdomainobjlist.c
>> @@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
>>                                   bool ref)
>>  {
>>      virDomainObjPtr obj;
>> -    virObjectLockRead(doms);
>> +    virObjectRWLockRead(doms);
> 
> If we are going to do this (which I'm no fan of), then we should also
> turn virObjectLock() into virObjectLockableLock(). For the consistency
> sake. Moreover, as I stated in discussion to v1 (not sure if you've read
> it before sending this series), I quite like that I'm able to type
> virObjectLock, hit shortcut for bringing up completion list and chose
> 'virObjectLock', 'virObjectLockRead', (or even) 'virObjectLockWrite'.
> With this patch I'm no longer able to do that so easily.
> 
> Michal
> 

I read the response and the others... I'm torn between RWLockRead and
just LockRead. I really don't care either way. I went with RW for the
stated reason though - fear that someone like you sees virObjectLockRead
(or worse virObjectLockWrite) and believes that is what they want.

If they are not operating on an RWLockableObject, then they really don't
get the lock and because we've decided to not error out in that case,
they don't have the safety they thought they had.

Maybe I'm wrong, but I have to present that argument.

As for altering virObjectLock to virObjectLockableLock - that ship
sailed long ago. I would say it would be virObjectMutexLock though to be
more precise, but using virObjectLock as a shorthand since there was
only one locking subsystem available is understandable. Time has brought
forth some new options and now we have to adjust the new code to fit the
more recent models. The old code could be adjusted, but there's far too
many places that need change and no one wants that insanely impossible task.

I suppose I could also see just reason to go with virObjectLockRWRdLock
and virObjectLockRWWrLock (and virObjectUnlockRW).

I haven't trained my editor to choose API names for me.

Not sure there's a perfect solution for this - perhaps multiple opinions
though. I suppose all that really matters is we decide either:

1. Leave things as they are - completely
2. Alter the naming scheme in some way

I can live with #1 even though I'm concerned about mis-use. Also, I
thought using overloaded functions was something that long ago was
decided to be less desirable. That is the Lock and Unlock operate on two
different object types based on something stored in the object instead
of two separate API's. The call is to two very different lower level
API's as well that cannot be used interchangeably.

While IIUC the goal would be to some day change all virObjectLock()'s to
be either LockRead or LockWrite and do away with virObjectLock and any
sort of reference to virMutexLock's and that's a noble goal. However, I
would also think it could be awhile before that's realizable. So yes,
it's a conundrum and I can be talked into dropping this series. Although
I do still see value in the "magic number check" prior to using a non
NULL @obj (a/k/a @anyobj).

John

FWIW: As it relates to common object - since I've chosen to use a
LockableLock and RWLockable as "parent"'s to my new object children,
those conditions that check virObjectIsClass(anyobj,... get a bit more
ugly looking or are replaced by something that can check the parent or
the parent's parent (letting someone else figure out 3 levels of a
family tree).