Now that the virNWFilterBinding APIs are using the nwfilter
update lock directly, there is no need for the virt drivers
to do it themselves.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/lxc/lxc_driver.c | 6 ------
src/qemu/qemu_driver.c | 18 ------------------
src/qemu/qemu_migration.c | 3 ---
src/qemu/qemu_process.c | 4 ----
4 files changed, 31 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 020ec257ae..4185a61267 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -971,8 +971,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
- virNWFilterReadLockFilterUpdates();
-
if (!(vm = lxcDomObjFromDomain(dom)))
goto cleanup;
@@ -1014,7 +1012,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
cleanup:
virDomainObjEndAPI(&vm);
virObjectEventStateQueue(driver->domainEventState, event);
- virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -1080,8 +1077,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
if (flags & VIR_DOMAIN_START_VALIDATE)
parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
- virNWFilterReadLockFilterUpdates();
-
if (!(caps = virLXCDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -1138,7 +1133,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
cleanup:
virDomainObjEndAPI(&vm);
virObjectEventStateQueue(driver->domainEventState, event);
- virNWFilterUnlockFilterUpdates();
return dom;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b74b0375a7..e4e1cd7bdf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1606,8 +1606,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (flags & VIR_DOMAIN_START_RESET_NVRAM)
start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM;
- virNWFilterReadLockFilterUpdates();
-
if (!(def = virDomainDefParseString(xml, driver->xmlopt,
NULL, parse_flags)))
goto cleanup;
@@ -1661,7 +1659,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
virDomainObjEndAPI(&vm);
virObjectEventStateQueue(driver->domainEventState, event);
virObjectEventStateQueue(driver->domainEventState, event2);
- virNWFilterUnlockFilterUpdates();
return dom;
}
@@ -5776,8 +5773,6 @@ qemuDomainRestoreFlags(virConnectPtr conn,
if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM)
reset_nvram = true;
- virNWFilterReadLockFilterUpdates();
-
fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
(flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0,
&wrapperFd, false, false);
@@ -5849,7 +5844,6 @@ qemuDomainRestoreFlags(virConnectPtr conn,
if (vm && ret < 0)
qemuDomainRemoveInactiveJob(driver, vm);
virDomainObjEndAPI(&vm);
- virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -6403,8 +6397,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
VIR_DOMAIN_START_FORCE_BOOT |
VIR_DOMAIN_START_RESET_NVRAM, -1);
- virNWFilterReadLockFilterUpdates();
-
if (!(vm = qemuDomainObjFromDomain(dom)))
goto cleanup;
@@ -6433,7 +6425,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
cleanup:
virDomainObjEndAPI(&vm);
- virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -7815,8 +7806,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
virDomainObj *vm = NULL;
int ret = -1;
- virNWFilterReadLockFilterUpdates();
-
if (!(vm = qemuDomainObjFromDomain(dom)))
goto cleanup;
@@ -7839,7 +7828,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
cleanup:
virDomainObjEndAPI(&vm);
- virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -7869,8 +7857,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
VIR_DOMAIN_AFFECT_CONFIG |
VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
- virNWFilterReadLockFilterUpdates();
-
cfg = virQEMUDriverGetConfig(driver);
if (!(vm = qemuDomainObjFromDomain(dom)))
@@ -7947,7 +7933,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
if (dev != dev_copy)
virDomainDeviceDefFree(dev_copy);
virDomainObjEndAPI(&vm);
- virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -13654,8 +13639,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainObj *vm = NULL;
int ret = -1;
- virNWFilterReadLockFilterUpdates();
-
if (!(vm = qemuDomObjFromSnapshot(snapshot)))
goto cleanup;
@@ -13666,7 +13649,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cleanup:
virDomainObjEndAPI(&vm);
- virNWFilterUnlockFilterUpdates();
return ret;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 5aecdddff0..43ee094486 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2782,8 +2782,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver,
int rv;
g_autofree char *tlsAlias = NULL;
- virNWFilterReadLockFilterUpdates();
-
if (flags & VIR_MIGRATE_OFFLINE) {
if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
VIR_MIGRATE_NON_SHARED_INC)) {
@@ -3103,7 +3101,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver,
virDomainObjEndAPI(&vm);
virObjectEventStateQueue(driver->domainEventState, event);
qemuMigrationCookieFree(mig);
- virNWFilterUnlockFilterUpdates();
virErrorRestore(&origErr);
return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 5af925e521..b19a6218d0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -9004,7 +9004,6 @@ qemuProcessReconnect(void *opaque)
qemuDomainRemoveInactiveJob(driver, obj);
}
virDomainObjEndAPI(&obj);
- virNWFilterUnlockFilterUpdates();
virIdentitySetCurrent(NULL);
return;
@@ -9056,8 +9055,6 @@ qemuProcessReconnectHelper(virDomainObj *obj,
data->obj = obj;
data->identity = virIdentityGetCurrent();
- virNWFilterReadLockFilterUpdates();
-
/* this lock and reference will be eventually transferred to the thread
* that handles the reconnect */
virObjectLock(obj);
@@ -9080,7 +9077,6 @@ qemuProcessReconnectHelper(virDomainObj *obj,
qemuDomainRemoveInactiveJobLocked(src->driver, obj);
virDomainObjEndAPI(&obj);
- virNWFilterUnlockFilterUpdates();
g_clear_object(&data->identity);
VIR_FREE(data);
return -1;
--
2.35.1
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
> Now that the virNWFilterBinding APIs are using the nwfilter
> update lock directly, there is no need for the virt drivers
> to do it themselves.
I'm always nervous when the ordering of locks is changed, and that's
what is happening with the combination of this patch and the previous
patch. Before it was always the NWFilterLock that was acquired first,
and then the domain object is retrieved (which involves getting the
domain-list lock while getting a ref to the domain object).
But since holding of the domain-list lock is localized to that short
period (and certainly never across a call to the NWFilter binding API)
I'm fairly certain this (along with the previous patch) is safe from
creating any new deadlocks.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
> ---
> src/lxc/lxc_driver.c | 6 ------
> src/qemu/qemu_driver.c | 18 ------------------
> src/qemu/qemu_migration.c | 3 ---
> src/qemu/qemu_process.c | 4 ----
> 4 files changed, 31 deletions(-)
>
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 020ec257ae..4185a61267 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -971,8 +971,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
>
> virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (!(vm = lxcDomObjFromDomain(dom)))
> goto cleanup;
>
> @@ -1014,7 +1012,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
> cleanup:
> virDomainObjEndAPI(&vm);
> virObjectEventStateQueue(driver->domainEventState, event);
> - virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> @@ -1080,8 +1077,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
> if (flags & VIR_DOMAIN_START_VALIDATE)
> parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (!(caps = virLXCDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> @@ -1138,7 +1133,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
> cleanup:
> virDomainObjEndAPI(&vm);
> virObjectEventStateQueue(driver->domainEventState, event);
> - virNWFilterUnlockFilterUpdates();
> return dom;
> }
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b74b0375a7..e4e1cd7bdf 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1606,8 +1606,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
> if (flags & VIR_DOMAIN_START_RESET_NVRAM)
> start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM;
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (!(def = virDomainDefParseString(xml, driver->xmlopt,
> NULL, parse_flags)))
> goto cleanup;
> @@ -1661,7 +1659,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
> virDomainObjEndAPI(&vm);
> virObjectEventStateQueue(driver->domainEventState, event);
> virObjectEventStateQueue(driver->domainEventState, event2);
> - virNWFilterUnlockFilterUpdates();
> return dom;
> }
>
> @@ -5776,8 +5773,6 @@ qemuDomainRestoreFlags(virConnectPtr conn,
> if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM)
> reset_nvram = true;
>
> - virNWFilterReadLockFilterUpdates();
> -
> fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
> (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0,
> &wrapperFd, false, false);
> @@ -5849,7 +5844,6 @@ qemuDomainRestoreFlags(virConnectPtr conn,
> if (vm && ret < 0)
> qemuDomainRemoveInactiveJob(driver, vm);
> virDomainObjEndAPI(&vm);
> - virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> @@ -6403,8 +6397,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
> VIR_DOMAIN_START_FORCE_BOOT |
> VIR_DOMAIN_START_RESET_NVRAM, -1);
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (!(vm = qemuDomainObjFromDomain(dom)))
> goto cleanup;
>
> @@ -6433,7 +6425,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
>
> cleanup:
> virDomainObjEndAPI(&vm);
> - virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> @@ -7815,8 +7806,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
> virDomainObj *vm = NULL;
> int ret = -1;
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (!(vm = qemuDomainObjFromDomain(dom)))
> goto cleanup;
>
> @@ -7839,7 +7828,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
>
> cleanup:
> virDomainObjEndAPI(&vm);
> - virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> @@ -7869,8 +7857,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
> VIR_DOMAIN_AFFECT_CONFIG |
> VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
>
> - virNWFilterReadLockFilterUpdates();
> -
> cfg = virQEMUDriverGetConfig(driver);
>
> if (!(vm = qemuDomainObjFromDomain(dom)))
> @@ -7947,7 +7933,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
> if (dev != dev_copy)
> virDomainDeviceDefFree(dev_copy);
> virDomainObjEndAPI(&vm);
> - virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> @@ -13654,8 +13639,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> virDomainObj *vm = NULL;
> int ret = -1;
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (!(vm = qemuDomObjFromSnapshot(snapshot)))
> goto cleanup;
>
> @@ -13666,7 +13649,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>
> cleanup:
> virDomainObjEndAPI(&vm);
> - virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 5aecdddff0..43ee094486 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2782,8 +2782,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver,
> int rv;
> g_autofree char *tlsAlias = NULL;
>
> - virNWFilterReadLockFilterUpdates();
> -
> if (flags & VIR_MIGRATE_OFFLINE) {
> if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
> VIR_MIGRATE_NON_SHARED_INC)) {
> @@ -3103,7 +3101,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver,
> virDomainObjEndAPI(&vm);
> virObjectEventStateQueue(driver->domainEventState, event);
> qemuMigrationCookieFree(mig);
> - virNWFilterUnlockFilterUpdates();
> virErrorRestore(&origErr);
> return ret;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 5af925e521..b19a6218d0 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -9004,7 +9004,6 @@ qemuProcessReconnect(void *opaque)
> qemuDomainRemoveInactiveJob(driver, obj);
> }
> virDomainObjEndAPI(&obj);
> - virNWFilterUnlockFilterUpdates();
> virIdentitySetCurrent(NULL);
> return;
>
> @@ -9056,8 +9055,6 @@ qemuProcessReconnectHelper(virDomainObj *obj,
> data->obj = obj;
> data->identity = virIdentityGetCurrent();
>
> - virNWFilterReadLockFilterUpdates();
> -
> /* this lock and reference will be eventually transferred to the thread
> * that handles the reconnect */
> virObjectLock(obj);
> @@ -9080,7 +9077,6 @@ qemuProcessReconnectHelper(virDomainObj *obj,
> qemuDomainRemoveInactiveJobLocked(src->driver, obj);
>
> virDomainObjEndAPI(&obj);
> - virNWFilterUnlockFilterUpdates();
> g_clear_object(&data->identity);
> VIR_FREE(data);
> return -1;
On Mon, Feb 28, 2022 at 11:24:07AM -0500, Laine Stump wrote: > On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: > > Now that the virNWFilterBinding APIs are using the nwfilter > > update lock directly, there is no need for the virt drivers > > to do it themselves. > > I'm always nervous when the ordering of locks is changed, and that's what is > happening with the combination of this patch and the previous patch. Before > it was always the NWFilterLock that was acquired first, and then the domain > object is retrieved (which involves getting the domain-list lock while > getting a ref to the domain object). > > But since holding of the domain-list lock is localized to that short period > (and certainly never across a call to the NWFilter binding API) I'm fairly > certain this (along with the previous patch) is safe from creating any new > deadlocks. Note the reason for that ordering was that in the past the nwfilter driver would have ability to call back into the virt driver to ask the virt driver to reload all nwfilters for VMs. With this callback we would acquire the nwfilter update lock, and then iterate over each VM in turn. The nwfilter driver no longer has this ability. It is completely self contained when re-loadeding nwfilters. The only calls are from the virt driver into the nwfilter driver. Thus there can never be any scenario where a nwfilter driver lock is held when the virt driver tries to acquire a domain lock. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2026 Red Hat, Inc.