[libvirt] [PATCH 13/17] nwfilter: Introduce virNWFilterObjListFindInstantiateFilter

[Michal Privoznik]

On 06/02/2017 12:25 PM, John Ferlan wrote:
> Create a common API to handle the instantiation path filter lookup.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/virnwfilterobj.c              |  23 +++++++
>  src/conf/virnwfilterobj.h              |   4 ++
>  src/libvirt_private.syms               |   1 +
>  src/nwfilter/nwfilter_gentech_driver.c | 119 ++++++++++++---------------------
>  4 files changed, 70 insertions(+), 77 deletions(-)
> 
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index c86b1a9..b21b570 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -195,6 +195,29 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
>  }
>  
>  
> +virNWFilterObjPtr
> +virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
> +                                        const char *filtername)
> +{
> +    virNWFilterObjPtr obj;
> +
> +    if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("referenced filter '%s' is missing"), filtername);
> +        return NULL;
> +    }
> +
> +    if (virNWFilterObjWantRemoved(obj)) {
> +        virReportError(VIR_ERR_NO_NWFILTER,
> +                       _("Filter '%s' is in use."), filtername);
> +        virNWFilterObjUnlock(obj);
> +        return NULL;
> +    }
> +
> +    return obj;
> +}
> +
> +
>  static int
>  _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
>                                   virNWFilterDefPtr def,
> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
> index e4dadda..85c8ead 100644
> --- a/src/conf/virnwfilterobj.h
> +++ b/src/conf/virnwfilterobj.h
> @@ -69,6 +69,10 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
>                               const char *name);
>  
>  virNWFilterObjPtr
> +virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
> +                                        const char *filtername);
> +
> +virNWFilterObjPtr
>  virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>                              virNWFilterDefPtr def,
>                              const char *configDir);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cda5f92..ee19cb9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -971,6 +971,7 @@ virNWFilterObjListAssignDef;
>  virNWFilterObjListExport;
>  virNWFilterObjListFindByName;
>  virNWFilterObjListFindByUUID;
> +virNWFilterObjListFindInstantiateFilter;
>  virNWFilterObjListFree;
>  virNWFilterObjListGetNames;
>  virNWFilterObjListLoadAllConfigs;
> diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
> index 5798371..68a2ddb 100644
> --- a/src/nwfilter/nwfilter_gentech_driver.c
> +++ b/src/nwfilter/nwfilter_gentech_driver.c
> @@ -60,11 +60,11 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = {
>   * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate
>   * will hold a lock on a virNWFilterObjPtr. This in turn invokes
>   * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec
> - * which invokes virNWFilterObjListFindByName. This iterates over every single
> - * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a
> - * filter in parallel, they'll both hold 1 lock at the top level in
> - * virNWFilterInstantiateFilterUpdate which will cause the other thread
> - * to deadlock in virNWFilterObjListFindByName.
> + * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over
> + * every single virNWFilterObjPtr in the list. So if 2 threads try to
> + * instantiate a filter in parallel, they'll both hold 1 lock at the top level
> + * in virNWFilterInstantiateFilterUpdate which will cause the other thread
> + * to deadlock in virNWFilterObjListFindInstantiateFilter.
>   *
>   * XXX better long term solution is to make virNWFilterObjList use a
>   * hash table as is done for virDomainObjList. You can then get
> @@ -383,20 +383,9 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver,
>      int ret = -1;
>  
>      VIR_DEBUG("Instantiating filter %s", inc->filterref);
> -    obj = virNWFilterObjListFindByName(driver->nwfilters,
> -                                       inc->filterref);
> -    if (!obj) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("referenced filter '%s' is missing"),
> -                       inc->filterref);
> -        goto cleanup;
> -    }
> -    if (virNWFilterObjWantRemoved(obj)) {
> -        virReportError(VIR_ERR_NO_NWFILTER,
> -                       _("Filter '%s' is in use."),
> -                       inc->filterref);
> +    if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters,
> +                                                        inc->filterref)))
>          goto cleanup;
> -    }
>  
>      /* create a temporary hashmap for depth-first tree traversal */
>      if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params,
> @@ -545,58 +534,46 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
>                  break;
>          } else if (inc) {
>              VIR_DEBUG("Following filter %s", inc->filterref);
> -            obj = virNWFilterObjListFindByName(driver->nwfilters, inc->filterref);
> -            if (obj) {
> -
> -                if (virNWFilterObjWantRemoved(obj)) {
> -                    virReportError(VIR_ERR_NO_NWFILTER,
> -                                   _("Filter '%s' is in use."),
> -                                   inc->filterref);
> -                    rc = -1;
> -                    virNWFilterObjUnlock(obj);
> -                    break;
> -                }
> +            if (!(obj =
> +                  virNWFilterObjListFindInstantiateFilter(driver->nwfilters,
> +                                                          inc->filterref))) {

No, please move the function call onto the same line as if(!(obj =.
83 chars long line a not EOW (as we know it)

> +                rc = -1;
> +                break;
> +            }
>  
> -                /* create a temporary hashmap for depth-first tree traversal */
> -                virNWFilterHashTablePtr tmpvars =
> -                                      virNWFilterCreateVarsFrom(inc->params,
> -                                                                vars);

Nor 84 chars long line.

> -                if (!tmpvars) {
> -                    rc = -1;
> -                    virNWFilterObjUnlock(obj);
> -                    break;
> -                }

ACK if you fix it.

Michal