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 <fro...@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
>
