We don't support all startup policies with all source types so to
correctly allow switching from a 'file' based cdrom with 'optional'
startup policy to a 'block' based one which doesn't support optional we
must update the startup policy field first. Obviously we need to have
fallback if the update fails.

Reported-by: Vojtech Juranek <vjuranek@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_driver.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bd41ddbc3c..dfc27572c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7038,6 +7038,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
 {
     virDomainDiskDef *disk = dev->data.disk;
     virDomainDiskDef *orig_disk = NULL;
+    virDomainStartupPolicy origStartupPolicy;
     virDomainDeviceDef oldDev = { .type = dev->type };

     if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) {
@@ -7047,6 +7048,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
     }

     oldDev.data.disk = orig_disk;
+    origStartupPolicy = orig_disk->startupPolicy;
     if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
                                      VIR_DOMAIN_DEVICE_ACTION_UPDATE,
                                      true) < 0)
@@ -7065,13 +7067,20 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
             return -1;
         }

+        /* update startup policy first before updating disk image */
+        orig_disk->startupPolicy = dev->data.disk->startupPolicy;
+
         if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
-                                           dev->data.disk->src, force) < 0)
+                                           dev->data.disk->src, force) < 0) {
+            /* revert startup policy before failing */
+            orig_disk->startupPolicy = origStartupPolicy;
             return -1;
+        }

         dev->data.disk->src = NULL;
     }

+    /* in case when we aren't updating disk source we update startup policy here */
     orig_disk->startupPolicy = dev->data.disk->startupPolicy;
     orig_disk->snapshot = dev->data.disk->snapshot;

-- 
2.31.1

On 9/10/21 3:35 PM, Peter Krempa wrote:
> We don't support all startup policies with all source types so to
> correctly allow switching from a 'file' based cdrom with 'optional'
> startup policy to a 'block' based one which doesn't support optional we
> must update the startup policy field first. Obviously we need to have
> fallback if the update fails.
> 
> Reported-by: Vojtech Juranek <vjuranek@redhat.com>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

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

Michal

On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa@redhat.com> wrote:
>
> We don't support all startup policies with all source types so to
> correctly allow switching from a 'file' based cdrom with 'optional'
> startup policy to a 'block' based one which doesn't support optional we
> must update the startup policy field first. Obviously we need to have
> fallback if the update fails.

Why is there a difference between file and block for startup policy?
Is this documented?

Do we have a way to switch from file to block with current libvirt
(centos 8 stream, rhel 8.4) without this patch, or do we need to wait
until this fix is available? (rhel 8.5?)

> Reported-by: Vojtech Juranek <vjuranek@redhat.com>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index bd41ddbc3c..dfc27572c4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7038,6 +7038,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
>  {
>      virDomainDiskDef *disk = dev->data.disk;
>      virDomainDiskDef *orig_disk = NULL;
> +    virDomainStartupPolicy origStartupPolicy;
>      virDomainDeviceDef oldDev = { .type = dev->type };
>
>      if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) {
> @@ -7047,6 +7048,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
>      }
>
>      oldDev.data.disk = orig_disk;
> +    origStartupPolicy = orig_disk->startupPolicy;
>      if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
>                                       VIR_DOMAIN_DEVICE_ACTION_UPDATE,
>                                       true) < 0)
> @@ -7065,13 +7067,20 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
>              return -1;
>          }
>
> +        /* update startup policy first before updating disk image */
> +        orig_disk->startupPolicy = dev->data.disk->startupPolicy;
> +
>          if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
> -                                           dev->data.disk->src, force) < 0)
> +                                           dev->data.disk->src, force) < 0) {
> +            /* revert startup policy before failing */
> +            orig_disk->startupPolicy = origStartupPolicy;
>              return -1;
> +        }
>
>          dev->data.disk->src = NULL;
>      }
>
> +    /* in case when we aren't updating disk source we update startup policy here */
>      orig_disk->startupPolicy = dev->data.disk->startupPolicy;
>      orig_disk->snapshot = dev->data.disk->snapshot;
>
> --
> 2.31.1
>