[libvirt] [PATCH] Fix /domain/features setting in qemuParseCommandLine
Commit 5e6ce1 moved down detection of the ACPI feature in qemuParseCommandLine. However, when ACPI is detected, it clears all feature flags in def->features to only set ACPI. This used to be fine because this was the first place were def->features was set, but after the move this is no longer necessarily true because this block comes before the ACPI check: if (strstr(def->emulator, "kvm")) { def->virtType = VIR_DOMAIN_VIRT_KVM; def->features |= (1 << VIR_DOMAIN_FEATURE_PAE); } Since def is allocated in qemuParseCommandLine using VIR_ALLOC, we can always use |= when modifying def->features --- This is an issue I noticed while reading this code for other reasons, I have only compile-tested it and I'm not sure how to test it. Christophe src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6549f57..0065e83 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7547,7 +7547,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; if (STREQ(def->os.arch, "i686")||STREQ(def->os.arch, "x86_64")) - def->features = (1 << VIR_DOMAIN_FEATURE_ACPI) + def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI) /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/; #define WANT_VALUE() \ const char *val = progargv[++i]; \ -- 1.7.10.4
On 10.07.2012 12:14, Christophe Fergeau wrote:
Commit 5e6ce1 moved down detection of the ACPI feature in qemuParseCommandLine. However, when ACPI is detected, it clears all feature flags in def->features to only set ACPI. This used to be fine because this was the first place were def->features was set, but after the move this is no longer necessarily true because this block comes before the ACPI check:
if (strstr(def->emulator, "kvm")) { def->virtType = VIR_DOMAIN_VIRT_KVM; def->features |= (1 << VIR_DOMAIN_FEATURE_PAE); }
Since def is allocated in qemuParseCommandLine using VIR_ALLOC, we can always use |= when modifying def->features ---
This is an issue I noticed while reading this code for other reasons, I have only compile-tested it and I'm not sure how to test it.
Christophe
src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6549f57..0065e83 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7547,7 +7547,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory;
if (STREQ(def->os.arch, "i686")||STREQ(def->os.arch, "x86_64")) - def->features = (1 << VIR_DOMAIN_FEATURE_ACPI) + def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI) /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/; #define WANT_VALUE() \ const char *val = progargv[++i]; \
Cool, so now we don't overwrite PAE feature set just a few lines above (not visible in this diff though). ACK with this squashed in: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml b/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml index e07c1f6..8abcb51 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml @@ -8,6 +8,9 @@ <type arch='i686' machine='pc'>hvm</type> <boot dev='network'/> </os> + <features> + <pae/> + </features> <clock offset='utc'> <timer name='kvmclock' present='no'/> </clock> Michal
On Tue, Jul 10, 2012 at 04:04:12PM +0200, Michal Privoznik wrote:
Cool, so now we don't overwrite PAE feature set just a few lines above (not visible in this diff though).
ACK with this squashed in:
I have now pushed this patch with the testcase update. Before pushing I have tested that without my patch the test fails, and after applying it the test succeeds. Christophe
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml b/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml index e07c1f6..8abcb51 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml @@ -8,6 +8,9 @@ <type arch='i686' machine='pc'>hvm</type> <boot dev='network'/> </os> + <features> + <pae/> + </features> <clock offset='utc'> <timer name='kvmclock' present='no'/> </clock>
Michal
participants (2)
-
Christophe Fergeau -
Michal Privoznik