[v2] qemu: fix qemuMigrationCapability enum

[PATCH v2] qemu: fix qemuMigrationCapability enum

Posted by Dmitry Frolov via Devel 9 months, 1 week ago

Enum variable of type qemuMigrationCapability is checked for zero in
src/qemu/qemu_migration_params.c:729.

"if (item->optional) { ..."

Actualy, QEMU_MIGRATION_CAP_XBZRLE enum constant has value 0.
So, at least, the condition is incorrect.

v1: introducing a separate enum for optional capabilities
v2: another approach: fix only the incorrect condition

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 src/qemu/qemu_migration_params.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index c10660d6f2..98d314cf2d 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -700,6 +700,12 @@ qemuMigrationParamsSetBlockDirtyBitmapMapping(qemuMigrationParams *migParams,
         ignore_value(virBitmapClearBit(migParams->caps, QEMU_MIGRATION_CAP_BLOCK_DIRTY_BITMAPS));
 }
 
+static bool
+qemuMigrationCapabilityIsOptional(qemuMigrationCapability cap)
+{
+    return cap == QEMU_MIGRATION_CAP_POSTCOPY_PREEMPT ||
+           cap == QEMU_MIGRATION_CAP_SWITCHOVER_ACK;
+}
 
 qemuMigrationParams *
 qemuMigrationParamsFromFlags(virTypedParameterPtr params,
@@ -725,7 +731,7 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
                       qemuMigrationCapabilityTypeToString(item->cap));
             ignore_value(virBitmapSetBit(migParams->caps, item->cap));
 
