[libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable

[Michal Privoznik]

On 07/25/2017 02:13 PM, John Ferlan wrote:
> 
> 
> On 07/25/2017 03:45 AM, Michal Privoznik wrote:
>> On 07/24/2017 05:22 PM, John Ferlan wrote:
>>> [...]
>>>
>>>>  /**
>>>>   * virObjectLock:
>>>> - * @anyobj: any instance of virObjectLockablePtr
>>>> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>>>>   *
>>>> - * Acquire a lock on @anyobj. The lock must be
>>>> - * released by virObjectUnlock.
>>>> + * Acquire a lock on @anyobj. The lock must be released by
>>>> + * virObjectUnlock. In case the passed object is instance of
>>>> + * virObjectRWLockable a write lock is acquired.
>>>>   *
>>>>   * The caller is expected to have acquired a reference
>>>>   * on the object before locking it (eg virObjectRef).
>>>> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
>>>>  void
>>>>  virObjectLock(void *anyobj)
>>>>  {
>>>> -    virObjectLockablePtr obj = virObjectGetLockableObj(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)");
>>>> +    }
>>>> +}
>>>>  
>>>> -    if (!obj)
>>>> -        return;
>>>>  
>>>> -    virMutexLock(&obj->lock);
>>>> +/**
>>>> + * virObjectLockRead:
>>>> + * @anyobj: any instance of virObjectRWLockablePtr
>>>> + *
>>>> + * Acquire a read lock on @anyobj. The lock must be
>>>> + * released by virObjectUnlock.
>>>> + *
>>>> + * The caller is expected to have acquired a reference
>>>> + * on the object before locking it (eg virObjectRef).
>>>> + * The object must be unlocked before releasing this
>>>> + * reference.
>>>> + */
>>>> +void
>>>> +virObjectLockRead(void *anyobj)
>>>> +{
>>>> +    if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>>>> +        virObjectRWLockablePtr obj = anyobj;
>>>> +        virRWLockRead(&obj->lock);
>>>> +    } else {
>>>> +        virObjectPtr obj = anyobj;
>>>> +        VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
>>>> +                 anyobj, obj ? obj->klass->name : "(unknown)");
>>>> +    }
>>>>  }
>>>>  
>>>
>>> Hopefully no one "confuses" which object is which or no one starts using
>>> virObjectLockRead for a virObjectLockable and expects that read lock to
>>> be in place (or error out) or gasp actually wait for a write lock to
>>> release the lock as this does not error out.
>>
>> This could have already happened if one would pass bare virObject to
>> virObjectLock().
>>
> 
> Consider passing a non virObject (such as what happened during an early
> change to the code for me - a virHashTablePtr) to virObjectLock...

Well, that's what happens in C to functions accepting void pointer. In
this sense yes, we are not true OOP - compiler would catch that you're
calling a method that is not defined for the class. But since we're
implementing OOP in runtime, we are able to catch OOP related problem
only at runtime. I'm not sure it is something we can check for at
compile time. And I think other C libraries that re-implement OOP (like
glib) do just the same as us.

> 
> In any case, we'll have to be more vigilant now to know that only
> ObjList @obj's for virdomainobjlist can use this new API.

Why? Maybe I'm misunderstanding what you're saying, but I think that
network driver, and basically any other driver which uses vir*ObjList
can use it. And not just lists. Other objects too - e.g. qemuCaps,
virDomainCaps, etc (I really roughly skimmed through users of
virObjectLockable).

And @obj's are untouched with this change, aren't they? I feel like
we're not on the same page here. My change just allowed two threads to
look up domains at the same time. It doesn't affect domains in any way.

> Longer term
> I'll adjust to use them.
> 
>>>
>>> This is a danger in the long term assumption of having a void function
>>> that has callers that can make the assumption that upon return the Lock
>>> really was taken.
>>
>> Well, I agree that this is one more thing to be cautious about, but no
>> more than making sure object passed to virObjectLock() is virObjectLockable.
>>
> 
> There are some ugly ways to handle this including using some kind of
> macro for virObjectLock that would force an abort of some sort. We have
> been "fortunate" to have well behaved and reviewed code, but with two
> lock API's now it's just one of those things to keep track of for reviews.

I don't find this change that frightening, but I agree that bare
VIR_WARN() we have might not be enough. There are plenty ways where
malicious pointers can be passed to virObject* APIs, and especially
virObjectLock(). Worst case scenario we might pass some "random" address
in memory, make the caller think the object is locked while in fact it
is not. We are strongly against abort(), but maybe we can revisit that
decision in this case because it can lead to data loss/corruption. This
error is different to OOM error or 'unable to start a domain' one.

> 
>>>
>>> Perhaps the better way to attack this would have been to create a
>>> virObjectLockWrite() function called only from vir*ObjListAdd and
>>> vir*ObjListRemove leaving the other virObjectLock()'s in tact and having
>>> the virObjectLock() code make the decision over whether to use the
>>> virRWLockRead or virMutexLock depending on the object class.
>>
>> What's the difference? If virObjectLock() does what you're suggesting
>> (what indeed is implemented too), what's the point in having
>> virObjectLockWrite()?
>>
>> Michal
>>
> 
> 1. Less code and a lock that could check status... I understand not
> wanting to modify 10 callers to check status, but modifying two and thus
> making it clearer to the caller that these are "different" might not be
> a bad thing.

Hold on. If we are going to make virObjectLock() return an error - why
do it just for virObjectRWLockable? Doing the following is no less
worse:

char a[] = "Some random string";
virObjectLock(a);

> 
> 2. The "default" action being more consumers probably will use the Read
> locks (as I believe is the premise/impetus of the series). The
> Add/Remove would seemingly be used less and thus the exception. So why
> not have the default for ObjList/virObjectRWLockableClass be to have the
> virObjectLock use a virRWLockRead instead of virRWLockWrite?

I beg to disagree. Whenever I see 'virObjectLock' I am sure that no
other thread is changing the object that is guarded by the mutex. If,
however, virObjectLock was grabbing read lock, it would be very
confusing and I would have to check every time whether the object I'm
passing is virObjectLockable or virObjectRWLockable because the
virObjectLock() function would behave differently depending on class of
the object.

Moreover, now I can do the following and the code still works:

diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c
index ccde72e72..4fe13fc40 100644
--- i/src/conf/virnetworkobj.c
+++ w/src/conf/virnetworkobj.c
@@ -60 +60 @@ virNetworkObjOnceInit(void)
-    if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(),
+    if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(),
diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h
index 8090c2e24..ee4a939f2 100644
--- i/src/conf/virnetworkobj.h
+++ w/src/conf/virnetworkobj.h
@@ -30 +30 @@ struct _virNetworkObj {
-    virObjectLockable parent;
+    virObjectRWLockable parent;


With your suggestion, I'd have to change all virObjectLock() in
src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash
table, not network object itself).

Michal