[PATCH v2 0/2] Fix calling of virNetworkUpdate() driver callback
v2 of: https://listman.redhat.com/archives/libvir-list/2021-March/msg00052.html I've pushed 1/2 from the v1 series (which just debug printed all arguments) which is unrelated to this problem. diff to v1: - Implemented different approach as suggested by Jano in his review of v1. I must say I like it more. Michal Prívozník (2): network: Implement virConnectSupportsFeature() lib: Fix calling of virNetworkUpdate() driver callback src/esx/esx_driver.c | 3 +++ src/libvirt-network.c | 24 ++++++++++++++++++++-- src/libvirt_internal.h | 5 +++++ src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/network/bridge_driver.c | 32 +++++++++++++++++++++++++++++ src/openvz/openvz_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_daemon_dispatch.c | 1 + src/test/test_driver.c | 1 + 10 files changed, 68 insertions(+), 2 deletions(-) -- 2.26.2
So far, it was not needed, but shortly a client will want to know whether virNetworkUpdate() API is fixed or not. See next commits for more info. Side note, this driver's implementation is called only when using sub-driver's connection, i.e. "network:///system". For any other URI the corresponding hypervisor's driver callback is called. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3c7a49b115..7c9e36c625 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -944,6 +944,35 @@ static int networkConnectIsAlive(virConnectPtr conn G_GNUC_UNUSED) } +static int +networkConnectSupportsFeature(virConnectPtr conn, int feature) +{ + if (virConnectSupportsFeatureEnsureACL(conn) < 0) + return -1; + + switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_MIGRATION_V2: + case VIR_DRV_FEATURE_MIGRATION_V3: + case VIR_DRV_FEATURE_MIGRATION_P2P: + case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION: + case VIR_DRV_FEATURE_FD_PASSING: + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_XML_MIGRATABLE: + case VIR_DRV_FEATURE_MIGRATION_OFFLINE: + case VIR_DRV_FEATURE_MIGRATION_PARAMS: + case VIR_DRV_FEATURE_MIGRATION_DIRECT: + case VIR_DRV_FEATURE_MIGRATION_V1: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: + default: + return 0; + } +} + + + static char * networkBuildDnsmasqLeaseTime(virNetworkDHCPLeaseTimeDefPtr lease) { @@ -5630,6 +5659,7 @@ static virHypervisorDriver networkHypervisorDriver = { .connectIsEncrypted = networkConnectIsEncrypted, /* 4.1.0 */ .connectIsSecure = networkConnectIsSecure, /* 4.1.0 */ .connectIsAlive = networkConnectIsAlive, /* 4.1.0 */ + .connectSupportsFeature = networkConnectSupportsFeature, /* 7.2.0 */ }; -- 2.26.2
The order in which virNetworkUpdate() accepts @section and @command arguments is not the same as in which it passes them onto networkUpdate() callback. Until recently, it did not really matter, because calling the API on client side meant arguments were encoded in reversed order (compared to the public API), but then on the server it was fixed again - because the server decoded RPC (still swapped), called public API (still swapped) and in turn called the network driver callback (with reversing the order - so magically fixing the order). Long story short, if the public API is called even number of times those swaps cancel each other out. The problem is when the API is called an odd numbed of times - which happens with split daemons and the right URI. There's one call in the client (e.g. virsh net-update), the other in a hypervisor daemon (say virtqemud) which ends up calling the API in the virnetworkd. The fix is obvious - fix the order in which arguments are passed to the callback. But, to maintain compatibility with older, yet unfixed, daemons new connection feature is introduced. The feature is detected just before calling the callback and allows client to pass arguments in correct order (talking to fixed daemon) or in reversed order (talking to older daemon). Unfortunately, older client talking to newer daemon can't be fixed. Let's hope that it's less frequent scenario. Fixes: 574b9bc66b6b10cc4cf50f299c3f0ff55f2cbefb Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1870552 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_driver.c | 3 +++ src/libvirt-network.c | 24 ++++++++++++++++++++++-- src/libvirt_internal.h | 5 +++++ src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/network/bridge_driver.c | 2 ++ src/openvz/openvz_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_daemon_dispatch.c | 1 + src/test/test_driver.c | 1 + 10 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 96cc8bda0d..5c96ab4de0 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1037,6 +1037,9 @@ esxConnectSupportsFeature(virConnectPtr conn, int feature) return priv->vCenter && supportsVMotion == esxVI_Boolean_True ? 1 : 0; + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + return 1; + case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION: case VIR_DRV_FEATURE_MIGRATION_DIRECT: diff --git a/src/libvirt-network.c b/src/libvirt-network.c index b84389f762..145487d599 100644 --- a/src/libvirt-network.c +++ b/src/libvirt-network.c @@ -543,8 +543,28 @@ virNetworkUpdate(virNetworkPtr network, if (conn->networkDriver && conn->networkDriver->networkUpdate) { int ret; - ret = conn->networkDriver->networkUpdate(network, section, command, - parentIndex, xml, flags); + int rc; + + /* Since its introduction in v0.10.2-rc1~9 the @section and @command + * arguments were mistakenly swapped when passed to driver's callback. + * Detect if the other side is fixed already or not. */ + rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, + VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER); + + VIR_DEBUG("Argument order feature detection returned: %d", rc); + if (rc < 0) + goto error; + + if (rc == 0) { + /* Feature not supported, preserve swapped order */ + ret = conn->networkDriver->networkUpdate(network, section, command, + parentIndex, xml, flags); + } else { + /* Feature supported, correct order can be used */ + ret = conn->networkDriver->networkUpdate(network, command, section, + parentIndex, xml, flags); + } + if (ret < 0) goto error; return ret; diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 2bf7744bd6..f4e592922d 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -126,6 +126,11 @@ typedef enum { * Support for driver close callback rpc */ VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK = 15, + + /* + * Whether the virNetworkUpdate() API implementation passes arguments to + * the driver's callback in correct order. */ + VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER = 16, } virDrvFeature; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e3d769b5d9..f82095ffc7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5752,6 +5752,7 @@ libxlConnectSupportsFeature(virConnectPtr conn, int feature) case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_MIGRATION_PARAMS: case VIR_DRV_FEATURE_MIGRATION_P2P: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8e0ec82e0b..2db544c945 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1651,6 +1651,7 @@ lxcConnectSupportsFeature(virConnectPtr conn, int feature) switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7c9e36c625..14db9ffc82 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -951,6 +951,8 @@ networkConnectSupportsFeature(virConnectPtr conn, int feature) return -1; switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + return 1; case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: case VIR_DRV_FEATURE_MIGRATION_P2P: diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index e898af85ab..9f65dff5d1 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1992,6 +1992,7 @@ openvzConnectSupportsFeature(virConnectPtr conn G_GNUC_UNUSED, int feature) switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_MIGRATION_PARAMS: case VIR_DRV_FEATURE_MIGRATION_V3: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 16c5ccae45..0ca08b3292 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1238,6 +1238,7 @@ qemuConnectSupportsFeature(virConnectPtr conn, int feature) case VIR_DRV_FEATURE_XML_MIGRATABLE: case VIR_DRV_FEATURE_MIGRATION_OFFLINE: case VIR_DRV_FEATURE_MIGRATION_PARAMS: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; case VIR_DRV_FEATURE_MIGRATION_DIRECT: case VIR_DRV_FEATURE_MIGRATION_V1: diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 9700dba450..f57ad0e0c9 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -4995,6 +4995,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server G_GNUC_UN case VIR_DRV_FEATURE_XML_MIGRATABLE: case VIR_DRV_FEATURE_MIGRATION_OFFLINE: case VIR_DRV_FEATURE_MIGRATION_PARAMS: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: default: if ((supported = virConnectSupportsFeature(conn, args->feature)) < 0) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 01b3e7bc82..019d61fce1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1590,6 +1590,7 @@ testConnectSupportsFeature(virConnectPtr conn G_GNUC_UNUSED, { switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: -- 2.26.2
On Tue, Mar 16, 2021 at 12:44:04PM +0100, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00052.html
I've pushed 1/2 from the v1 series (which just debug printed all arguments) which is unrelated to this problem.
diff to v1: - Implemented different approach as suggested by Jano in his review of v1. I must say I like it more.
Michal Prívozník (2): network: Implement virConnectSupportsFeature() lib: Fix calling of virNetworkUpdate() driver callback
Terrifying, but makes sense. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
src/esx/esx_driver.c | 3 +++ src/libvirt-network.c | 24 ++++++++++++++++++++-- src/libvirt_internal.h | 5 +++++ src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/network/bridge_driver.c | 32 +++++++++++++++++++++++++++++ src/openvz/openvz_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_daemon_dispatch.c | 1 + src/test/test_driver.c | 1 + 10 files changed, 68 insertions(+), 2 deletions(-)
-- 2.26.2
participants (2)
-
Martin Kletzander -
Michal Privoznik