9:08 a.m.
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(a)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
10:20 a.m.
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
2:07 a.m.
New subject: [PATCH v2] 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.
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(a)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
3:05 a.m.
New subject: [PATCH v2] qemu: fix qemuMigrationCapability enum
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(a)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
1:53 a.m.
New subject: [PATCH v3] 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.
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(a)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
3:07 a.m.
New subject: [PATCH v2] qemu: fix qemuMigrationCapability enum
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(a)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
7
days inactive
13
days old
5 comments
3 participants
participants (3)
-
Dmitry Frolov -
Jiri Denemark -
Peter Krempa