[libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available

[Eduardo Habkost]

On Fri, Aug 03, 2018 at 01:59:47PM +0100, Daniel P. Berrangé wrote:
> It is increasingly likely that some distro is going to change the
> default "x86" machine type in QEMU from "pc" to "q35". This will
> certainly break existing applications which write their XML on the
> assumption that its using a "pc" machine by default. For example they'll
> lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> radically.
> 
> Libvirt promises to isolate applications from hypervisor changes that
> may cause incompatibilities, so we must ensure that we always use the
> "pc" machine type if it is available. Only use QEMU's own reported
> default machine type if "pc" does not exist.
> 
> Note this change assumes there will always be a "pc" alias as long as a
> versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> machine type but not provide the "pc" alias, it is too hard to decide
> which to default so. Versioned machine types are supposed to be
> considered opaque strings, so we can't apply any sensible ordering
> ourselves and QEMU isn't reporting the list of machines in any sensible
> ordering itself.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>

Won't this break qemuParseCommandLine() if it sees a QEMU binary
running without "-machine"?  It will assume the QEMU default is
"pc" but this may be not true.


> ---
>  src/qemu/qemu_capabilities.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 0fb800589a..9eb58afef3 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2242,6 +2242,17 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
>      int ret = -1;
>      size_t i;
>      size_t defIdx = 0;
> +    ssize_t preferredIdx = -1;
> +    const char *preferredAlias = NULL;
> +
> +    /* Historically QEMU defaulted to 'pc' machine type but in
> +     * future might switch 'q35'. Such a change is considered
> +     * an ABI break from lbivirt's POV, so we must be sure to
> +     * keep 'pc' as default machine no matter what QEMU says.
> +     */
> +    if (qemuCaps->arch == VIR_ARCH_X86_64 ||
> +        qemuCaps->arch == VIR_ARCH_I686)
> +        preferredAlias = "pc";
>  
>      if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0)
>          return -1;
> @@ -2263,12 +2274,16 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
>          mach->maxCpus = machines[i]->maxCpus;
>          mach->hotplugCpus = machines[i]->hotplugCpus;
>  
> +        if (STREQ_NULLABLE(mach->alias, preferredAlias))
> +            preferredIdx = qemuCaps->nmachineTypes - 1;
> +

Isn't STREQ_NULLABLE(NULL, NULL) true?  You don't want to set
preferredIdx here if preferredAlias==NULL.

>          if (machines[i]->isDefault)
>              defIdx = qemuCaps->nmachineTypes - 1;
>      }
>  
> -    if (defIdx)
> -        virQEMUCapsSetDefaultMachine(qemuCaps, defIdx);
> +    if (preferredIdx == -1)
> +        preferredIdx = defIdx;
> +    virQEMUCapsSetDefaultMachine(qemuCaps, preferredIdx);
>  
>      ret = 0;
>  
> -- 
> 2.17.1

-- 
Eduardo