[libvirt] [PATCH 0/2] qemu: blockcopy: fix regression with block devices

Peter Krempa (2): qemu: blockcopy: Report error on image format detection failure qemu: blockcopy: Fix conditions when virStorageSource should be initialized src/qemu/qemu_driver.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) -- 2.23.0

We tolerate image format detection during block copy in very specific circumstances, but the code didn't error out on failure of the format detection. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 18bd0101e7..e43d6554a1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18104,15 +18104,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (!mirror_reuse) { mirror->format = disk->src->format; } else { - if (mirror_initialized && - virStorageSourceIsLocalStorage(mirror)) { - /* If the user passed the REUSE_EXT flag, then either they - * can also pass the RAW flag or use XML to tell us the format. - * So if we get here, we assume it is safe for us to probe the - * format from the file that we will be using. */ - mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user, - cfg->group); - } else { + /* If the user passed the REUSE_EXT flag, then either they + * can also pass the RAW flag or use XML to tell us the format. + * So if we get here, we assume it is safe for us to probe the + * format from the file that we will be using. */ + if (!mirror_initialized || + !virStorageSourceIsLocalStorage(mirror) || + (mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user, + cfg->group)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("reused mirror destination format must be specified")); goto endjob; -- 2.23.0

On Fri, Nov 29, 2019 at 02:43:59PM +0100, Peter Krempa wrote:
We tolerate image format detection during block copy in very specific circumstances, but the code didn't error out on failure of the format detection.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Commit 4b58fdf280a which enabled block copy also for network destinations needed to limit when the 'mirror' storage source is initialized in cases when we e.g. don't have an appropriate backend. Limiting it just to virStorageFileSupportsCreate is too restrictive as for example we can't precreate block devices and thus wouldn't initialize the 'mirror' but since it's a local source we'd try to examine it. This would fail since it wouldn't be initialized. Fix it by introducing a more granular check whether certain operations are supported and fix the check interlocks. https://bugzilla.redhat.com/show_bug.cgi?id=1778058 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e43d6554a1..962a4d3a55 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18009,7 +18009,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, qemuBlockJobDataPtr job = NULL; g_autoptr(virStorageSource) mirror = mirrorsrc; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - bool mirror_initialized = false; + bool supports_create = false; + bool supports_access = false; + bool supports_detect = false; g_autoptr(qemuBlockStorageSourceChainData) data = NULL; g_autoptr(qemuBlockStorageSourceChainData) crdata = NULL; virStorageSourcePtr n; @@ -18090,14 +18092,17 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, goto endjob; } - if (virStorageFileSupportsCreate(mirror) == 1) { + supports_access = virStorageFileSupportsAccess(mirror) == 1; + supports_create = virStorageFileSupportsCreate(mirror) == 1; + supports_detect = virStorageFileSupportsBackingChainTraversal(mirror) == 1; + + if (supports_access || supports_create || supports_detect) { if (qemuDomainStorageFileInit(driver, vm, mirror, NULL) < 0) goto endjob; - - mirror_initialized = true; } - if (qemuDomainBlockCopyValidateMirror(mirror, disk->dst, &existing) < 0) + if (supports_access && + qemuDomainBlockCopyValidateMirror(mirror, disk->dst, &existing) < 0) goto endjob; if (!mirror->format) { @@ -18108,7 +18113,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, * can also pass the RAW flag or use XML to tell us the format. * So if we get here, we assume it is safe for us to probe the * format from the file that we will be using. */ - if (!mirror_initialized || + if (!supports_detect || !virStorageSourceIsLocalStorage(mirror) || (mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user, cfg->group)) < 0) { @@ -18132,7 +18137,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* pre-create the image file. In case when 'blockdev' is used this is * required so that libvirt can properly label the image for access by qemu */ if (!existing) { - if (mirror_initialized) { + if (supports_create) { if (virStorageFileCreate(mirror) < 0) { virReportSystemError(errno, "%s", _("failed to create copy target")); goto endjob; -- 2.23.0

On Fri, Nov 29, 2019 at 02:44:00PM +0100, Peter Krempa wrote:
Commit 4b58fdf280a which enabled block copy also for network destinations needed to limit when the 'mirror' storage source is initialized in cases when we e.g. don't have an appropriate backend.
Limiting it just to virStorageFileSupportsCreate is too restrictive as for example we can't precreate block devices and thus wouldn't initialize the 'mirror' but since it's a local source we'd try to examine it. This would fail since it wouldn't be initialized.
Fix it by introducing a more granular check whether certain operations are supported and fix the check interlocks.
https://bugzilla.redhat.com/show_bug.cgi?id=1778058
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko
-
Peter Krempa