[libvirt] [PATCH v2 0/3] ensure diskchain is not broken on guest bootup
v1->v2: Add a helper function qemuDiskChainCheckBroken to check diskchain after invoking qemuDomainDetermineDiskChain() This patchset try to check whether the diskchain is broken or not when domain boot up. If dischain is broken, report like: virsh start rhel6qcow2 error: Failed to start domain rhel6qcow2 error: invalid argument: Backing file '/var/lib/libvirt/images/thirddisk' of \ image '/var/lib/libvirt/images/thirddisk.snap3' is missing. For storage pool, it still can list volumes out even if their diskchain is broken Guannan Ren(3) qemu: refactor qemuDomainCheckDiskPresence for only disk presence check qemu: add helper functions for diskchain checking qemu: check presence of each disk and its backing file as well src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------- src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_process.c | 6 ---- src/util/virstoragefile.c | 47 +++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 6 files changed, 141 insertions(+), 51 deletions(-)
Refactor this function to make it focus on disk presence checking, including diskchain checking, and not only for CDROM and Floppy. This change is good for the following patches. --- src/qemu/qemu_domain.c | 98 +++++++++++++++++++++++++++++--------------------- 1 file changed, 58 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index da3b768..03a2aa6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2026,6 +2026,61 @@ cleanup: virObjectUnref(cfg); } +static int +qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + bool cold_boot) +{ + char uuid[VIR_UUID_STRING_BUFLEN]; + virDomainEventPtr event = NULL; + int startupPolicy = disk->startupPolicy; + + virUUIDFormat(vm->def->uuid, uuid); + + switch ((enum virDomainStartupPolicy) startupPolicy) { + case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: + break; + + case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: + virReportSystemError(errno, + _("cannot access file '%s'"), + disk->src); + goto error; + break; + + case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: + if (cold_boot) { + virReportSystemError(errno, + _("cannot access file '%s'"), + disk->src); + goto error; + } + break; + + case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: + case VIR_DOMAIN_STARTUP_POLICY_LAST: + /* this should never happen */ + break; + } + + VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " + "due to inaccessible source '%s'", + disk->dst, vm->def->name, uuid, disk->src); + + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias, + VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START); + if (event) + qemuDomainEventQueue(driver, event); + + VIR_FREE(disk->src); + + return 0; + +error: + return -1; +} + int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -2034,12 +2089,8 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, int ret = -1; size_t i; virDomainDiskDefPtr disk; - char uuid[VIR_UUID_STRING_BUFLEN]; - virDomainEventPtr event = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virUUIDFormat(vm->def->uuid, uuid); - for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i]; @@ -2053,42 +2104,9 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, continue; } - switch ((enum virDomainStartupPolicy) disk->startupPolicy) { - case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: - break; - - case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); - goto cleanup; - break; - - case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: - if (cold_boot) { - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); - goto cleanup; - } - break; - - case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: - case VIR_DOMAIN_STARTUP_POLICY_LAST: - /* this should never happen */ - break; - } - - VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " - "due to inaccessible source '%s'", - disk->dst, vm->def->name, uuid, disk->src); - - event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias, - VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START); - if (event) - qemuDomainEventQueue(driver, event); - - VIR_FREE(disk->src); + if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) < 0) + goto cleanup; } ret = 0; -- 1.8.3.1
On 07/26/2013 02:37 PM, Guannan Ren wrote:
Refactor this function to make it focus on disk presence checking, including diskchain checking, and not only for CDROM and Floppy. This change is good for the following patches. --- src/qemu/qemu_domain.c | 98 +++++++++++++++++++++++++++++--------------------- 1 file changed, 58 insertions(+), 40 deletions(-)
Previous ACK still stands, you addressed my only point. Martin
*src/util/virstoragefile.c: Add a helper function to get the first name of missing backing files, if the name is NULL, it means the diskchain is not broken. *src/qemu/qemu_domain.c: qemuDiskChainCheckBroken(disk) to check if its chain is broken --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 22 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/util/virstoragefile.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 5 files changed, 75 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9615ea..f7166d6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1882,6 +1882,7 @@ virSocketAddrSetPort; # util/virstoragefile.h +virStorageFileChainGetBroken; virStorageFileChainLookup; virStorageFileFeatureTypeFromString; virStorageFileFeatureTypeToString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 03a2aa6..be77991 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2187,6 +2187,28 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, } int +qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) +{ + char *brokenFile = NULL; + + if (!disk->src || !disk->backingChain) + return 0; + + if (virStorageFileChainGetBroken(disk->backingChain, &brokenFile) < 0) + return -1; + + if (brokenFile) { + virReportError(VIR_ERR_INVALID_ARG, + _("Backing file '%s' of image '%s' is missing."), + brokenFile, disk->src); + VIR_FREE(brokenFile); + return -1; + } + + return 0; +} + +int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9a959d6..0a4a51e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -347,6 +347,9 @@ bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, bool start_with_state); + +int qemuDiskChainCheckBroken(virDomainDiskDefPtr disk); + int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb6df91..e02e28a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; } + if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } + if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -1097,6 +1104,46 @@ virStorageFileGetMetadata(const char *path, int format, } /** + * virStorageFileChainCheckBroken + * + * Check whether CHAIN is broken, return the broken file name if yes, + * otherwise return NULL + */ +int +virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **brokenFile) +{ + virStorageFileMetadataPtr tmp; + char *file = NULL; + int ret = -1; + + if (!chain) + return 0; + + *brokenFile = NULL; + + tmp = chain; + while (tmp) { + /* No backing store or backing store is not file */ + if (!tmp->backingStoreRaw) + break; + if (!tmp->backingStore) { + if (VIR_STRDUP(file, tmp->backingStoreRaw) < 0) + goto error; + break; + } + tmp = tmp->backingMeta; + } + + *brokenFile = file; + ret = 0; + +error: + return ret; +} + + +/** * virStorageFileFreeMetadata: * * Free pointers in passed structure and structure itself. diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4cb47e6..1f89839 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -90,6 +90,8 @@ virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format); +int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **broken_file); const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, -- 1.8.3.1
On 07/26/2013 02:37 PM, Guannan Ren wrote:
*src/util/virstoragefile.c: Add a helper function to get the first name of missing backing files, if the name is NULL, it means the diskchain is not broken. *src/qemu/qemu_domain.c: qemuDiskChainCheckBroken(disk) to check if its chain is broken --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 22 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/util/virstoragefile.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 5 files changed, 75 insertions(+)
[...]
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb6df91..e02e28a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; }
+ if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } +
As mentioned before, the pre-existing problem here is that the disk chain will be reported as broken in case root doesn't have access to the folder the file is in (for example). Could we somehow pass a function or seclabel to fallback to (from the driver)? Just a hypothetical question, no need to fix that in this series.
if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -1097,6 +1104,46 @@ virStorageFileGetMetadata(const char *path, int format, }
/** + * virStorageFileChainCheckBroken + * + * Check whether CHAIN is broken, return the broken file name if yes, + * otherwise return NULL + */
This doesn't cope with the functionality, probably missed it when reworking the function.
+int +virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **brokenFile) +{ + virStorageFileMetadataPtr tmp; + char *file = NULL;
no need to have this variable, you can use bokenFile all the way (probably leftover before some rework). ACK with those two things fixed. Both this and the previous (I forgot to mention it there) should go in after 1.1.1 release. Even though it fixes an error, it mainly prepares the field for other patches and I'd rather wait with this. Don't hesitate to disagree though, we can discuss that on IRC with other in case there's huge need for that. Martin
On 07/29/2013 07:27 PM, Martin Kletzander wrote:
On 07/26/2013 02:37 PM, Guannan Ren wrote:
*src/util/virstoragefile.c: Add a helper function to get the first name of missing backing files, if the name is NULL, it means the diskchain is not broken. *src/qemu/qemu_domain.c: qemuDiskChainCheckBroken(disk) to check if its chain is broken --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 22 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/util/virstoragefile.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 5 files changed, 75 insertions(+)
[...]
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb6df91..e02e28a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; }
+ if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } + As mentioned before, the pre-existing problem here is that the disk chain will be reported as broken in case root doesn't have access to the folder the file is in (for example). Could we somehow pass a function or seclabel to fallback to (from the driver)? Just a hypothetical question, no need to fix that in this series.
yeah, right, I will try to resolve it in separate patch.
if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -1097,6 +1104,46 @@ virStorageFileGetMetadata(const char *path, int format, }
/** + * virStorageFileChainCheckBroken + * + * Check whether CHAIN is broken, return the broken file name if yes, + * otherwise return NULL + */
This doesn't cope with the functionality, probably missed it when reworking the function.
+int +virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **brokenFile) +{ + virStorageFileMetadataPtr tmp; + char *file = NULL; no need to have this variable, you can use bokenFile all the way (probably leftover before some rework).
ACK with those two things fixed. Both this and the previous (I forgot to mention it there) should go in after 1.1.1 release. Even though it fixes an error, it mainly prepares the field for other patches and I'd rather wait with this. Don't hesitate to disagree though, we can discuss that on IRC with other in case there's huge need for that.
Martin
Thanks for the review. I will send a v3 version with these fixed
*src/util/virstoragefile.c: Add a helper function to get the first name of missing backing files, if the name is NULL, it means the diskchain is not broken. *src/qemu/qemu_domain.c: qemuDiskChainCheckBroken(disk) to check if its chain is broken --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 22 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/util/virstoragefile.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 5 files changed, 74 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9615ea..f7166d6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1882,6 +1882,7 @@ virSocketAddrSetPort; # util/virstoragefile.h +virStorageFileChainGetBroken; virStorageFileChainLookup; virStorageFileFeatureTypeFromString; virStorageFileFeatureTypeToString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 03a2aa6..be77991 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2187,6 +2187,28 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, } int +qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) +{ + char *brokenFile = NULL; + + if (!disk->src || !disk->backingChain) + return 0; + + if (virStorageFileChainGetBroken(disk->backingChain, &brokenFile) < 0) + return -1; + + if (brokenFile) { + virReportError(VIR_ERR_INVALID_ARG, + _("Backing file '%s' of image '%s' is missing."), + brokenFile, disk->src); + VIR_FREE(brokenFile); + return -1; + } + + return 0; +} + +int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9a959d6..0a4a51e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -347,6 +347,9 @@ bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, bool start_with_state); + +int qemuDiskChainCheckBroken(virDomainDiskDefPtr disk); + int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb6df91..0b9cec3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; } + if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } + if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -1097,6 +1104,45 @@ virStorageFileGetMetadata(const char *path, int format, } /** + * virStorageFileChainCheckBroken + * + * If CHAIN is broken, set *brokenFile to the broken file name, + * otherwise set it to NULL. Caller MUST free *brokenFile after use. + * Return 0 on success, negative on error. + */ +int +virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **brokenFile) +{ + virStorageFileMetadataPtr tmp; + int ret = -1; + + if (!chain) + return 0; + + *brokenFile = NULL; + + tmp = chain; + while (tmp) { + /* Break if no backing store or backing store is not file */ + if (!tmp->backingStoreRaw) + break; + if (!tmp->backingStore) { + if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) + goto error; + break; + } + tmp = tmp->backingMeta; + } + + ret = 0; + +error: + return ret; +} + + +/** * virStorageFileFreeMetadata: * * Free pointers in passed structure and structure itself. diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4cb47e6..1f89839 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -90,6 +90,8 @@ virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format); +int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **broken_file); const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, -- 1.8.3.1
On Mon 29 Jul 2013 02:51:15 PM CEST, Guannan Ren wrote:
*src/util/virstoragefile.c: Add a helper function to get the first name of missing backing files, if the name is NULL, it means the diskchain is not broken. *src/qemu/qemu_domain.c: qemuDiskChainCheckBroken(disk) to check if its chain is broken --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 22 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/util/virstoragefile.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 5 files changed, 74 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9615ea..f7166d6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1882,6 +1882,7 @@ virSocketAddrSetPort;
# util/virstoragefile.h +virStorageFileChainGetBroken; virStorageFileChainLookup; virStorageFileFeatureTypeFromString; virStorageFileFeatureTypeToString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 03a2aa6..be77991 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2187,6 +2187,28 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, }
int +qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) +{ + char *brokenFile = NULL; + + if (!disk->src || !disk->backingChain) + return 0; + + if (virStorageFileChainGetBroken(disk->backingChain, &brokenFile) < 0) + return -1; + + if (brokenFile) { + virReportError(VIR_ERR_INVALID_ARG, + _("Backing file '%s' of image '%s' is missing."), + brokenFile, disk->src); + VIR_FREE(brokenFile); + return -1; + } + + return 0; +} + +int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9a959d6..0a4a51e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -347,6 +347,9 @@ bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, bool start_with_state); + +int qemuDiskChainCheckBroken(virDomainDiskDefPtr disk); + int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb6df91..0b9cec3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; }
+ if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } + if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -1097,6 +1104,45 @@ virStorageFileGetMetadata(const char *path, int format, }
/** + * virStorageFileChainCheckBroken + * + * If CHAIN is broken, set *brokenFile to the broken file name, + * otherwise set it to NULL. Caller MUST free *brokenFile after use. + * Return 0 on success, negative on error. + */ +int +virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **brokenFile) +{ + virStorageFileMetadataPtr tmp; + int ret = -1; + + if (!chain) + return 0; + + *brokenFile = NULL; + + tmp = chain; + while (tmp) { + /* Break if no backing store or backing store is not file */ + if (!tmp->backingStoreRaw) + break; + if (!tmp->backingStore) { + if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) + goto error; + break; + } + tmp = tmp->backingMeta; + } + + ret = 0; + +error: + return ret; +} + + +/** * virStorageFileFreeMetadata: * * Free pointers in passed structure and structure itself. diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4cb47e6..1f89839 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -90,6 +90,8 @@ virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format); +int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, + char **broken_file);
const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start,
ACK, Martin
For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error on chain issue. --- src/qemu/qemu_domain.c | 21 ++++++++++++--------- src/qemu/qemu_process.c | 6 ------ 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be77991..b607454 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2091,22 +2091,25 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + VIR_DEBUG("Checking for disk presence"); for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i]; - if (!disk->startupPolicy || !disk->src) + if (!disk->src) continue; - if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ - continue; + if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0) + if (qemuDiskChainCheckBroken(disk) >= 0) + continue; + + if (disk->startupPolicy) { + virResetLastError(); + if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0) + continue; } - if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, - cold_boot) < 0) - goto cleanup; + goto cleanup; } ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e8e459e..61a897c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3621,16 +3621,10 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) goto cleanup; - VIR_DEBUG("Checking for CDROM and floppy presence"); if (qemuDomainCheckDiskPresence(driver, vm, flags & VIR_QEMU_PROCESS_START_COLD) < 0) goto cleanup; - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], - false) < 0) - goto cleanup; - } /* Get the advisory nodeset from numad if 'placement' of * either <vcpu> or <numatune> is 'auto'. -- 1.8.3.1
On 07/26/2013 02:37 PM, Guannan Ren wrote:
For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error on chain issue. --- src/qemu/qemu_domain.c | 21 ++++++++++++--------- src/qemu/qemu_process.c | 6 ------ 2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be77991..b607454 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2091,22 +2091,25 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ VIR_DEBUG("Checking for disk presence"); for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i];
- if (!disk->startupPolicy || !disk->src) + if (!disk->src) continue;
- if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ - continue; + if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0) + if (qemuDiskChainCheckBroken(disk) >= 0) + continue;
Multiline if(), this should be: if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 && qemuDiskChainCheckBroken(disk) >= 0) continue; Other than that, this for is very badly readable and every time I'm reviewing a patch touching this loop I'm having problems wrapping my head around it.
+ + if (disk->startupPolicy) { + virResetLastError();
This properly resets possible unrelated error which might have been reported in qemuDomainDetermineDiskChain() for example, but if you have backing chain broken and you have startupPolicy='mandatory', the error you'll get when starting a domain will be "cannot access file: <filename>: No such file or directory", but the file <filename> will exist, the error will just be overridden. And we definitely don't want that. The virResetLastError() makes sense only in the previously discussed MetadataRecurse. That breaks the test suite, but the check for the WARN-ing is wrong there, so it should be fixed instead. This loop should be reworked to always properly output an error that is related to the situation, no errors should be shadowed etc. Simply what we are trying to do elsewhere. Martin
For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error. --- src/qemu/qemu_domain.c | 31 +++++++++++++------------------ src/qemu/qemu_process.c | 6 ------ 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be77991..1e75adb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2043,19 +2043,11 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); goto error; - break; case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: - if (cold_boot) { - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); + if (cold_boot) goto error; - } break; case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: @@ -2064,6 +2056,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; } + virResetLastError(); VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); @@ -2091,22 +2084,24 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + VIR_DEBUG("Checking for disk presence"); for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i]; - if (!disk->startupPolicy || !disk->src) + if (!disk->src) continue; - if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ - continue; + if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 && + qemuDiskChainCheckBroken(disk) >= 0) + continue; + + if (disk->startupPolicy) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0) + continue; } - if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, - cold_boot) < 0) - goto cleanup; + goto cleanup; } ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e8e459e..61a897c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3621,16 +3621,10 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) goto cleanup; - VIR_DEBUG("Checking for CDROM and floppy presence"); if (qemuDomainCheckDiskPresence(driver, vm, flags & VIR_QEMU_PROCESS_START_COLD) < 0) goto cleanup; - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], - false) < 0) - goto cleanup; - } /* Get the advisory nodeset from numad if 'placement' of * either <vcpu> or <numatune> is 'auto'. -- 1.8.3.1
v3->v4 fix code indentation issue. For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error. --- src/qemu/qemu_domain.c | 33 +++++++++++++-------------------- src/qemu/qemu_process.c | 6 ------ 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be77991..1ff802c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2043,19 +2043,11 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); goto error; - break; case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: - if (cold_boot) { - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); + if (cold_boot) goto error; - } break; case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: @@ -2064,6 +2056,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; } + virResetLastError(); VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); @@ -2089,30 +2082,30 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, int ret = -1; size_t i; virDomainDiskDefPtr disk; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + VIR_DEBUG("Checking for disk presence"); for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i]; - if (!disk->startupPolicy || !disk->src) + if (!disk->src) continue; - if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ + if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 && + qemuDiskChainCheckBroken(disk) >= 0) continue; + + if (disk->startupPolicy) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0) + continue; } - if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, - cold_boot) < 0) - goto cleanup; + goto error; } ret = 0; -cleanup: - virObjectUnref(cfg); +error: return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e8e459e..61a897c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3621,16 +3621,10 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) goto cleanup; - VIR_DEBUG("Checking for CDROM and floppy presence"); if (qemuDomainCheckDiskPresence(driver, vm, flags & VIR_QEMU_PROCESS_START_COLD) < 0) goto cleanup; - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], - false) < 0) - goto cleanup; - } /* Get the advisory nodeset from numad if 'placement' of * either <vcpu> or <numatune> is 'auto'. -- 1.8.3.1
On 07/30/2013 08:26 AM, Guannan Ren wrote:
For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error. --- src/qemu/qemu_domain.c | 31 +++++++++++++------------------ src/qemu/qemu_process.c | 6 ------ 2 files changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be77991..1e75adb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2043,19 +2043,11 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break;
case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); goto error; - break;
case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: - if (cold_boot) { - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); + if (cold_boot) goto error; - } break;
case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: @@ -2064,6 +2056,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; }
+ virResetLastError();
Even though it makes perfect sense to have it here, I'd move it one function up, because it seems more readable to me.
VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); @@ -2091,22 +2084,24 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ VIR_DEBUG("Checking for disk presence"); for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i];
- if (!disk->startupPolicy || !disk->src) + if (!disk->src) continue;
- if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ - continue; + if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 && + qemuDiskChainCheckBroken(disk) >= 0) + continue; + + if (disk->startupPolicy) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0)
And rewrite this to one condition. So basically ACK with this squashed in: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1e75adb..c54f9f6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2056,7 +2056,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; } - virResetLastError(); VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); @@ -2095,10 +2094,11 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, qemuDiskChainCheckBroken(disk) >= 0) continue; - if (disk->startupPolicy) { - if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, - cold_boot) >= 0) - continue; + if (disk->startupPolicy && + qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0) { + virResetLastError(); + continue; } goto cleanup; -- Martin
On 07/31/2013 04:49 PM, Martin Kletzander wrote:
On 07/30/2013 08:26 AM, Guannan Ren wrote:
For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error. --- src/qemu/qemu_domain.c | 31 +++++++++++++------------------ src/qemu/qemu_process.c | 6 ------ 2 files changed, 13 insertions(+), 24 deletions(-)
And rewrite this to one condition.
So basically ACK with this squashed in:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1e75adb..c54f9f6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2056,7 +2056,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; }
- virResetLastError(); VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); @@ -2095,10 +2094,11 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, qemuDiskChainCheckBroken(disk) >= 0) continue;
- if (disk->startupPolicy) { - if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, - cold_boot) >= 0) - continue; + if (disk->startupPolicy && + qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0) { + virResetLastError(); + continue; }
goto cleanup; --
Martin
Thanks for the review. pushed. Guannan
On 07/31/2013 04:49 PM, Martin Kletzander wrote:
On 07/30/2013 08:26 AM, Guannan Ren wrote:
For disk with startupPolicy support, such as cdrom and floppy when its chain is broken, the startup policy will apply, otherwise, report an error. --- src/qemu/qemu_domain.c | 31 +++++++++++++------------------ src/qemu/qemu_process.c | 6 ------ 2 files changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be77991..1e75adb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2043,19 +2043,11 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break;
case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); goto error; - break;
case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: - if (cold_boot) { - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); + if (cold_boot) goto error; - } break;
case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: @@ -2064,6 +2056,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; }
+ virResetLastError(); Even though it makes perfect sense to have it here, I'd move it one function up, because it seems more readable to me.
VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); @@ -2091,22 +2084,24 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ VIR_DEBUG("Checking for disk presence"); for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i];
- if (!disk->startupPolicy || !disk->src) + if (!disk->src) continue;
- if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ - continue; + if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 && + qemuDiskChainCheckBroken(disk) >= 0) + continue; + + if (disk->startupPolicy) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk, + cold_boot) >= 0)
And rewrite this to one condition.
It's okay to rewrite it to one condition, but existing format is more readable. The outer if clause is used to check whether disk have startupPolicy set. The inter if clause is used to do the actual startupPolicy work. Maybe later, we can do more work for disk with startupPolicy attribute in its xml definition. Guannan
participants (2)
-
Guannan Ren -
Martin Kletzander