[PATCH] qemu: Do not silently allow non-available timers on non-x86 systems
libvirt currently silently allows <timer name="kvmclock"/> and some other timer tags in the guest XML definition for timers that do not exist on non-x86 systems. We should not silently ignore these tags since the users might not get what they expected otherwise. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887 Signed-off-by: Thomas Huth <thuth@redhat.com> --- src/qemu/qemu_validate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..667ac5cc23 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -371,6 +371,18 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_TSC: case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: + if (!ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Configuring the '%s' timer is not supported " + "for virtType=%s arch=%s machine=%s guests"), + virDomainTimerNameTypeToString(timer->name), + virDomainVirtTypeToString(def->virtType), + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_TIMER_NAME_LAST: break; -- 2.18.1
On 7/22/20 1:21 PM, Thomas Huth wrote:
libvirt currently silently allows <timer name="kvmclock"/> and some other timer tags in the guest XML definition for timers that do not exist on non-x86 systems. We should not silently ignore these tags since the users might not get what they expected otherwise.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887 Signed-off-by: Thomas Huth <thuth@redhat.com> --- src/qemu/qemu_validate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..667ac5cc23 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -371,6 +371,18 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_TSC: case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: + if (!ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Configuring the '%s' timer is not supported " + "for virtType=%s arch=%s machine=%s guests"), + virDomainTimerNameTypeToString(timer->name), + virDomainVirtTypeToString(def->virtType), + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_TIMER_NAME_LAST: break;
This would render previously as valid accepted domains invalid, e.g. on s390x using kvmclock: As long as the user does not specify the "present" attribute the domain starts without error since qemus cpu parameter is not extended. The feedback of all other archs would be good to have. @Daniel: What's your opinion? -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 28/07/2020 17.13, Boris Fiuczynski wrote:
On 7/22/20 1:21 PM, Thomas Huth wrote:
libvirt currently silently allows <timer name="kvmclock"/> and some other timer tags in the guest XML definition for timers that do not exist on non-x86 systems. We should not silently ignore these tags since the users might not get what they expected otherwise.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887 Signed-off-by: Thomas Huth <thuth@redhat.com> --- src/qemu/qemu_validate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..667ac5cc23 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -371,6 +371,18 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_TSC: case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: + if (!ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Configuring the '%s' timer is not supported " + "for virtType=%s arch=%s machine=%s guests"), + virDomainTimerNameTypeToString(timer->name), + virDomainVirtTypeToString(def->virtType), + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_TIMER_NAME_LAST: break;
This would render previously as valid accepted domains invalid, e.g. on s390x using kvmclock: As long as the user does not specify the "present" attribute the domain starts without error since qemus cpu parameter is not extended.
Shall I turn it into a VIR_WARN() instead? Thomas
On Fri, Jul 31, 2020 at 08:05:23 +0200, Thomas Huth wrote:
On 28/07/2020 17.13, Boris Fiuczynski wrote:
On 7/22/20 1:21 PM, Thomas Huth wrote:
libvirt currently silently allows <timer name="kvmclock"/> and some other timer tags in the guest XML definition for timers that do not exist on non-x86 systems. We should not silently ignore these tags since the users might not get what they expected otherwise.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887 Signed-off-by: Thomas Huth <thuth@redhat.com> --- src/qemu/qemu_validate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..667ac5cc23 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -371,6 +371,18 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_TSC: case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: + if (!ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Configuring the '%s' timer is not supported " + "for virtType=%s arch=%s machine=%s guests"), + virDomainTimerNameTypeToString(timer->name), + virDomainVirtTypeToString(def->virtType), + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_TIMER_NAME_LAST: break;
This would render previously as valid accepted domains invalid, e.g. on s390x using kvmclock: As long as the user does not specify the "present" attribute the domain starts without error since qemus cpu parameter is not extended.
Shall I turn it into a VIR_WARN() instead?
No, a VIR_WARN is basically useless. It just spams the logs. This function can reject previously accepted configs in cases they were wrong and could not work before. Said that we should take care when doing so and do it in really justified scenarios e.g. when user requests to enable something which definitely will not work, but not e.g. when disabling that same thing.
participants (3)
-
Boris Fiuczynski -
Peter Krempa -
Thomas Huth