[libvirt] [PATCH v5 2/3] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

[Michal Privoznik]

On 02/06/2018 08:20 PM, John Ferlan wrote:
> Implement the self locking object list for nwfilter object lists
> that uses two hash tables to store the nwfilter object by UUID or
> by Name.
> 
> As part of this alter the uuid argument to virNWFilterObjLookupByUUID
> to expect an already formatted uuidstr.
> 
> Alter the existing list traversal code to implement the hash table
> find/lookup/search functionality.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/virnwfilterobj.c      | 405 ++++++++++++++++++++++++++++-------------
>  src/conf/virnwfilterobj.h      |   2 +-
>  src/nwfilter/nwfilter_driver.c |   5 +-
>  3 files changed, 283 insertions(+), 129 deletions(-)


> @@ -425,24 +483,26 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>          return NULL;
>      }
>  
> -
> -    /* Get a READ lock and immediately promote to WRITE while we adjust
> -     * data within. */
> -    if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
> +    /* We're about to make some changes to objects on the list - so get the
> +     * list READ lock in order to Find the object and WRITE lock the object
> +     * since both paths would immediately promote it anyway */
> +    virObjectRWLockRead(nwfilters);
> +    if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) {
> +        virObjectRWLockWrite(obj);
> +        virObjectRWUnlock(nwfilters);
>  
>          objdef = obj->def;
>          if (virNWFilterDefEqual(def, objdef)) {
> -            virNWFilterObjPromoteToWrite(obj);
>              virNWFilterDefFree(objdef);
>              obj->def = def;
>              virNWFilterObjDemoteFromWrite(obj);
>              return obj;
>          }
>  
> -        /* Set newDef and run the trigger with a demoted lock since it may need
> -         * to grab a read lock on this object and promote before returning. */
> -        virNWFilterObjPromoteToWrite(obj);
>          obj->newDef = def;
> +
> +        /* Demote while the trigger runs since it may need to grab a read
> +         * lock on this object and promote before returning. */
>          virNWFilterObjDemoteFromWrite(obj);
>  
>          /* trigger the update on VMs referencing the filter */
> @@ -462,39 +522,113 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>          return obj;
>      }
>  
> +    /* Promote the nwfilters to add a new object */
> +    virObjectRWUnlock(nwfilters);
> +    virObjectRWLockWrite(nwfilters);

How can this work? What if there's another thread waiting for the write
lock (e.g. just to define an NWFilter)? The Unlock(nwfilters) wakes up
the other thread, it does its job, unlocks the list waking us in turn.
So we lock @nwfilters for writing and add something into the hash table
- without all the checks (e.g. duplicity check) done earlier.

Michal