[libvirt] [PATCH 0/7] Some virObjectRW* adjustments

John Ferlan jferlan at redhat.com
Fri Jul 28 16:38:54 UTC 2017


As indicated in a couple of recent reviews, the new virObjectRWLockable
object and API's I believe needed a few adjustments in order to make
them better. So I present those thoughts in patch format to be hopefully
dicussed and dissected.

Essentially this series, will modify the newly added virObjectLockRead
to return a status and to have that status managed. Since there were only
9 new callers, this was straightforward.  None of the callers actually
needed any sort of "divert logic" in order to jump around code that
shouldn't do the Unlock now.  Although that could be a problem if ever
changing other more deeply nested type locks. That's someone else's
headache though.

Next, introduce and use virObjectLockWrite to be only for the new
style RWLocks. This too will return status. Only the vir*Remove caller
has a code path that I'm not happy with, but since these type locks
have been processing along anyway without technically ensuring they
have the lock before doing so, well no harm, no foul.

Next, introduce and use virObjectRWUnlock. Rather than overloading the
virObjectUnlock, let's just keep each caller doing it's one thing to
be sure that the calling code is doing the "right thing". In this case,
returning a failure status is just not going to work. We've gone too
far down the path in order for a failure to cause some failure in the
caller. We'll just treat it like virObjectUnlock and if the resource
remains locked, well we'll find out relatively soon as the next time
someone goes to read/write the list they'll be waiting for an unlock
that will never happen because of some coding mistake. But that will
save us from perhaps other dire consequences involving corruption.

Finally, revert the virObjectLock and virObjectUnlock code to just
manage virObjectLockable locks. No worse than what happened before
the RWLock was introduced. Leaving the consumers to handle things
as they would before - which probably ends up in some sort of awful
state leading to memory corruption and a quick death as opposed to
abort()'g in the void() function which has been deemed a don't do
that type operation for libvirt.

The last 2 patches actually have appeared before in various common
object series postings, so they could be a v2 or 3 type logic, but
I see them more related to this than that, so I present them here
again. 

John Ferlan (7):
  util: Alter virObjectLockRead to return status
  util: Introduce and use virObjectLockWrite
  util: Only have virObjectLock handle virObjectLockable
  util: Introduce virObjectGetRWLockableObj
  util: Introduce and use virObjectRWUnlock
  util: Create common error path for invalid object
  util: Add safety net of checks to ensure valid object

 src/conf/virdomainobjlist.c |  81 +++++++++++++---------
 src/libvirt_private.syms    |   2 +
 src/util/virobject.c        | 160 +++++++++++++++++++++++++++++++++-----------
 src/util/virobject.h        |  10 ++-
 4 files changed, 181 insertions(+), 72 deletions(-)

-- 
2.9.4




More information about the libvir-list mailing list