To express empty drive we historically use storage source with empty path. Unfortunately NBD disks may be declared without a path. Add a helper to wrap this logic. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 20 ++++++++++++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 22 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf4548..e819049 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1941,6 +1941,7 @@ virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; +virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 299edcd..ca8be7f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1976,6 +1976,26 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src) /** + * virStorageSourceIsEmpty: + * + * @src: disk source to check + * + * Returns true if @src points to an empty storage source. + */ +bool +virStorageSourceIsEmpty(virStorageSourcePtr src) +{ + if (virStorageSourceIsLocalStorage(src) && !src->path) + return true; + + if (src->type == VIR_STORAGE_TYPE_NONE) + return true; + + return false; +} + + +/** * virStorageSourceBackingStoreClear: * * @src: disk source to clear diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index eccbf4e..2583e10 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -353,6 +353,7 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); +bool virStorageSourceIsEmpty(virStorageSourcePtr src); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); -- 2.1.0

The formatted UUID isn't used anywhere else in qemuDomainCheckDiskStartupPolicy. Drop it. --- src/qemu/qemu_domain.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c0306d7..bd7d511 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2403,12 +2403,9 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, size_t diskIndex, bool cold_boot) { - char uuid[VIR_UUID_STRING_BUFLEN]; int startupPolicy = vm->def->disks[diskIndex]->startupPolicy; int device = vm->def->disks[diskIndex]->device; - virUUIDFormat(vm->def->uuid, uuid); - switch ((virDomainStartupPolicy) startupPolicy) { case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: /* Once started with an optional disk, qemu saves its section -- 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 | 22 +++++++++++++++------- src/storage/storage_driver.h | 3 ++- tests/virstoragetest.c | 2 +- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bd7d511..88c5d1b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2659,7 +2659,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..1963bd6 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 fail, virHashTablePtr cycle) { int ret = -1; @@ -2847,9 +2848,12 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, else backingStore->format = backingFormat; - if (virStorageFileGetMetadataRecurse(backingStore, - uid, gid, allow_probe, - cycle) < 0) { + if ((ret = virStorageFileGetMetadataRecurse(backingStore, + uid, gid, allow_probe, fail, + cycle)) < 0) { + if (fail) + goto cleanup; + /* if we fail somewhere midway, just accept and return a * broken chain */ ret = 0; @@ -2883,15 +2887,19 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * + * If @fail 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 fail) { - 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, fail=%d", + src->path, src->format, (int)uid, (int)gid, allow_probe, fail); virHashTablePtr cycle = NULL; int ret = -1; @@ -2903,7 +2911,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, fail, 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

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 | 26 ++------------------------ src/qemu/qemu_process.c | 5 ++--- 2 files changed, 4 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 88c5d1b..72638cf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2440,27 +2440,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, @@ -2490,8 +2469,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virFileExists(path)) continue; - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 && - qemuDiskChainCheckBroken(disk) >= 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0) continue; if (disk->startupPolicy && @@ -2660,7 +2638,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (virStorageFileGetMetadata(disk->src, uid, gid, cfg->allowDiskFormatProbing, - false) < 0) + true) < 0) ret = -1; cleanup: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 07335a0..a807f50 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3342,9 +3342,8 @@ qemuProcessReconnect(void *opaque) 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) - goto error; + ignore_value(qemuDomainDetermineDiskChain(driver, obj, + obj->def->disks[i], true)); dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = obj->def->disks[i]; -- 2.1.0

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 1963bd6..87f3c1c 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 fail, @@ -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, fail, cycle)) < 0) { if (fail) @@ -2910,7 +2920,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, fail, cycle); virHashFree(cycle); -- 2.1.0

Now that we have a simple function to check locality of storage, reuse it in qemuDomainCheckDiskPresence(). Also reuse check for empty storage source. --- src/qemu/qemu_domain.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72638cf..c9e8f89 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2453,20 +2453,18 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, for (i = vm->def->ndisks; i > 0; i--) { size_t idx = i - 1; virDomainDiskDefPtr disk = vm->def->disks[idx]; - const char *path = virDomainDiskGetSource(disk); virStorageFileFormat format = virDomainDiskGetFormat(disk); - virStorageType type = virStorageSourceGetActualType(disk->src); - if (!path) + if (virStorageSourceIsEmpty(disk->src)) continue; /* There is no need to check the backing chain for disks * without backing support, the fact that the file exists is * more than enough */ - if (type != VIR_STORAGE_TYPE_NETWORK && + if (virStorageSourceIsLocalStorage(disk->src) && format >= VIR_STORAGE_FILE_NONE && format < VIR_STORAGE_FILE_BACKING && - virFileExists(path)) + virFileExists(virDomainDiskGetSource(disk))) continue; if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0) -- 2.1.0