[PATCH] qemuProcessSetupDisksTransientHotplug: skip system_reset when the all disks have transient shareBacking option

[Peter Krempa]

On Tue, Jul 06, 2021 at 21:44:34 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> 
> When both <on_reboot>destroy</on_reboot/> and <transient shareBacking='yes'>
> is set to the domain xml, the guest doesn't start.
> 
> # virsh start Guest
> error: Failed to start domain 'Guest'
> error: internal error: qemu unexpectedly closed the monitor
> #
> 
> That's because libvirt does system_reset qmp command when 
> <transient shareBacking='yes'> is set, and the qemu is launched
> with '-no-reboot' option, so system_reset goes to shutdown.
> 
> The boot order issue happens only if the guest has not only the
> transient share backing disks but also disks which isn't set the
> option.
> 
> Let's skip system_reset when the all disks has the option.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> ---
>  src/qemu/qemu_process.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)




> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2b03b0ab98..82dba64e21 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7058,7 +7058,8 @@ qemuProcessSetupDisksTransientHotplug(virDomainObj *vm,
>                                        qemuDomainAsyncJob asyncJob)
>  {
>      qemuDomainObjPrivate *priv = vm->privateData;
> -    bool hasHotpluggedDisk = false;
> +    bool needReset = true;
> +    size_t attached = 0;
>      size_t i;
>  
>      for (i = 0; i < vm->def->ndisks; i++) {
> @@ -7071,12 +7072,24 @@ qemuProcessSetupDisksTransientHotplug(virDomainObj *vm,
>          if (qemuDomainAttachDiskGeneric(priv->driver, vm, domdisk, asyncJob) < 0)
>              return -1;
>  
> -        hasHotpluggedDisk = true;
> +        attached++;
> +    }
> +
> +    /* system_reset is needed in case the domain also has the other disks (doesn't
> +     * have <transient shareBacking='yes'/> option) to fix the boot order. */
> +    if (!attached || vm->def->ndisks == attached)
> +        needReset = false;
> +
> +    if (needReset && priv->allowReboot == VIR_TRISTATE_BOOL_NO) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Doesn't support this configuration. "
> +                         "Change the domain xml to allow rebooting the guest."));

Note that errors here are a very unfortunate case. Everything should be
pre-checked before we attempt to start the VM.

> +        return -1;
>      }
>  
>      /* in order to allow booting from such disks we need to issue a system-reset
>       * so that the firmware tables recording bootable devices are regerated */
> -    if (hasHotpluggedDisk) {
> +    if (needReset) {
>          int rc;
>  
>          if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0)

NACK, this doesn't actually fix what it intends to properly and probably
breaks other things.

The system reset is needed so that the ACPI tables are generated
properly, this will work accidentally with one disk, but not when you
have multiple boot devices.

What is (unfortunately) required is a special case to ignore the reset
event when starting up the VM before it was actually started in case we
have disk which will cause a system reset.