[PATCH] qemu: fix qemuMigrationCapability enum
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.1
On 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.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
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
participants (3)
-
Dmitry Frolov -
Jiri Denemark -
Peter Krempa