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

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(a)redhat.com&gt; >> --- >> 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).