[PATCH 0/2] qemu: Build fixes following merge of mapped-ram
Patches to fix the recent build failures caused by merging the mapped-ram series. They get a green light from a CI run in my fork https://gitlab.com/jfehlig/libvirt/-/pipelines/1727301916 I would push them as build-breakers, but the first one does raise a question that should be answered beforehand IMO. See the individual patches for more commentary. Also, feel free to push if the patches are fine and it's outside of MDT timezone working hours :-). Jim Fehlig (2): qemu: Fix CLang build qemu: Fix build on 32-bit platforms src/qemu/qemu_migration_params.c | 2 +- src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_process.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) -- 2.43.0
Commit f0169f4d caused a build failure with CLang due to potentially uninitialized variable ../src/qemu/qemu_migration_params.c:825:17: error: variable 'nchannels' is used uninitialized whenever '&&' condition is false [-Werror,-Wsometimes-uninitialized] 825 | if (params && virTypedParamsGetInt(params, nparams, | ^~~~~~ ../src/qemu/qemu_migration_params.c:830:17: note: uninitialized use occurs here 830 | if (nchannels < 1) { | ^~~~~~~~~ Initialize the number of channels to 1, the default value when the number of channels is not specified. Fixes: f0169f4d6ce3915cf70bc3e21fa874369e22c840 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- Is it valid to specify '--parallel' without '--parallel-channels'? If so, should the number of channels default to 1? src/qemu/qemu_migration_params.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index b696b0d13e..f8b9e2bd7e 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -820,7 +820,7 @@ qemuMigrationParamsForSave(virTypedParameterPtr params, return NULL; if (flags & VIR_DOMAIN_SAVE_PARALLEL) { - int nchannels; + int nchannels = 1; if (params && virTypedParamsGetInt(params, nparams, VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, -- 2.43.0
On Thu, Mar 20, 2025 at 04:16:59PM -0600, Jim Fehlig via Devel wrote:
Commit f0169f4d caused a build failure with CLang due to potentially uninitialized variable
../src/qemu/qemu_migration_params.c:825:17: error: variable 'nchannels' is used uninitialized whenever '&&' condition is false [-Werror,-Wsometimes-uninitialized] 825 | if (params && virTypedParamsGetInt(params, nparams, | ^~~~~~ ../src/qemu/qemu_migration_params.c:830:17: note: uninitialized use occurs here 830 | if (nchannels < 1) { | ^~~~~~~~~
Initialize the number of channels to 1, the default value when the number of channels is not specified.
Fixes: f0169f4d6ce3915cf70bc3e21fa874369e22c840 Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
Is it valid to specify '--parallel' without '--parallel-channels'? If so, should the number of channels default to 1?
I've posted a patch series addressing this issue by removing --parallel option completely including the VIR_DOMAIN_SAVE_PARALLEL flag. IMHO there is no need to have --parallel option and VIR_DOMAIN_SAVE_PARALLEL flag, for more details see the reasoning in the specific patches. Pavel
src/qemu/qemu_migration_params.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index b696b0d13e..f8b9e2bd7e 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -820,7 +820,7 @@ qemuMigrationParamsForSave(virTypedParameterPtr params, return NULL;
if (flags & VIR_DOMAIN_SAVE_PARALLEL) { - int nchannels; + int nchannels = 1;
if (params && virTypedParamsGetInt(params, nparams, VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, -- 2.43.0
Commits c2518f7bc7 and 28a0621528 introduced build failures on 32-bit platforms by using incorrect format specifiers with g_strdup_printf. In one case, an 'unsigned long' format specifier is used with a 'long long int' variable. Fix by changing the format specifier to 'uintmax_t', and casting the variable likewise. In a second case, an 'unsigned long' format specifier is used with a 'size_t' variable, which is 'unsigned int' on 32-bit systems. Fix by changing the format specifier to use the 'z' modifier. Fixes: c2518f7bc7dd4f8ab8655a12ec3a000e1eb5b232 Fixes: 28a06215280b99708ed8dc2d183f62ba7b34ccf8 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- A lot of ways to skin this cat, but I think this would be the preferred approach. And while adding the 2 'Fixes:' lines to the commit message, I questioned whether this should be 2 patches. src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_process.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7610d43e6c..6249dc8299 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2259,8 +2259,8 @@ qemuMonitorMigrateToFdSet(virDomainObj *vm, qemuFDPassAddFD(fdPassMigrate, directFd, "-directio-fd"); qemuFDPassTransferMonitor(fdPassMigrate, mon); - uri = g_strdup_printf("file:%s,offset=%#lx", - qemuFDPassGetPath(fdPassMigrate), offset); + uri = g_strdup_printf("file:%s,offset=%#jx", + qemuFDPassGetPath(fdPassMigrate), (uintmax_t)offset); ret = qemuMonitorJSONMigrate(mon, flags, uri); return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c244895863..c3eeadfc3b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4917,7 +4917,7 @@ qemuProcessIncomingDefNew(virQEMUDriver *driver, } else { qemuFDPassAddFD(inc->fdPassMigrate, fd, "-buffered-fd"); } - inc->uri = g_strdup_printf("file:%s,offset=%#lx", + inc->uri = g_strdup_printf("file:%s,offset=%#zx", qemuFDPassGetPath(inc->fdPassMigrate), offset); } else { inc->uri = qemuMigrationDstGetURI(migrateFrom, *fd); -- 2.43.0
On Thu, Mar 20, 2025 at 04:17:00PM -0600, Jim Fehlig via Devel wrote:
Commits c2518f7bc7 and 28a0621528 introduced build failures on 32-bit platforms by using incorrect format specifiers with g_strdup_printf.
In one case, an 'unsigned long' format specifier is used with a 'long long int' variable. Fix by changing the format specifier to 'uintmax_t', and casting the variable likewise.
In a second case, an 'unsigned long' format specifier is used with a 'size_t' variable, which is 'unsigned int' on 32-bit systems. Fix by changing the format specifier to use the 'z' modifier.
Fixes: c2518f7bc7dd4f8ab8655a12ec3a000e1eb5b232 Fixes: 28a06215280b99708ed8dc2d183f62ba7b34ccf8 Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
A lot of ways to skin this cat, but I think this would be the preferred approach. And while adding the 2 'Fixes:' lines to the commit message, I questioned whether this should be 2 patches.
src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_process.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Jim Fehlig -
Pavel Hrdina