[libvirt] [PATCH v2 0/8] Some virObjectRW* adjustments

John Ferlan posted 8 patches 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170801000508.14341-1-jferlan@redhat.com
src/conf/virdomainobjlist.c |  62 ++++++++--------
src/libvirt_private.syms    |   4 +-
src/util/virobject.c        | 169 +++++++++++++++++++++++++++++++++-----------
src/util/virobject.h        |  10 ++-
4 files changed, 169 insertions(+), 76 deletions(-)
[libvirt] [PATCH v2 0/8] Some virObjectRW* adjustments
Posted by John Ferlan 6 years, 8 months ago
v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html

Differences from v1:

 * Use the names virObjectRWLockRead, virObjectRWLockWrite and
   virObjectRWUnlock

 * Instead of an 'int' return, make the virObjectRWLock{Read|Write} be
   void returns just like virObject{Lock|Unlock}

 * Separate out the magic number check for the virObjectIsClass consumers
   into its own patch and describe the reasons for usage.

 * Apply the same magic number check to virObject{Ref|Unref} separately.

BTW: This looks and works eally nice with what I have for common objects...

John Ferlan (8):
  util: Rename virObjectLockRead to virObjectRWLockRead
  util: Introduce and use virObjectRWLockWrite
  util: Only have virObjectLock handle virObjectLockable
  util: Introduce virObjectGetRWLockableObj
  util: Introduce and use virObjectRWUnlock
  util: Create common error path for invalid object
  util: Add magic number check for object validity
  util: Add object checking for virObject{Ref|Unref}

 src/conf/virdomainobjlist.c |  62 ++++++++--------
 src/libvirt_private.syms    |   4 +-
 src/util/virobject.c        | 169 +++++++++++++++++++++++++++++++++-----------
 src/util/virobject.h        |  10 ++-
 4 files changed, 169 insertions(+), 76 deletions(-)

-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/8] Some virObjectRW* adjustments
Posted by Michal Privoznik 6 years, 8 months ago
On 08/01/2017 02:05 AM, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html
> 
> Differences from v1:
> 
>  * Use the names virObjectRWLockRead, virObjectRWLockWrite and
>    virObjectRWUnlock
> 
>  * Instead of an 'int' return, make the virObjectRWLock{Read|Write} be
>    void returns just like virObject{Lock|Unlock}
> 
>  * Separate out the magic number check for the virObjectIsClass consumers
>    into its own patch and describe the reasons for usage.
> 
>  * Apply the same magic number check to virObject{Ref|Unref} separately.
> 
> BTW: This looks and works eally nice with what I have for common objects...
> 
> John Ferlan (8):
>   util: Rename virObjectLockRead to virObjectRWLockRead
>   util: Introduce and use virObjectRWLockWrite
>   util: Only have virObjectLock handle virObjectLockable
>   util: Introduce virObjectGetRWLockableObj
>   util: Introduce and use virObjectRWUnlock
>   util: Create common error path for invalid object
>   util: Add magic number check for object validity
>   util: Add object checking for virObject{Ref|Unref}
> 
>  src/conf/virdomainobjlist.c |  62 ++++++++--------
>  src/libvirt_private.syms    |   4 +-
>  src/util/virobject.c        | 169 +++++++++++++++++++++++++++++++++-----------
>  src/util/virobject.h        |  10 ++-
>  4 files changed, 169 insertions(+), 76 deletions(-)
> 

Okay, I've ran some local tests and indeed no tool showed any
misbehaviour when my test binary was mutex-locking an RW lock or
rwlocking a mutex. While I still believe that us, reviewers have to be
careful around locks anyway, rename to virObjectRWLock* can help us
remember if we forget.

ACK series.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/8] Some virObjectRW* adjustments
Posted by John Ferlan 6 years, 8 months ago

On 08/03/2017 11:34 AM, Michal Privoznik wrote:
> On 08/01/2017 02:05 AM, John Ferlan wrote:
>> v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html
>>
>> Differences from v1:
>>
>>  * Use the names virObjectRWLockRead, virObjectRWLockWrite and
>>    virObjectRWUnlock
>>
>>  * Instead of an 'int' return, make the virObjectRWLock{Read|Write} be
>>    void returns just like virObject{Lock|Unlock}
>>
>>  * Separate out the magic number check for the virObjectIsClass consumers
>>    into its own patch and describe the reasons for usage.
>>
>>  * Apply the same magic number check to virObject{Ref|Unref} separately.
>>
>> BTW: This looks and works eally nice with what I have for common objects...
>>
>> John Ferlan (8):
>>   util: Rename virObjectLockRead to virObjectRWLockRead
>>   util: Introduce and use virObjectRWLockWrite
>>   util: Only have virObjectLock handle virObjectLockable
>>   util: Introduce virObjectGetRWLockableObj
>>   util: Introduce and use virObjectRWUnlock
>>   util: Create common error path for invalid object
>>   util: Add magic number check for object validity
>>   util: Add object checking for virObject{Ref|Unref}
>>
>>  src/conf/virdomainobjlist.c |  62 ++++++++--------
>>  src/libvirt_private.syms    |   4 +-
>>  src/util/virobject.c        | 169 +++++++++++++++++++++++++++++++++-----------
>>  src/util/virobject.h        |  10 ++-
>>  4 files changed, 169 insertions(+), 76 deletions(-)
>>
> 
> Okay, I've ran some local tests and indeed no tool showed any
> misbehaviour when my test binary was mutex-locking an RW lock or
> rwlocking a mutex. While I still believe that us, reviewers have to be
> careful around locks anyway, rename to virObjectRWLock* can help us
> remember if we forget.
> 
> ACK series.
> 
> Michal
> 

Thanks - I'll hold off on pushing though... I'm fine with using
VIR_ERROR instead of VIR_WARN too...

In any case, I'm away next week and wouldn't want to leave that kind of
present for anyone to revert!  Besides, I'm assuming there may be other
opinions and there isn't a "rush" to get this in as nothing is broken yet...

John

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