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(a)redhat.com&gt; --- 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

keepalive is a RPC feature, drivers must say that it's not supported. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

On 2/16/22 16:41, Peter Krempa wrote: 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 2/17/22 10:45, Peter Krempa wrote: 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. Yeah, it's just that I'm unable to come with anything better. Michal

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(a)redhat.com&gt; --- 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