When preparing to do a blockcopy, the mirror image is modified so
that QEMU can access it. For instance, the mirror has seclabels
set, if it is a NVMe disk it is detached from the host and so on.
And usually, the restore is done upon successful finish of the
blockcopy operation. But, if something fails then we need to
explicitly revoke the access to the mirror image (and thus
reattach NVMe disk back to the host).

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538

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

diff to v1:
- Fix call of qemuDomainStorageSourceChainAccessRevoke() so that it is
called even if data = crdata = NULL.

 src/qemu/qemu_driver.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31f199fdef..dfe0adaad8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
     virDomainDiskDefPtr disk = NULL;
     int ret = -1;
     bool need_unlink = false;
+    bool need_revoke = false;
     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
     const char *format = NULL;
     bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
@@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 
     if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0)
         goto endjob;
+    need_revoke = true;
 
     if (blockdev) {
         if (mirror_reuse) {
@@ -18232,14 +18234,17 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 
  endjob:
     if (ret < 0 &&
-        virDomainObjIsActive(vm) &&
-        (data || crdata)) {
-        qemuDomainObjEnterMonitor(driver, vm);
-        if (data)
-            qemuBlockStorageSourceChainDetach(priv->mon, data);
-        if (crdata)
-            qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]);
-        ignore_value(qemuDomainObjExitMonitor(driver, vm));
+        virDomainObjIsActive(vm)) {
+        if (data || crdata) {
+            qemuDomainObjEnterMonitor(driver, vm);
+            if (data)
+                qemuBlockStorageSourceChainDetach(priv->mon, data);
+            if (crdata)
+                qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]);
+            ignore_value(qemuDomainObjExitMonitor(driver, vm));
+        }
+        if (need_revoke)
+            qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror);
     }
     if (need_unlink && virStorageFileUnlink(mirror) < 0)
         VIR_WARN("%s", _("unable to remove just-created copy target"));
-- 
2.25.3

On Thu, Apr 16, 2020 at 09:42:46AM +0200, Michal Privoznik wrote:
> When preparing to do a blockcopy, the mirror image is modified so
> that QEMU can access it. For instance, the mirror has seclabels
> set, if it is a NVMe disk it is detached from the host and so on.
> And usually, the restore is done upon successful finish of the
> blockcopy operation. But, if something fails then we need to
> explicitly revoke the access to the mirror image (and thus
> reattach NVMe disk back to the host).
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> diff to v1:
> - Fix call of qemuDomainStorageSourceChainAccessRevoke() so that it is
> called even if data = crdata = NULL.
> 
>  src/qemu/qemu_driver.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 31f199fdef..dfe0adaad8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>      virDomainDiskDefPtr disk = NULL;
>      int ret = -1;
>      bool need_unlink = false;
> +    bool need_revoke = false;
>      g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>      const char *format = NULL;
>      bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
> @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  
>      if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0)
>          goto endjob;
> +    need_revoke = true;
>  
>      if (blockdev) {
>          if (mirror_reuse) {
> @@ -18232,14 +18234,17 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  
>   endjob:
>      if (ret < 0 &&
> -        virDomainObjIsActive(vm) &&
> -        (data || crdata)) {
> -        qemuDomainObjEnterMonitor(driver, vm);
> -        if (data)
> -            qemuBlockStorageSourceChainDetach(priv->mon, data);
> -        if (crdata)
> -            qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]);
> -        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        virDomainObjIsActive(vm)) {
> +        if (data || crdata) {
> +            qemuDomainObjEnterMonitor(driver, vm);
> +            if (data)
> +                qemuBlockStorageSourceChainDetach(priv->mon, data);
> +            if (crdata)
> +                qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]);
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        }
> +        if (need_revoke)
> +            qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror);
>      }
>      if (need_unlink && virStorageFileUnlink(mirror) < 0)
>          VIR_WARN("%s", _("unable to remove just-created copy target"));
> -- 
> 2.25.3
> 

Reviewed-by: Pavel Mores <pmores@redhat.com>