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

*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,

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();

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