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@xxxxxxxxxx> > >> --- > >> Â 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.
