Re: [libvirt] [PATCH 3/5] Push nwfilter update locking up to top level

On 01/23/2014 07:37 AM, Daniel P. Berrange wrote: Makes sense... there are other paths as well: - SIGHUP or restart of libvirt that recreates all filters - late instantiation of filters following detection of VM's IP address

src/conf/nwfilter_conf.c::virNWFilterObjLoad : I don't see this function grabbing the lock. I think this is missing.

>diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c >index 89913cf8..f0e78ed 100644 >--- a/src/nwfilter/nwfilter_gentech_driver.c >+++ b/src/nwfilter/nwfilter_gentech_driver.c >@@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, > int ifindex; > int rc; > >- virNWFilterLockFilterUpdates(); > > /* after grabbing the filter update lock check for the interface; if > it's not there anymore its filters will be or are being removed >@@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, > foundNewFilter); > > cleanup: >- virNWFilterUnlockFilterUpdates(); > > return rc; > } This function is called by virNWFilterInstantiateFilter and virNWFilterUpdateInstantiateFilter. So, in the former case this is called when a VM is started, in the latter case if VMs' filters are updated while the VM is running. I think you are covering the VM creation case with the above calls for lxc and further below with the changes to the qemu driver and the uml driver. There is at least one other part that needs to be covered: src/conf/nwfilter_conf.c::virNWFilterInstFiltersOnAllVMs :: kicked off by nwfilterStateReload upon SIGHUP. We need a lock there now. src/conf/nwfilter_conf.c::virNWFilterTriggerVMFilterRebuild:: - called by virNWFilterTestUnassignDef: - called by src/nwfilter/nwfilter/nwfilterUndefine:: You seem to just reorder some locks there; now we need a (writer) lock there - called by virNWFilterObjAssignDef: which must be called with lock called following above reasoning

>@@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, > int rc; > bool foundNewFilter = false; > >- virNWFilterLockFilterUpdates(); >- > rc = __virNWFilterInstantiateFilter(driver, > vmuuid, > true, >@@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, > } > } > >- virNWFilterUnlockFilterUpdates(); >- > return rc; > } This function here is called by src/nwfilter/nwfilter_learnip.c::learnIPAddressThread src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule They instantiate the filters once a VM's IP address has been detected. So this is where the *Late() comes from. If you remove the locking from here, you have to lock it there. Considering what you do layer, I would keep the lock here and convert this into a reader lock layer on.