[libvirt PATCH] drivers: Group global feature together

Andrea Bolognani posted 1 patch 2 years, 8 months ago
Failed in applying to current master (apply log)
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(-)
[libvirt PATCH] drivers: Group global feature together
Posted by Andrea Bolognani 2 years, 8 months ago
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

Re: [libvirt PATCH] drivers: Group global feature together
Posted by Ján Tomko 2 years, 8 months ago
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;

Re: [libvirt PATCH] drivers: Group global feature together
Posted by Andrea Bolognani 2 years, 8 months ago
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