1. Patchew
  2. Libvirt
  3. View series

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

Thomas Huth posted 1 patch 3 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200805102226.18158-1-thuth@redhat.com
src/qemu/qemu_validate.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
[PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
Posted by Thomas Huth 3 years, 8 months ago 
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.
Note: The error is only generated if the timer is marked with
present="yes" - otherwise we would suddenly refuse XML definitions
that worked without problems before.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2: Check also for timer->present == 1

 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..561e7b12c7 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) && timer->present == 1) {
+                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
Re: [PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
Posted by Daniel Henrique Barboza 3 years, 8 months ago 

On 8/5/20 7:22 AM, 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.
> Note: The error is only generated if the timer is marked with
> present="yes" - otherwise we would suddenly refuse XML definitions
> that worked without problems before.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


>   v2: Check also for timer->present == 1
> 
>   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..561e7b12c7 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) && timer->present == 1) {
> +                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;
>   
>
Re: [PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
Posted by Boris Fiuczynski 3 years, 8 months ago 
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

On 8/5/20 12:22 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.
> Note: The error is only generated if the timer is marked with
> present="yes" - otherwise we would suddenly refuse XML definitions
> that worked without problems before.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2: Check also for timer->present == 1
> 
>   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..561e7b12c7 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) && timer->present == 1) {
> +                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;
>   
> 


-- 
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
Re: [PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
Posted by Thomas Huth 3 years, 7 months ago 
On 05/08/2020 12.22, 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.
> Note: The error is only generated if the timer is marked with
> present="yes" - otherwise we would suddenly refuse XML definitions
> that worked without problems before.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2: Check also for timer->present == 1
> 
>  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..561e7b12c7 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) && timer->present == 1) {
> +                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;

Ping! The patch got two Reviewed-bys, so I guess it should be fine now?
Could somebody please pick it up?

Thanks,
 Thomas
Re: [PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
Posted by Daniel P. Berrangé 3 years, 7 months ago 
On Wed, Sep 02, 2020 at 06:23:32PM +0200, Thomas Huth wrote:
> On 05/08/2020 12.22, 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.
> > Note: The error is only generated if the timer is marked with
> > present="yes" - otherwise we would suddenly refuse XML definitions
> > that worked without problems before.
> > 
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  v2: Check also for timer->present == 1
> > 
> >  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..561e7b12c7 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) && timer->present == 1) {
> > +                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;
> 
> Ping! The patch got two Reviewed-bys, so I guess it should be fine now?
> Could somebody please pick it up?

I've pushed


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
Posted by Michal Privoznik 3 years, 7 months ago 
On 8/5/20 12:22 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.
> Note: The error is only generated if the timer is marked with
> present="yes" - otherwise we would suddenly refuse XML definitions
> that worked without problems before.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2: Check also for timer->present == 1
> 
>   src/qemu/qemu_validate.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)

Terribly sorry for letting this slip.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

and pushed now.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.