Since the class it represents is based on virObjectRWLockableClass
and in order to make sure we diffentiate lest anyone somehow
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}.

Signed-off-by: John Ferlan <jferlan@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);
     obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
     if (ref) {
         virObjectRef(obj);
@@ -160,7 +160,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     virDomainObjPtr obj;
 
-    virObjectLockRead(doms);
+    virObjectRWLockRead(doms);
     virUUIDFormat(uuid, uuidstr);
 
     obj = virHashLookup(doms->objs, uuidstr);
@@ -204,7 +204,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
 {
     virDomainObjPtr obj;
 
-    virObjectLockRead(doms);
+    virObjectRWLockRead(doms);
     obj = virHashLookup(doms->objsName, name);
     virObjectRef(obj);
     virObjectUnlock(doms);
@@ -653,7 +653,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
                              virConnectPtr conn)
 {
     struct virDomainObjListData data = { filter, conn, active, 0 };
-    virObjectLockRead(doms);
+    virObjectRWLockRead(doms);
     virHashForEach(doms->objs, virDomainObjListCount, &data);
     virObjectUnlock(doms);
     return data.count;
@@ -697,7 +697,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
 {
     struct virDomainIDData data = { filter, conn,
                                     0, maxids, ids };
-    virObjectLockRead(doms);
+    virObjectRWLockRead(doms);
     virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
     virObjectUnlock(doms);
     return data.numids;
@@ -751,7 +751,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
     struct virDomainNameData data = { filter, conn,
                                       0, 0, maxnames, names };
     size_t i;
-    virObjectLockRead(doms);
+    virObjectRWLockRead(doms);
     virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
     virObjectUnlock(doms);
     if (data.oom) {
@@ -792,7 +792,7 @@ virDomainObjListForEach(virDomainObjListPtr doms,
     struct virDomainListIterData data = {
         callback, opaque, 0,
     };
-    virObjectLockRead(doms);
+    virObjectRWLockRead(doms);
     virHashForEach(doms->objs, virDomainObjListHelper, &data);
     virObjectUnlock(doms);
     return data.ret;
@@ -925,7 +925,7 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
 {
     struct virDomainListData data = { NULL, 0 };
 
-    virObjectLockRead(domlist);
+    virObjectRWLockRead(domlist);
     sa_assert(domlist->objs);
     if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
         virObjectUnlock(domlist);
@@ -962,7 +962,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
     *nvms = 0;
     *vms = NULL;
 
-    virObjectLockRead(domlist);
+    virObjectRWLockRead(domlist);
     for (i = 0; i < ndoms; i++) {
         virDomainPtr dom = doms[i];
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 37b815c..99302d2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2305,10 +2305,10 @@ virObjectListFree;
 virObjectListFreeCount;
 virObjectLock;
 virObjectLockableNew;
-virObjectLockRead;
 virObjectNew;
 virObjectRef;
 virObjectRWLockableNew;
+virObjectRWLockRead;
 virObjectUnlock;
 virObjectUnref;
 
diff --git a/src/util/virobject.c b/src/util/virobject.c
index b1bb378..b97f251 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -399,7 +399,7 @@ virObjectLock(void *anyobj)
 
 
 /**
- * virObjectLockRead:
+ * virObjectRWLockRead:
  * @anyobj: any instance of virObjectRWLockable
  *
  * Acquire a read lock on @anyobj. The lock must be
@@ -409,9 +409,14 @@ virObjectLock(void *anyobj)
  * on the object before locking it (eg virObjectRef).
  * The object must be unlocked before releasing this
  * reference.
+ *
+ * NB: It's possible to return without the lock if
+ *     @anyobj was invalid - this has been considered
+ *     a programming error rather than something that
+ *     should be checked.
  */
 void
-virObjectLockRead(void *anyobj)
+virObjectRWLockRead(void *anyobj)
 {
     if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
         virObjectRWLockablePtr obj = anyobj;
@@ -429,7 +434,7 @@ virObjectLockRead(void *anyobj)
  * @anyobj: any instance of virObjectLockable or virObjectRWLockable
  *
  * Release a lock on @anyobj. The lock must have been acquired by
- * virObjectLock or virObjectLockRead.
+ * virObjectLock or virObjectRWLockRead.
  */
 void
 virObjectUnlock(void *anyobj)
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 5985fad..e7ca983 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -125,7 +125,7 @@ virObjectLock(void *lockableobj)
     ATTRIBUTE_NONNULL(1);
 
 void
-virObjectLockRead(void *lockableobj)
+virObjectRWLockRead(void *lockableobj)
     ATTRIBUTE_NONNULL(1);
 
 void
-- 
2.9.4

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

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

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

> 
> Signed-off-by: John Ferlan <jferlan@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

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

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@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).

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

On 08/01/2017 05:36 PM, John Ferlan wrote:
> 
> 
> 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.

Ah, okay. I'm no native speaker. Learned something new :-)

> 
>>> 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@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.

Well, true. But aren't we trying to fix something that is no issue? I
mean, have somebody ran into such issue? But truth is I no longer care
that much.

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

You should, esp. with such long function names :-)

> 
> 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).

I don't think that we want to replace all mutexes with rwlocks. Firstly,
rwlocks come with some overhead over mutexes, secondly there are some
places where we do writes in all critical sections. For instance I don't
think that event loop's mutex is a candidate for rwlock. On the other
hand, it's not a virObject...

Michal

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