src/qemu/qemu_migration_params.c | 16 +++++++++++----- src/qemu/qemu_migration_params.h | 12 ++++++++++-- 2 files changed, 21 insertions(+), 7 deletions(-)
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.
Thus, all uninitialized .optinnal fields of the static array
qemuMigrationParamsFlagMap[] will be implicitly initialized with
value 0 (QEMU_MIGRATION_CAP_XBZRLE).
To my opinion, introducing a separate enum for optional capabilities,
would be a better solution.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
src/qemu/qemu_migration_params.c | 16 +++++++++++-----
src/qemu/qemu_migration_params.h | 12 ++++++++++--
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index c10660d6f2..23c463dbbb 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -104,6 +104,11 @@ VIR_ENUM_IMPL(qemuMigrationCapability,
"dirty-bitmaps",
"return-path",
"zero-copy-send",
+);
+
+VIR_ENUM_IMPL(qemuMigrationOptCap,
+ QEMU_MIGRATION_OPTCAP_LAST,
+ "none",
"postcopy-preempt",
"switchover-ack",
);
@@ -152,7 +157,7 @@ struct _qemuMigrationParamsFlagMapItem {
/* An optional capability to set in addition to @cap in case it is
* supported. Depending on @part either one or both sides of migration
* has to support the optional capability to be enabled. */
- qemuMigrationCapability optional;
+ qemuMigrationOptCap optional;
/* Bit-wise OR of qemuMigrationParty. Determines whether the capability has
* to be enabled on the source, on the destination, or on both sides of
* migration. */
@@ -200,7 +205,7 @@ static const qemuMigrationParamsFlagMapItem qemuMigrationParamsFlagMap[] = {
{.match = QEMU_MIGRATION_FLAG_REQUIRED,
.flag = VIR_MIGRATE_POSTCOPY,
.cap = QEMU_MIGRATION_CAP_POSTCOPY,
- .optional = QEMU_MIGRATION_CAP_POSTCOPY_PREEMPT,
+ .optional = QEMU_MIGRATION_OPTCAP_POSTCOPY_PREEMPT,
.party = QEMU_MIGRATION_SOURCE | QEMU_MIGRATION_DESTINATION},
{.match = QEMU_MIGRATION_FLAG_REQUIRED,
@@ -211,7 +216,7 @@ static const qemuMigrationParamsFlagMapItem qemuMigrationParamsFlagMap[] = {
{.match = QEMU_MIGRATION_FLAG_FORBIDDEN,
.flag = VIR_MIGRATE_TUNNELLED,
.cap = QEMU_MIGRATION_CAP_RETURN_PATH,
- .optional = QEMU_MIGRATION_CAP_SWITCHOVER_ACK,
+ .optional = QEMU_MIGRATION_OPTCAP_SWITCHOVER_ACK,
.party = QEMU_MIGRATION_SOURCE | QEMU_MIGRATION_DESTINATION},
{.match = QEMU_MIGRATION_FLAG_REQUIRED,
@@ -725,8 +730,9 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
qemuMigrationCapabilityTypeToString(item->cap));
ignore_value(virBitmapSetBit(migParams->caps, item->cap));
- if (item->optional) {
- qemuMigrationCapability opt = item->optional;
+ if (item->optional > QEMU_MIGRATION_OPTCAP_NONE &&
+ item->optional < QEMU_MIGRATION_OPTCAP_LAST) {
+ qemuMigrationOptCap opt = item->optional;
ignore_value(virBitmapSetBit(migParams->optional, opt));
if (item->party != party)
ignore_value(virBitmapSetBit(migParams->remoteOptional, opt));
diff --git a/src/qemu/qemu_migration_params.h b/src/qemu/qemu_migration_params.h
index 17fc63f527..3246b8487e 100644
--- a/src/qemu/qemu_migration_params.h
+++ b/src/qemu/qemu_migration_params.h
@@ -40,13 +40,21 @@ typedef enum {
QEMU_MIGRATION_CAP_BLOCK_DIRTY_BITMAPS,
QEMU_MIGRATION_CAP_RETURN_PATH,
QEMU_MIGRATION_CAP_ZERO_COPY_SEND,
- QEMU_MIGRATION_CAP_POSTCOPY_PREEMPT,
- QEMU_MIGRATION_CAP_SWITCHOVER_ACK,
QEMU_MIGRATION_CAP_LAST
} qemuMigrationCapability;
VIR_ENUM_DECL(qemuMigrationCapability);
+typedef enum {
+ QEMU_MIGRATION_OPTCAP_NONE,
+ QEMU_MIGRATION_OPTCAP_POSTCOPY_PREEMPT,
+ QEMU_MIGRATION_OPTCAP_SWITCHOVER_ACK,
+
+ QEMU_MIGRATION_OPTCAP_LAST
+} qemuMigrationOptCap;
+VIR_ENUM_DECL(qemuMigrationOptCap);
+
+
typedef enum {
QEMU_MIGRATION_PARAM_COMPRESS_LEVEL,
QEMU_MIGRATION_PARAM_COMPRESS_THREADS,
--
2.34.1On Tue, May 06, 2025 at 17:08:18 +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.
> Thus, all uninitialized .optinnal fields of the static array
> qemuMigrationParamsFlagMap[] will be implicitly initialized with
> value 0 (QEMU_MIGRATION_CAP_XBZRLE).
Heh, funny.
> To my opinion, introducing a separate enum for optional capabilities,
> would be a better solution.
First, the bug does not actually cause any issues in real world as it
only means QEMU_MIGRATION_CAP_XBZRLE can never be used as an optional
feature. And it isn't used that way anywhere.
Your solution is not fixing any real bug while breaking a lot of stuff.
Just grep for QEMU_MIGRATION_CAP_LAST in the code and you'll see several
places broken by this patch.
Since the optional capability is always converted to a bitmap when
non-zero, we could just as well try to specify it as such right away.
It's just an idea that came to my mind when looking at this patch. I
haven't really checked whether it's reasonably doable :-)
Jirka
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.1On 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
>
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
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© 2016 - 2025 Red Hat, Inc.