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

[Stefan Berger]

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?


    Stefan