[PATCH 0/8] driver: Fix handling of driver feature flags
In a review of code using FD passing I was asked to add the check, so I wanted to see how it works and found a few issues. Peter Krempa (8): driver: Introduce global driver feature flag handling function virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_REMOTE virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_PROGRAM_KEEPALIVE virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_REMOTE_(CLOSE|EVENT)_CALLBACK virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_TYPED_PARAM_STRING virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_FD_PASSING virDomainCreate(XML)WithFiles: Add check for VIR_DRV_FEATURE_FD_PASSING src/ch/ch_driver.c | 5 +++ src/driver.c | 65 +++++++++++++++++++++++++++++++++++++ src/driver.h | 3 ++ src/esx/esx_driver.c | 4 +++ src/libvirt-domain.c | 24 ++++++++++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 5 +++ src/lxc/lxc_driver.c | 5 +++ src/network/bridge_driver.c | 5 +++ src/openvz/openvz_driver.c | 5 +++ src/qemu/qemu_driver.c | 5 +++ src/test/test_driver.c | 5 +++ src/vz/vz_driver.c | 5 +++ 13 files changed, 137 insertions(+) -- 2.35.1
The 'virDrvFeature' has a combination of features which are asserted by the specific driver and features which are actually global. In many cases the implementation was cargo-culted into newer drivers without re-assesing whether it makes sense. This patch introduces a global function which will specifically handle these global flags and defer the rest to the driver. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/ch/ch_driver.c | 5 +++++ src/driver.c | 41 +++++++++++++++++++++++++++++++++++++ src/driver.h | 3 +++ src/esx/esx_driver.c | 4 ++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 5 +++++ src/lxc/lxc_driver.c | 5 +++++ src/network/bridge_driver.c | 5 +++++ src/openvz/openvz_driver.c | 5 +++++ src/qemu/qemu_driver.c | 5 +++++ src/test/test_driver.c | 5 +++++ src/vz/vz_driver.c | 5 +++++ 12 files changed, 89 insertions(+) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index a0ff687d05..ac9298c0b5 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -919,9 +919,14 @@ static int chConnectSupportsFeature(virConnectPtr conn, int feature) { + int supported; + if (virConnectSupportsFeatureEnsureACL(conn) < 0) return -1; + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: diff --git a/src/driver.c b/src/driver.c index 9ae95cb4c3..8c70662ccb 100644 --- a/src/driver.c +++ b/src/driver.c @@ -316,3 +316,44 @@ virConnectValidateURIPath(const char *uriPath, return true; } + + +/** + * virDriverFeatureIsGlobal: + * @feat: a VIR_DRV_FEATURE + * @supported: If a feature is globally handled + * + * Certain driver feature flags are really not for individual drivers to decide + * whether they implement them or not, but are rather global based on e.g. + * whether the RPC protocol supports it. + * + * This function returns 'true' and fills @supported if a feature is a global + * feature and the individual driver implementations don't decide whether + * they support it or not. + */ +bool +virDriverFeatureIsGlobal(virDrvFeature feat, + int *supported G_GNUC_UNUSED) + +{ + switch (feat) { + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + case VIR_DRV_FEATURE_FD_PASSING: + 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_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 false; + } +} diff --git a/src/driver.h b/src/driver.h index 7f45231f24..cd7cd96844 100644 --- a/src/driver.h +++ b/src/driver.h @@ -131,3 +131,6 @@ int virSetConnectStorage(virConnectPtr conn); bool virConnectValidateURIPath(const char *uriPath, const char *entityName, bool privileged); + +bool virDriverFeatureIsGlobal(virDrvFeature feat, + int *supported); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 467740804a..3149f3e963 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1018,6 +1018,10 @@ esxConnectSupportsFeature(virConnectPtr conn, int feature) { esxPrivate *priv = conn->privateData; esxVI_Boolean supportsVMotion = esxVI_Boolean_Undefined; + int supported; + + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_MIGRATION_V1: diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2380f02b88..6f0d72ca38 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1528,6 +1528,7 @@ virStreamClass; # driver.h virConnectValidateURIPath; +virDriverFeatureIsGlobal; virDriverShouldAutostart; virGetConnectInterface; virGetConnectNetwork; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4c61d330ed..478ab3e941 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5697,9 +5697,14 @@ libxlConnectListAllDomains(virConnectPtr conn, static int libxlConnectSupportsFeature(virConnectPtr conn, int feature) { + int supported; + if (virConnectSupportsFeatureEnsureACL(conn) < 0) return -1; + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_MIGRATION_V3: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ff83557bac..020ec257ae 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1618,9 +1618,14 @@ static int lxcStateCleanup(void) static int lxcConnectSupportsFeature(virConnectPtr conn, int feature) { + int supported; + if (virConnectSupportsFeatureEnsureACL(conn) < 0) return -1; + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3750da7962..d6ae05360b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -857,9 +857,14 @@ static int networkConnectIsAlive(virConnectPtr conn G_GNUC_UNUSED) static int networkConnectSupportsFeature(virConnectPtr conn, int feature) { + int supported; + if (virConnectSupportsFeatureEnsureACL(conn) < 0) return -1; + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b2589059c8..aa1db09540 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1938,6 +1938,11 @@ openvzNodeGetCPUMap(virConnectPtr conn G_GNUC_UNUSED, static int openvzConnectSupportsFeature(virConnectPtr conn G_GNUC_UNUSED, int feature) { + int supported; + + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_MIGRATION_PARAMS: case VIR_DRV_FEATURE_MIGRATION_V3: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f2620200f0..f1f708e511 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1173,9 +1173,14 @@ static int qemuConnectClose(virConnectPtr conn) static int qemuConnectSupportsFeature(virConnectPtr conn, int feature) { + int supported; + if (virConnectSupportsFeatureEnsureACL(conn) < 0) return -1; + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 03c41ca192..4eca5c4a65 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1667,6 +1667,11 @@ static int testConnectSupportsFeature(virConnectPtr conn G_GNUC_UNUSED, int feature) { + int supported; + + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 86bc53d631..fc91b6dddf 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3006,9 +3006,14 @@ vzDomainMigratePrepare3Params(virConnectPtr conn, static int vzConnectSupportsFeature(virConnectPtr conn G_GNUC_UNUSED, int feature) { + int supported; + if (virConnectSupportsFeatureEnsureACL(conn) < 0) return -1; + if (virDriverFeatureIsGlobal(feature, &supported)) + return supported; + switch ((virDrvFeature) feature) { case VIR_DRV_FEATURE_MIGRATION_PARAMS: case VIR_DRV_FEATURE_MIGRATION_P2P: -- 2.35.1
VIR_DRV_FEATURE_REMOTE is a special flag which is asserted only when the connection is remote. All drivers implementing it must return 0 for it to work. Handle it in the global handler and add a comment why. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/driver.c b/src/driver.c index 8c70662ccb..b450400084 100644 --- a/src/driver.c +++ b/src/driver.c @@ -333,10 +333,18 @@ virConnectValidateURIPath(const char *uriPath, */ bool virDriverFeatureIsGlobal(virDrvFeature feat, - int *supported G_GNUC_UNUSED) + int *supported) { switch (feat) { + /* This is a special case where the generated remote driver dispatcher + * function intercepts this specific flag and returns '1'. Thus any local + * implementation must return 0, so that the return value properly reflects + * whether we are going through the remote driver */ + case VIR_DRV_FEATURE_REMOTE: + *supported = 0; + return true; + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: case VIR_DRV_FEATURE_FD_PASSING: @@ -350,7 +358,6 @@ virDriverFeatureIsGlobal(virDrvFeature feat, 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: -- 2.35.1
keepalive is a RPC feature, drivers must say that it's not supported. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/driver.c b/src/driver.c index b450400084..6bebfeba58 100644 --- a/src/driver.c +++ b/src/driver.c @@ -342,6 +342,9 @@ virDriverFeatureIsGlobal(virDrvFeature feat, * implementation must return 0, so that the return value properly reflects * whether we are going through the remote driver */ case VIR_DRV_FEATURE_REMOTE: + /* keepalive is handled at RPC level, driver implementations must always + * return 0, to signal that direct/embedded use doesn't use keepalive */ + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: *supported = 0; return true; @@ -357,7 +360,6 @@ virDriverFeatureIsGlobal(virDrvFeature feat, 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_CLOSE_CALLBACK: case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: default: -- 2.35.1
They are features of the RPC; drivers must say that it's not supported. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/driver.c b/src/driver.c index 6bebfeba58..9852a1cb17 100644 --- a/src/driver.c +++ b/src/driver.c @@ -345,6 +345,11 @@ virDriverFeatureIsGlobal(virDrvFeature feat, /* keepalive is handled at RPC level, driver implementations must always * return 0, to signal that direct/embedded use doesn't use keepalive */ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + /* Support for close callbacks and remote event filtering are both features + * of the RPC protocol and thus normal drivers must not signal support + * for them. */ + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: *supported = 0; return true; @@ -360,8 +365,6 @@ virDriverFeatureIsGlobal(virDrvFeature feat, case VIR_DRV_FEATURE_MIGRATION_PARAMS: case VIR_DRV_FEATURE_MIGRATION_DIRECT: case VIR_DRV_FEATURE_MIGRATION_V1: - case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: - case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: default: return false; } -- 2.35.1
This was a lockout to make strings in typed parameters compatible with versions which didn't have them. Now all drivers need to expose this capability. This namely enables it for 'esx' and 'vz' drivers, while they don't seem to be implementing any parameters for now, they might later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/driver.c b/src/driver.c index 9852a1cb17..d070861cfd 100644 --- a/src/driver.c +++ b/src/driver.c @@ -353,7 +353,13 @@ virDriverFeatureIsGlobal(virDrvFeature feat, *supported = 0; return true; + /* Limitation of string support in typed parameters was an RPC limitation. + * At this point everything supports them and thus also drivers need to + * always advertise this feature */ case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + *supported = 1; + return true; + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATION_V2: -- 2.35.1
The fix was on RPC level so everything should advertise it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/driver.c b/src/driver.c index d070861cfd..54b4ad5b43 100644 --- a/src/driver.c +++ b/src/driver.c @@ -357,10 +357,12 @@ virDriverFeatureIsGlobal(virDrvFeature feat, * At this point everything supports them and thus also drivers need to * always advertise this feature */ case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + /* Feature flag exposes that accidental switching of order of arguments + * in RPC was fixed. All implementations need to advertise this feature */ + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: *supported = 1; return true; - case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: -- 2.35.1
On 2/16/22 16:41, Peter Krempa wrote:
The fix was on RPC level so everything should advertise it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/driver.c b/src/driver.c index d070861cfd..54b4ad5b43 100644 --- a/src/driver.c +++ b/src/driver.c @@ -357,10 +357,12 @@ virDriverFeatureIsGlobal(virDrvFeature feat, * At this point everything supports them and thus also drivers need to * always advertise this feature */ case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + /* Feature flag exposes that accidental switching of order of arguments + * in RPC was fixed. All implementations need to advertise this feature */ + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: *supported = 1; return true;
- case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3:
Just FYI, RPC layer doesn't have to necessary be involved. We have embed mode which uses drivers directly. Even though, network driver is currently doesn't work in embed mode. And strictly speaking this was not tied to RPC, the problem was in public API implementation which called the callback with swapped arguments. So any of local drivers (remote driver, ESX, test driver, ...) were affected too, even though it's the remote driver alone that implemented the callback. I'm not objecting to this patch, just trying to shed more light into the problem. Michal
On Thu, Feb 17, 2022 at 10:39:59 +0100, Michal Prívozník wrote:
On 2/16/22 16:41, Peter Krempa wrote:
The fix was on RPC level so everything should advertise it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/driver.c b/src/driver.c index d070861cfd..54b4ad5b43 100644 --- a/src/driver.c +++ b/src/driver.c @@ -357,10 +357,12 @@ virDriverFeatureIsGlobal(virDrvFeature feat, * At this point everything supports them and thus also drivers need to * always advertise this feature */ case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + /* Feature flag exposes that accidental switching of order of arguments + * in RPC was fixed. All implementations need to advertise this feature */ + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: *supported = 1; return true;
- case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3:
Just FYI, RPC layer doesn't have to necessary be involved. We have embed mode which uses drivers directly. Even though, network driver is currently doesn't work in embed mode. And strictly speaking this was not tied to RPC, the problem was in public API implementation which called the callback with swapped arguments. So any of local drivers (remote driver, ESX, test driver, ...) were affected too, even though it's the remote driver alone that implemented the callback.
So, the problematic ordering was in the public API wrapper function?
I'm not objecting to this patch, just trying to shed more light into the problem.
I can update the comment. Actually the idea is that the comment captures the reason for the flag, so it should be acurate.
On 2/17/22 10:45, Peter Krempa wrote:
On Thu, Feb 17, 2022 at 10:39:59 +0100, Michal Prívozník wrote:
On 2/16/22 16:41, Peter Krempa wrote:
The fix was on RPC level so everything should advertise it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/driver.c b/src/driver.c index d070861cfd..54b4ad5b43 100644 --- a/src/driver.c +++ b/src/driver.c @@ -357,10 +357,12 @@ virDriverFeatureIsGlobal(virDrvFeature feat, * At this point everything supports them and thus also drivers need to * always advertise this feature */ case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + /* Feature flag exposes that accidental switching of order of arguments + * in RPC was fixed. All implementations need to advertise this feature */ + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: *supported = 1; return true;
- case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3:
Just FYI, RPC layer doesn't have to necessary be involved. We have embed mode which uses drivers directly. Even though, network driver is currently doesn't work in embed mode. And strictly speaking this was not tied to RPC, the problem was in public API implementation which called the callback with swapped arguments. So any of local drivers (remote driver, ESX, test driver, ...) were affected too, even though it's the remote driver alone that implemented the callback.
So, the problematic ordering was in the public API wrapper function?
Indeed: git show -W b0f78d626a18bcecae3a4d165540ab88bfbfc9ee -- src/libvirt-network.c You'll see that the public API was given @command and then @section arguments, but the callback was called with @section and @command (reversed). This went unnoticed because both arguments are of the same type (thus RPC serializer didn't report any error) and because the public API is called again on the daemon side (during dispatch) so the order was swapped again and thus network driver saw arguments in correct order. Problem arose only when split daemons were introduced -> suddenly the API was called three times (one time at client, one time at virtqemud and finally in virnetworkd), so there was imbalance of swapping. And yes, if client connected to virtnetworkd directly everything worked, because again - two swapps were done.
I'm not objecting to this patch, just trying to shed more light into the problem.
I can update the comment. Actually the idea is that the comment captures the reason for the flag, so it should be acurate.
Yeah, it's just that I'm unable to come with anything better. Michal
On Thu, Feb 17, 2022 at 10:56:11 +0100, Michal Prívozník wrote:
On 2/17/22 10:45, Peter Krempa wrote:
On Thu, Feb 17, 2022 at 10:39:59 +0100, Michal Prívozník wrote:
On 2/16/22 16:41, Peter Krempa wrote:
The fix was on RPC level so everything should advertise it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
[...]
git show -W b0f78d626a18bcecae3a4d165540ab88bfbfc9ee -- src/libvirt-network.c
You'll see that the public API was given @command and then @section arguments, but the callback was called with @section and @command (reversed). This went unnoticed because both arguments are of the same type (thus RPC serializer didn't report any error) and because the public API is called again on the daemon side (during dispatch) so the order was swapped again and thus network driver saw arguments in correct order. Problem arose only when split daemons were introduced -> suddenly the API was called three times (one time at client, one time at virtqemud and finally in virnetworkd), so there was imbalance of swapping. And yes, if client connected to virtnetworkd directly everything worked, because again - two swapps were done.
I'm not objecting to this patch, just trying to shed more light into the problem.
I can update the comment. Actually the idea is that the comment captures the reason for the flag, so it should be acurate.
Yeah, it's just that I'm unable to come with anything better.
How about: /* Feature flag exposes that the accidental switching of order of * arguments in the public API trampoline virNetworkUpdate is known. * Updated clients thus use the correct ordering with an updated * server. All drivers must signal support for this feature. */
On 2/17/22 12:42, Peter Krempa wrote:
On Thu, Feb 17, 2022 at 10:56:11 +0100, Michal Prívozník wrote:
On 2/17/22 10:45, Peter Krempa wrote:
On Thu, Feb 17, 2022 at 10:39:59 +0100, Michal Prívozník wrote:
On 2/16/22 16:41, Peter Krempa wrote:
The fix was on RPC level so everything should advertise it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
[...]
git show -W b0f78d626a18bcecae3a4d165540ab88bfbfc9ee -- src/libvirt-network.c
You'll see that the public API was given @command and then @section arguments, but the callback was called with @section and @command (reversed). This went unnoticed because both arguments are of the same type (thus RPC serializer didn't report any error) and because the public API is called again on the daemon side (during dispatch) so the order was swapped again and thus network driver saw arguments in correct order. Problem arose only when split daemons were introduced -> suddenly the API was called three times (one time at client, one time at virtqemud and finally in virnetworkd), so there was imbalance of swapping. And yes, if client connected to virtnetworkd directly everything worked, because again - two swapps were done.
I'm not objecting to this patch, just trying to shed more light into the problem.
I can update the comment. Actually the idea is that the comment captures the reason for the flag, so it should be acurate.
Yeah, it's just that I'm unable to come with anything better.
How about:
/* Feature flag exposes that the accidental switching of order of * arguments in the public API trampoline virNetworkUpdate is known. * Updated clients thus use the correct ordering with an updated * server. All drivers must signal support for this feature. */
Perfect! Thanks. Michal
The feature implies that fd passing works with RPC. Non-remote impls thus should always report support. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/driver.c b/src/driver.c index 54b4ad5b43..758ee3e320 100644 --- a/src/driver.c +++ b/src/driver.c @@ -360,10 +360,14 @@ virDriverFeatureIsGlobal(virDrvFeature feat, /* Feature flag exposes that accidental switching of order of arguments * in RPC was fixed. All implementations need to advertise this feature */ case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + /* The remote driver intercepts and always reports the feature since it was + * introduced. This means that all driver implementations should advertise + * it too as it works natively without RPC. Always enabling this will also + * prevent regressions when a driver is used in embedded mode */ + case VIR_DRV_FEATURE_FD_PASSING: *supported = 1; return true; - case VIR_DRV_FEATURE_FD_PASSING: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: case VIR_DRV_FEATURE_MIGRATION_P2P: -- 2.35.1
All APIs using FD passing have this check to prevent sending a 'VIR_NET_CALL_WITH_FDS' to an older daemon but virDomainCreateXMLWithFiles was missing it. Now the LXC driver was historically not exposing VIR_DRV_FEATURE_FD_PASSING, but that is not a problem as LXC always goes through the remote driver which intercepts it and injects VIR_DRV_FEATURE_FD_PASSING when it was implemented. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index b8a6f10333..8be2c21194 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -244,6 +244,18 @@ virDomainCreateXMLWithFiles(virConnectPtr conn, const char *xmlDesc, virCheckNonNullArgGoto(xmlDesc, error); virCheckReadOnlyGoto(conn->flags, error); + if (nfiles > 0) { + int rc; + + if ((rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, + VIR_DRV_FEATURE_FD_PASSING)) <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("fd passing is not supported by this connection")); + goto error; + } + } + if (conn->driver->domainCreateXMLWithFiles) { virDomainPtr ret; ret = conn->driver->domainCreateXMLWithFiles(conn, xmlDesc, @@ -6869,6 +6881,18 @@ virDomainCreateWithFiles(virDomainPtr domain, unsigned int nfiles, virCheckReadOnlyGoto(conn->flags, error); + if (nfiles > 0) { + int rc; + + if ((rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, + VIR_DRV_FEATURE_FD_PASSING)) <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("fd passing is not supported by this connection")); + goto error; + } + } + if (conn->driver->domainCreateWithFiles) { int ret; ret = conn->driver->domainCreateWithFiles(domain, -- 2.35.1
On a Wednesday in 2022, Peter Krempa wrote:
In a review of code using FD passing I was asked to add the check, so I wanted to see how it works and found a few issues.
Peter Krempa (8): driver: Introduce global driver feature flag handling function virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_REMOTE virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_PROGRAM_KEEPALIVE virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_REMOTE_(CLOSE|EVENT)_CALLBACK virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_TYPED_PARAM_STRING virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_FD_PASSING virDomainCreate(XML)WithFiles: Add check for VIR_DRV_FEATURE_FD_PASSING
src/ch/ch_driver.c | 5 +++ src/driver.c | 65 +++++++++++++++++++++++++++++++++++++ src/driver.h | 3 ++ src/esx/esx_driver.c | 4 +++ src/libvirt-domain.c | 24 ++++++++++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 5 +++ src/lxc/lxc_driver.c | 5 +++ src/network/bridge_driver.c | 5 +++ src/openvz/openvz_driver.c | 5 +++ src/qemu/qemu_driver.c | 5 +++ src/test/test_driver.c | 5 +++ src/vz/vz_driver.c | 5 +++ 13 files changed, 137 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Feb 16, 2022 at 04:41:04PM +0100, Peter Krempa wrote:
In a review of code using FD passing I was asked to add the check, so I wanted to see how it works and found a few issues.
Peter Krempa (8): driver: Introduce global driver feature flag handling function virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_REMOTE virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_PROGRAM_KEEPALIVE virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_REMOTE_(CLOSE|EVENT)_CALLBACK virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_TYPED_PARAM_STRING virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER virDriverFeatureIsGlobal: Handle VIR_DRV_FEATURE_FD_PASSING virDomainCreate(XML)WithFiles: Add check for VIR_DRV_FEATURE_FD_PASSING
Reviewed-by: Andrea Bolognani <abologna@redhat.com> I've also posted a small follow-up patch: https://listman.redhat.com/archives/libvir-list/2022-February/msg00656.html -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Michal Prívozník -
Peter Krempa