[libvirt] [PATCHv2 0/4] Improve backing store error reporting
New version incorporates some review feedback from John. The changes were borderline-trivial so I've reposted the series. Patch 2/4 is new. Peter Krempa (4): util: storage: Allow metadata crawler to report useful errors qemu: Sanitize argument names and empty disk check in qemuDomainDetermineDiskChain qemu: Report better errors from broken backing chains storage: Improve error message when traversing backing chains src/qemu/qemu_domain.c | 36 +++++++----------------------------- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 11 +++++++---- src/security/virt-aa-helper.c | 2 +- src/storage/storage_driver.c | 42 +++++++++++++++++++++++++++++++----------- src/storage/storage_driver.h | 3 ++- tests/virstoragetest.c | 2 +- 9 files changed, 57 insertions(+), 54 deletions(-) -- 2.1.0
Add a new parameter to virStorageFileGetMetadata that will break the backing chain detection process and report useful error message rather than having to use virStorageFileChainGetBroken. This patch just introduces the option, usage will be provided separately. --- src/qemu/qemu_domain.c | 3 ++- src/security/virt-aa-helper.c | 2 +- src/storage/storage_driver.c | 24 +++++++++++++++++------- src/storage/storage_driver.h | 3 ++- tests/virstoragetest.c | 2 +- 5 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5859ba7..19b935d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2693,7 +2693,8 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (virStorageFileGetMetadata(disk->src, uid, gid, - cfg->allowDiskFormatProbing) < 0) + cfg->allowDiskFormatProbing, + false) < 0) ret = -1; cleanup: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a06ba44..9afc8db 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -932,7 +932,7 @@ get_files(vahControl * ctl) */ if (!disk->src->backingStore) { bool probe = ctl->allowDiskFormatProbing; - virStorageFileGetMetadata(disk->src, -1, -1, probe); + virStorageFileGetMetadata(disk->src, -1, -1, probe, false); } /* XXX passing ignoreOpenFailure = true to get back to the behavior diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 433d7b7..c3b29c4 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2783,6 +2783,7 @@ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, uid_t uid, gid_t gid, bool allow_probe, + bool report_broken, virHashTablePtr cycle) { int ret = -1; @@ -2847,9 +2848,13 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, else backingStore->format = backingFormat; - if (virStorageFileGetMetadataRecurse(backingStore, - uid, gid, allow_probe, - cycle) < 0) { + if ((ret = virStorageFileGetMetadataRecurse(backingStore, + uid, gid, + allow_probe, report_broken, + cycle)) < 0) { + if (report_broken) + goto cleanup; + /* if we fail somewhere midway, just accept and return a * broken chain */ ret = 0; @@ -2883,15 +2888,20 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * + * If @report_broken is true, the whole function fails with a possibly sane + * error instead of just returning a broken chain. + * * Caller MUST free result after use via virStorageSourceFree. */ int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, - bool allow_probe) + bool allow_probe, + bool report_broken) { - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - src->path, src->format, (int)uid, (int)gid, allow_probe); + VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d, report_broken=%d", + src->path, src->format, (int)uid, (int)gid, + allow_probe, report_broken); virHashTablePtr cycle = NULL; int ret = -1; @@ -2903,7 +2913,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src, src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; ret = virStorageFileGetMetadataRecurse(src, uid, gid, - allow_probe, cycle); + allow_probe, report_broken, cycle); virHashFree(cycle); return ret; diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index e773928..54a17a3 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -50,7 +50,8 @@ bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, - bool allow_probe) + bool allow_probe, + bool fail) ATTRIBUTE_NONNULL(1); int virStorageTranslateDiskSourcePool(virConnectPtr conn, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e2ee3ff..29f5c7a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -119,7 +119,7 @@ testStorageFileGetMetadata(const char *path, if (VIR_STRDUP(ret->path, path) < 0) goto error; - if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0) + if (virStorageFileGetMetadata(ret, uid, gid, allow_probe, false) < 0) goto error; return ret; -- 2.1.0
On 09/18/2014 05:54 AM, Peter Krempa wrote:
Add a new parameter to virStorageFileGetMetadata that will break the backing chain detection process and report useful error message rather than having to use virStorageFileChainGetBroken.
This patch just introduces the option, usage will be provided separately. --- src/qemu/qemu_domain.c | 3 ++- src/security/virt-aa-helper.c | 2 +- src/storage/storage_driver.c | 24 +++++++++++++++++------- src/storage/storage_driver.h | 3 ++- tests/virstoragetest.c | 2 +- 5 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5859ba7..19b935d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2693,7 +2693,8 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
if (virStorageFileGetMetadata(disk->src, uid, gid, - cfg->allowDiskFormatProbing) < 0) + cfg->allowDiskFormatProbing, + false) < 0) ret = -1;
cleanup: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a06ba44..9afc8db 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -932,7 +932,7 @@ get_files(vahControl * ctl) */ if (!disk->src->backingStore) { bool probe = ctl->allowDiskFormatProbing; - virStorageFileGetMetadata(disk->src, -1, -1, probe); + virStorageFileGetMetadata(disk->src, -1, -1, probe, false); }
/* XXX passing ignoreOpenFailure = true to get back to the behavior diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 433d7b7..c3b29c4 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2783,6 +2783,7 @@ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, uid_t uid, gid_t gid, bool allow_probe, + bool report_broken, virHashTablePtr cycle) { int ret = -1; @@ -2847,9 +2848,13 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, else backingStore->format = backingFormat;
- if (virStorageFileGetMetadataRecurse(backingStore, - uid, gid, allow_probe, - cycle) < 0) { + if ((ret = virStorageFileGetMetadataRecurse(backingStore, + uid, gid, + allow_probe, report_broken, + cycle)) < 0) { + if (report_broken) + goto cleanup; + /* if we fail somewhere midway, just accept and return a * broken chain */ ret = 0; @@ -2883,15 +2888,20 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * + * If @report_broken is true, the whole function fails with a possibly sane + * error instead of just returning a broken chain. + * * Caller MUST free result after use via virStorageSourceFree. */ int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, - bool allow_probe) + bool allow_probe, + bool report_broken) { - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - src->path, src->format, (int)uid, (int)gid, allow_probe); + VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d, report_broken=%d", + src->path, src->format, (int)uid, (int)gid, + allow_probe, report_broken);
virHashTablePtr cycle = NULL; int ret = -1; @@ -2903,7 +2913,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src, src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
ret = virStorageFileGetMetadataRecurse(src, uid, gid, - allow_probe, cycle); + allow_probe, report_broken, cycle);
virHashFree(cycle); return ret; diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index e773928..54a17a3 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -50,7 +50,8 @@ bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src);
int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, - bool allow_probe) + bool allow_probe, + bool fail)
s/fail/report_broken ACK John
ATTRIBUTE_NONNULL(1);
int virStorageTranslateDiskSourcePool(virConnectPtr conn, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e2ee3ff..29f5c7a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -119,7 +119,7 @@ testStorageFileGetMetadata(const char *path, if (VIR_STRDUP(ret->path, path) < 0) goto error;
- if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0) + if (virStorageFileGetMetadata(ret, uid, gid, allow_probe, false) < 0) goto error;
return ret;
Reuse virStorageSourceIsEmpty and rename "force" argument to "force_probe". --- src/qemu/qemu_domain.c | 8 +++----- src/qemu/qemu_domain.h | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 19b935d..515bcac 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2670,20 +2670,18 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - bool force) + bool force_probe) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = 0; uid_t uid; gid_t gid; - int type = virStorageSourceGetActualType(disk->src); - if (type != VIR_STORAGE_TYPE_NETWORK && - !disk->src->path) + if (virStorageSourceIsEmpty(disk->src)) goto cleanup; if (disk->src->backingStore) { - if (force) + if (force_probe) virStorageSourceBackingStoreClear(disk->src); else goto cleanup; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4ae2c57..494e9f8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -366,7 +366,7 @@ int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - bool force); + bool force_probe); int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.1.0
On 09/18/2014 05:54 AM, Peter Krempa wrote:
Reuse virStorageSourceIsEmpty and rename "force" argument to "force_probe". --- src/qemu/qemu_domain.c | 8 +++----- src/qemu/qemu_domain.h | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-)
ACK - thanks :-) John
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 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); } 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); } 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*/ + if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], + true, false) < 0) goto error; dev.type = VIR_DOMAIN_DEVICE_DISK; -- 2.1.0
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;
Report also the name of the parent file and uid/gid used to access it to help debugging broken storage configurations. --- src/storage/storage_driver.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c3b29c4..7c518bf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2781,6 +2781,7 @@ virStorageFileChown(virStorageSourcePtr src, /* Recursive workhorse for virStorageFileGetMetadata. */ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + virStorageSourcePtr parent, uid_t uid, gid_t gid, bool allow_probe, bool report_broken, @@ -2805,9 +2806,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, return -1; if (virStorageFileAccess(src, F_OK) < 0) { - virReportSystemError(errno, - _("Cannot access backing file %s"), - src->path); + if (src == parent) { + virReportSystemError(errno, + _("Cannot access storage file '%s' " + "(as uid:%d, gid:%d)"), + src->path, (int)uid, (int)gid); + } else { + virReportSystemError(errno, + _("Cannot access backing file '%s' " + "of storage file '%s' (as uid:%d, gid:%d)"), + src->path, parent->path, (int)uid, (int)gid); + } + goto cleanup; } @@ -2848,7 +2858,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, else backingStore->format = backingFormat; - if ((ret = virStorageFileGetMetadataRecurse(backingStore, + if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent, uid, gid, allow_probe, report_broken, cycle)) < 0) { @@ -2912,7 +2922,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (src->format <= VIR_STORAGE_FILE_NONE) src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(src, uid, gid, + ret = virStorageFileGetMetadataRecurse(src, src, uid, gid, allow_probe, report_broken, cycle); virHashFree(cycle); -- 2.1.0
On 09/18/2014 05:54 AM, Peter Krempa wrote:
Report also the name of the parent file and uid/gid used to access it to help debugging broken storage configurations. --- src/storage/storage_driver.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
ACK John
On 09/23/14 22:35, John Ferlan wrote:
On 09/18/2014 05:54 AM, Peter Krempa wrote:
Report also the name of the parent file and uid/gid used to access it to help debugging broken storage configurations. --- src/storage/storage_driver.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
ACK
Thanks. I've fixed the nits you've pointed out and pushed this series. Peter
participants (2)
-
John Ferlan -
Peter Krempa