[libvirt] [PATCHv2 3/4] qemu: Report better errors from broken backing chains
John Ferlan
jferlan at redhat.com
Tue Sep 23 20:34:14 UTC 2014
On 09/18/2014 05:54 AM, Peter Krempa wrote:
> Request erroring out from the backing chain traveller and drop qemu's
> internal backing chain integrity tester.
>
> The backin chain traveller reports errors by itself with possibly more
s/backin/backing
> detail than qemuDiskChainCheckBroken ever could.
>
> We also need to make sure that we reconnect to existing qemu instances
> even at the cost of losing the backing chain info (this really should be
> stored in the XML rather than reloaded from disk, but that needs some
> work).
> ---
> src/qemu/qemu_domain.c | 29 ++++-------------------------
> src/qemu/qemu_domain.h | 3 ++-
> src/qemu/qemu_driver.c | 10 +++++-----
> src/qemu/qemu_hotplug.c | 2 +-
> src/qemu/qemu_process.c | 11 +++++++----
> 5 files changed, 19 insertions(+), 36 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 515bcac..b93e0d5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2476,27 +2476,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
> return -1;
> }
>
> -static int
> -qemuDiskChainCheckBroken(virDomainDiskDefPtr disk)
> -{
> - char *brokenFile = NULL;
> -
> - if (!virDomainDiskGetSource(disk))
> - return 0;
> -
> - if (virStorageFileChainGetBroken(disk->src, &brokenFile) < 0)
> - return -1;
> -
> - if (brokenFile) {
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("Backing file '%s' of image '%s' is missing."),
> - brokenFile, virDomainDiskGetSource(disk));
> - VIR_FREE(brokenFile);
> - return -1;
> - }
> -
> - return 0;
> -}
>
> int
> qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> @@ -2524,8 +2503,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> virFileExists(virDomainDiskGetSource(disk)))
> continue;
>
> - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 &&
> - qemuDiskChainCheckBroken(disk) >= 0)
> + if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) >= 0)
> continue;
>
> if (disk->startupPolicy &&
> @@ -2670,7 +2648,8 @@ int
> qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk,
> - bool force_probe)
> + bool force_probe,
> + bool report_broken)
> {
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> int ret = 0;
> @@ -2692,7 +2671,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> if (virStorageFileGetMetadata(disk->src,
> uid, gid,
> cfg->allowDiskFormatProbing,
> - false) < 0)
> + report_broken) < 0)
> ret = -1;
>
> cleanup:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 494e9f8..cb0115a 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -366,7 +366,8 @@ int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk,
> - bool force_probe);
> + bool force_probe,
> + bool report_broken);
>
> int qemuDomainStorageFileInit(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5fd89a3..d0aff1a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6688,7 +6688,7 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn,
> if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
> goto end;
>
> - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> + if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
> goto end;
>
> switch (disk->device) {
> @@ -13067,7 +13067,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
> for (i = 0; i < snap->def->ndisks; i++) {
> if (snap->def->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> continue;
> - qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], true);
> + qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], true, true);
We want to return failure immediately, but do nothing about it? At the
very least an ignore_value()
> }
> if (orig_err) {
> virSetError(orig_err);
> @@ -14875,7 +14875,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
> oldsrc = disk->src;
> disk->src = disk->mirror;
>
> - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> + if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
> goto cleanup;
>
> if (disk->mirror->format &&
> @@ -15388,7 +15388,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
> goto endjob;
> }
>
> - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> + if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
> goto endjob;
>
> if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) &&
> @@ -15757,7 +15757,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
> disk->dst);
> goto endjob;
> }
> - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> + if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
> goto endjob;
>
> if (!top)
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7bc19cd..d631887 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -779,7 +779,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> if (qemuSetUnprivSGIO(dev) < 0)
> goto end;
>
> - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> + if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
> goto end;
>
> switch (disk->device) {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8853d27..0f2a34f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1090,7 +1090,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> - qemuDomainDetermineDiskChain(driver, vm, disk, true);
> + qemuDomainDetermineDiskChain(driver, vm, disk, true, true);
Same here... We want to report_broken, but do nothing about it. At least
an ignore_value().
> } else if (disk->mirror &&
> (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)) {
> @@ -3428,9 +3428,12 @@ qemuProcessReconnect(void *opaque)
> if (virStorageTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0)
> goto error;
>
> - /* XXX we should be able to restore all data from XML in the future */
> - if (qemuDomainDetermineDiskChain(driver, obj,
> - obj->def->disks[i], true) < 0)
> + /* XXX we should be able to restore all data from XML in the future.
> + * This should be the only place that calls qemuDomainDetermineDiskChain
> + * with @report_broken == false to guarantee best-effort domain
> + * reconnect*/
NIT: Add space between 't' and '*'
ACK with the nits.
John
> + if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i],
> + true, false) < 0)
> goto error;
>
> dev.type = VIR_DOMAIN_DEVICE_DISK;
>
More information about the libvir-list
mailing list