-            if (item->optional) {
+            if (qemuMigrationCapabilityIsOptional(item->optional)) {
                 qemuMigrationCapability opt = item->optional;
                 ignore_value(virBitmapSetBit(migParams->optional, opt));
                 if (item->party != party)
-- 
2.34.1

Re: [PATCH v2] qemu: fix qemuMigrationCapability enum

Posted by Peter Krempa via Devel 9 months, 1 week ago

On Wed, May 07, 2025 at 10:07:41 +0300, Dmitry Frolov via Devel wrote:
> Enum variable of type qemuMigrationCapability is checked for zero in
> src/qemu/qemu_migration_params.c:729.
> 
> "if (item->optional) { ..."
> 
> Actualy, QEMU_MIGRATION_CAP_XBZRLE enum constant has value 0.
> So, at least, the condition is incorrect.
> 
> v1: introducing a separate enum for optional capabilities
> v2: another approach: fix only the incorrect condition
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>  src/qemu/qemu_migration_params.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
> index c10660d6f2..98d314cf2d 100644
> --- a/src/qemu/qemu_migration_params.c
> +++ b/src/qemu/qemu_migration_params.c
> @@ -700,6 +700,12 @@ qemuMigrationParamsSetBlockDirtyBitmapMapping(qemuMigrationParams *migParams,
>          ignore_value(virBitmapClearBit(migParams->caps, QEMU_MIGRATION_CAP_BLOCK_DIRTY_BITMAPS));
>  }
>  
> +static bool
> +qemuMigrationCapabilityIsOptional(qemuMigrationCapability cap)
> +{
> +    return cap == QEMU_MIGRATION_CAP_POSTCOPY_PREEMPT ||
> +           cap == QEMU_MIGRATION_CAP_SWITCHOVER_ACK;
> +}

This makes the code much more fragile for exactly 0 benefit.

Arguably yes QEMU_MIGRATION_CAP_XBZRLE can't be used, but it isn't used
either.

I'm not really persuaded that it is a problem worth changing the code
for (e.g. just docum,ent at the definition of
qemuMigrationParamsFlagMap[] that QEMU_MIGRATION_CAP_XBZRLE must not be
used in the current state.

Alternative is to initialize all members of the array where the optional
field is not used to QEMU_MIGRATION_CAP_LAST. Yes it's more lines and
yes it will need to be documented as well.

Please do not make the code worse just to appease a static code
validator's false positive.

>  qemuMigrationParams *
>  qemuMigrationParamsFromFlags(virTypedParameterPtr params,
> @@ -725,7 +731,7 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
>                        qemuMigrationCapabilityTypeToString(item->cap));
>              ignore_value(virBitmapSetBit(migParams->caps, item->cap));
>  
> -            if (item->optional) {

and check that item->optional != QEMU_MIGRATION_CAP_LAST

> +            if (qemuMigrationCapabilityIsOptional(item->optional)) {
>                  qemuMigrationCapability opt = item->optional;
>                  ignore_value(virBitmapSetBit(migParams->optional, opt));
>                  if (item->party != party)
> -- 
> 2.34.1
>

Re: [PATCH v2] qemu: fix qemuMigrationCapability enum

Posted by Jiri Denemark via Devel 9 months, 1 week ago

On Wed, May 07, 2025 at 10:07:41 +0300, Dmitry Frolov wrote:
> Enum variable of type qemuMigrationCapability is checked for zero in
> src/qemu/qemu_migration_params.c:729.
> 
> "if (item->optional) { ..."
> 
> Actualy, QEMU_MIGRATION_CAP_XBZRLE enum constant has value 0.
> So, at least, the condition is incorrect.
> 
> v1: introducing a separate enum for optional capabilities
> v2: another approach: fix only the incorrect condition
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>  src/qemu/qemu_migration_params.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
> index c10660d6f2..98d314cf2d 100644
> --- a/src/qemu/qemu_migration_params.c
> +++ b/src/qemu/qemu_migration_params.c
> @@ -700,6 +700,12 @@ qemuMigrationParamsSetBlockDirtyBitmapMapping(qemuMigrationParams *migParams,
>          ignore_value(virBitmapClearBit(migParams->caps, QEMU_MIGRATION_CAP_BLOCK_DIRTY_BITMAPS));
>  }
>  
> +static bool
> +qemuMigrationCapabilityIsOptional(qemuMigrationCapability cap)
> +{
> +    return cap == QEMU_MIGRATION_CAP_POSTCOPY_PREEMPT ||
> +           cap == QEMU_MIGRATION_CAP_SWITCHOVER_ACK;
> +}
>  
>  qemuMigrationParams *
>  qemuMigrationParamsFromFlags(virTypedParameterPtr params,
> @@ -725,7 +731,7 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
>                        qemuMigrationCapabilityTypeToString(item->cap));
>              ignore_value(virBitmapSetBit(migParams->caps, item->cap));
>  
> -            if (item->optional) {
> +            if (qemuMigrationCapabilityIsOptional(item->optional)) {
>                  qemuMigrationCapability opt = item->optional;
>                  ignore_value(virBitmapSetBit(migParams->optional, opt));
>                  if (item->party != party)

The issue was that item->optional is initialized to a value that matches
one of the capabilities and this patch doesn't do anything with it. The
check whether item->optional is non-zero works fine so changing just
this check does not make any sense.

Jirka

[PATCH v3] qemu: fix qemuMigrationCapability enum

Posted by Dmitry Frolov via Devel 9 months ago

Enum variable of type qemuMigrationCapability is checked for zero in
src/qemu/qemu_migration_params.c:729.

"if (item->optional) { ..."

Actualy, QEMU_MIGRATION_CAP_XBZRLE enum constant has value 0.
So, at least, the condition is incorrect.

Adding QEMU_MIGRATION_CAP_NONE == 0 to enum has several advantages:
- less invasive
- allows comparing with 0
- this approach is wide used in libvirt
- no need to document anything
and only one disadvantage:
- 0-th bit will be reserved (won`t be used) in the corresponding bitmaps.

v1: introducing a separate enum for optional capabilities
v2: another approach: fix only the incorrect condition
v3: third way: add 0-th constanat to enum

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 src/qemu/qemu_migration_params.c | 1 +
 src/qemu/qemu_migration_params.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index c10660d6f2..ad6ab04a4b 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -92,6 +92,7 @@ VIR_ENUM_IMPL(qemuMigrationCompressMethod,
 
 VIR_ENUM_IMPL(qemuMigrationCapability,
               QEMU_MIGRATION_CAP_LAST,
+              "none",
               "xbzrle",
               "auto-converge",
               "rdma-pin-all",
diff --git a/src/qemu/qemu_migration_params.h b/src/qemu/qemu_migration_params.h
index 17fc63f527..60e6413b3c 100644
--- a/src/qemu/qemu_migration_params.h
+++ b/src/qemu/qemu_migration_params.h
@@ -28,6 +28,7 @@
 #include "virenum.h"
 
 typedef enum {
+    QEMU_MIGRATION_CAP_NONE,
     QEMU_MIGRATION_CAP_XBZRLE,
     QEMU_MIGRATION_CAP_AUTO_CONVERGE,
     QEMU_MIGRATION_CAP_RDMA_PIN_ALL,
-- 
2.34.1