[PATCH] qemu: Do not silently allow non-available timers on non-x86 systems

Peter Krempa pkrempa at redhat.com
Fri Jul 31 07:32:35 UTC 2020


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 at 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.




More information about the libvir-list mailing list