[libvirt] [PATCH v2 3/4] Add a mutex to serialize updates to firewall

Stefan Berger stefanb at linux.vnet.ibm.com
Mon Jan 27 21:50:56 UTC 2014

On 01/27/2014 12:18 PM, Daniel P. Berrange wrote:
> The nwfilter conf update mutex previously serialized
> updates to the internal data structures for firewall
> rules, and updates to the firewall itself. Since the

Hm, wasn't aware (anymore) of this double-purpose.

I also hadn't looked at this patch in the first round...

> former is going to be turned into a read/write lock
> instead of a mutex, a new lock is required to serialize
> access to the firewall itself.
> With this new lock, the lock ordering rules will be
> for virNWFilter{Define,Undefine}
>        1. nwfilter driver lock
>        2. nwfilter update lock

Insert: 3. nwfilter callback drivers lock

This is then used in this order also by nwfilterStateReload

>        3. virt driver lock
>        4. domain object lock
>        5. gentech driver lock
> and VM start
>        1. nwfilter update lock
>        2. virt driver lock
>        3. domain object lock
>        4. gentech driver lock
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>   src/nwfilter/nwfilter_driver.c         |  4 +++-
>   src/nwfilter/nwfilter_gentech_driver.c | 19 +++++++++++++++++--
>   src/nwfilter/nwfilter_gentech_driver.h |  2 +-
>   3 files changed, 21 insertions(+), 4 deletions(-)
> diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
> index 89913cf8..d500963 100644
> --- a/src/nwfilter/nwfilter_gentech_driver.c
> +++ b/src/nwfilter/nwfilter_gentech_driver.c
> @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
>       int rc;
>       virNWFilterLockFilterUpdates();
> +    virMutexLock(&updateMutex);

Since the filter updates lock had the two purposes before, you are now 
introducing a separate lock to assign a purpose to each lock.
Further below you are preventing concurrent teardowns to this here.

I am wondering how much further down this lock here could actually be 
pushed. This and the other function (virNWFilterInstantiateFilterLate) 
where you  place this lock are calling __virNWFilterInstantiateFilter 
and nothing else calls that function [and the filter read protection 
above the lock call will remain]. So I think this lock could be placed 
inside __virNWFilterInstantiateFilter(). Also looking at that function I 
am not sure whether there is anything worth protecting using this newly 
introduced lock then. It ends up calling virNWFilterInstantiate(). Here 
I would be a bit careful with the threads being started to learn the IP 
addresses. So maybe this function could be the place where to serialize 
access. What's your take?


More information about the libvir-list mailing list