[libvirt PATCH] drivers: Group global feature together
All these features are supposed to be handled by the call to virDriverFeatureIsGlobal() placed right above the switch statement, so group them together and add a comment. If any of these features is actually encountered as part of the switch statements, that means there's a bug in the driver and we should error out. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- Applies on top of [PATCH 0/8] driver: Fix handling of driver feature flags https://listman.redhat.com/archives/libvir-list/2022-February/msg00644.html src/ch/ch_driver.c | 13 +++++++------ src/esx/esx_driver.c | 21 +++++++++++---------- src/libxl/libxl_driver.c | 16 +++++++++------- src/lxc/lxc_driver.c | 11 ++++++----- src/network/bridge_driver.c | 15 ++++++++------- src/openvz/openvz_driver.c | 16 +++++++++------- src/qemu/qemu_driver.c | 16 +++++++++------- 7 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ac9298c0b5..9cbd7b71df 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -928,23 +928,24 @@ chConnectSupportsFeature(virConnectPtr conn, return supported; switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; 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_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; } diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 3149f3e963..6e5f9c8cc1 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1024,7 +1024,17 @@ esxConnectSupportsFeature(virConnectPtr conn, int feature) return supported; switch ((virDrvFeature) feature) { - case VIR_DRV_FEATURE_MIGRATION_V1: + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; + + case VIR_DRV_FEATURE_MIGRATION_V1: supportsVMotion = esxSupportsVMotion(priv); if (supportsVMotion == esxVI_Boolean_Undefined) @@ -1034,10 +1044,6 @@ 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: case VIR_DRV_FEATURE_MIGRATION_OFFLINE: @@ -1045,11 +1051,6 @@ esxConnectSupportsFeature(virConnectPtr conn, int feature) case VIR_DRV_FEATURE_MIGRATION_PARAMS: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: - 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: - case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_XML_MIGRATABLE: default: return 0; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 478ab3e941..46596978d4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5706,22 +5706,24 @@ libxlConnectSupportsFeature(virConnectPtr conn, int feature) return supported; switch ((virDrvFeature) feature) { - case VIR_DRV_FEATURE_MIGRATION_V3: + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; + case VIR_DRV_FEATURE_MIGRATION_V3: 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: case VIR_DRV_FEATURE_MIGRATION_DIRECT: case VIR_DRV_FEATURE_MIGRATION_OFFLINE: case VIR_DRV_FEATURE_MIGRATION_V1: case VIR_DRV_FEATURE_MIGRATION_V2: - 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: case VIR_DRV_FEATURE_XML_MIGRATABLE: default: return 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 020ec257ae..023292a75a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1627,10 +1627,15 @@ lxcConnectSupportsFeature(virConnectPtr conn, int feature) return supported; switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION: case VIR_DRV_FEATURE_MIGRATION_DIRECT: case VIR_DRV_FEATURE_MIGRATION_OFFLINE: @@ -1639,10 +1644,6 @@ lxcConnectSupportsFeature(virConnectPtr conn, int feature) case VIR_DRV_FEATURE_MIGRATION_V1: case VIR_DRV_FEATURE_MIGRATION_V2: case VIR_DRV_FEATURE_MIGRATION_V3: - 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: case VIR_DRV_FEATURE_XML_MIGRATABLE: default: return 0; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d6ae05360b..559600d2af 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -866,23 +866,24 @@ networkConnectSupportsFeature(virConnectPtr conn, int feature) return supported; switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; 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; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index aa1db09540..c034fd5af9 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1944,22 +1944,24 @@ openvzConnectSupportsFeature(virConnectPtr conn G_GNUC_UNUSED, int feature) return supported; switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; 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: case VIR_DRV_FEATURE_MIGRATION_DIRECT: case VIR_DRV_FEATURE_MIGRATION_OFFLINE: case VIR_DRV_FEATURE_MIGRATION_P2P: case VIR_DRV_FEATURE_MIGRATION_V1: case VIR_DRV_FEATURE_MIGRATION_V2: - 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: - case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_XML_MIGRATABLE: default: return 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1f708e511..433ba09745 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1182,23 +1182,25 @@ qemuConnectSupportsFeature(virConnectPtr conn, int feature) return supported; switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1; 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_NETWORK_UPDATE_HAS_CORRECT_ORDER: return 1; 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; } -- 2.35.1
s/feature/features/ On a Wednesday in 2022, Andrea Bolognani wrote:
All these features are supposed to be handled by the call to virDriverFeatureIsGlobal() placed right above the switch statement, so group them together and add a comment. If any of these features is actually encountered as part of the switch statements, that means there's a bug in the driver and we should error out.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- Applies on top of
[PATCH 0/8] driver: Fix handling of driver feature flags https://listman.redhat.com/archives/libvir-list/2022-February/msg00644.html
src/ch/ch_driver.c | 13 +++++++------ src/esx/esx_driver.c | 21 +++++++++++---------- src/libxl/libxl_driver.c | 16 +++++++++------- src/lxc/lxc_driver.c | 11 ++++++----- src/network/bridge_driver.c | 15 ++++++++------- src/openvz/openvz_driver.c | 16 +++++++++------- src/qemu/qemu_driver.c | 16 +++++++++------- 7 files changed, 59 insertions(+), 49 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ac9298c0b5..9cbd7b71df 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -928,23 +928,24 @@ chConnectSupportsFeature(virConnectPtr conn, return supported;
switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1;
Here you return an error without reporting an error. Would virReportEnumRangeError be reasonable to use here? Jano
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_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;
On Wed, Feb 16, 2022 at 09:26:21PM +0100, Ján Tomko wrote:
switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1;
Here you return an error without reporting an error.
Would virReportEnumRangeError be reasonable to use here?
Mh. While the error message ("Unexpected enum value N for virFoo") is not too specific about this, virReportEnumRangeError() is usually called when the provided value cannot be translated back to one of the enum values; in this case it *can* be converted, it's just not supposed to be encountered in the specific situation. Given the subtly different semantics, I'm leaning towards not using that helper. Would something like virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Global feature %d should have already been handled"), feature); work for you? I'd drop the comment of course. -- Andrea Bolognani / Red Hat / Virtualization
On a Thursday in 2022, Andrea Bolognani wrote:
On Wed, Feb 16, 2022 at 09:26:21PM +0100, Ján Tomko wrote:
switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1;
Here you return an error without reporting an error.
Would virReportEnumRangeError be reasonable to use here?
Mh. While the error message ("Unexpected enum value N for virFoo") is not too specific about this, virReportEnumRangeError() is usually called when the provided value cannot be translated back to one of the enum values; in this case it *can* be converted, it's just not supposed to be encountered in the specific situation. Given the subtly different semantics, I'm leaning towards not using that helper.
Would something like
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Global feature %d should have already been handled"), feature);
work for you? I'd drop the comment of course.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Apr 07, 2022 at 02:24:05PM +0200, Ján Tomko wrote:
On a Thursday in 2022, Andrea Bolognani wrote:
On Wed, Feb 16, 2022 at 09:26:21PM +0100, Ján Tomko wrote:
switch ((virDrvFeature) feature) { + case VIR_DRV_FEATURE_REMOTE: + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: case VIR_DRV_FEATURE_TYPED_PARAM_STRING: case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: - return 1; + case VIR_DRV_FEATURE_FD_PASSING: + /* Should have already been handled by virDriverFeatureIsGlobal() */ + return -1;
Here you return an error without reporting an error.
Would virReportEnumRangeError be reasonable to use here?
Mh. While the error message ("Unexpected enum value N for virFoo") is not too specific about this, virReportEnumRangeError() is usually called when the provided value cannot be translated back to one of the enum values; in this case it *can* be converted, it's just not supposed to be encountered in the specific situation. Given the subtly different semantics, I'm leaning towards not using that helper.
Would something like
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Global feature %d should have already been handled"), feature);
work for you? I'd drop the comment of course.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks! Pushed after squashing in the virReportError() calls and tweaking the error message slightly. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Ján Tomko