src/qemu/qemu_validate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
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.
© 2016 - 2024 Red Hat, Inc.