On 04/11/2014 12:21 AM, Eric Blake wrote:
The chain lookup function was inconsistent on whether it left a message in the log when looking up a name that is not found on the chain (leaving a message for OOM or if name was relative but not part of the chain), and could litter the log even when successful (when name was relative but deep in the chain, use of virFindBackingFile early in the chain would complain about a file not found). It's easier to make the function consistently emit a message exactly once on failure, and to let all callers rely on the clean semantics.
* src/util/virstoragefile.c (virStorageFileChainLookup): Always report error on failure. Simplify relative lookups. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Avoid overwriting error.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 10 +--------- src/util/virstoragefile.c | 17 +++++++++-------- 2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0c6a74..b8cfe27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15346,9 +15346,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, disk->src.path, top, &top_meta, &top_parent))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find top '%s' in chain for '%s'"), - top, path); goto endjob; } if (!top_meta || !top_meta->backingStore) { @@ -15360,13 +15357,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { base_canon = top_meta->backingStore; } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, - base, NULL, NULL))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find base '%s' below '%s' in chain " - "for '%s'"), - base ? base : "(default)", top_canon, path); + base, NULL, NULL))) goto endjob; - }
Does removal of {} violate the one line "rule of thumb"... In either case the "if" portion should be made consistent as well as this now has if (xxx) { oneline } else onelonglinebecauseofarguments Probably would be nice to have an extra line after endjob; and before following comment - just a readability thing for me at least.
/* Note that this code exploits the fact that * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2013914..66a6ab1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1537,7 +1537,8 @@ int virStorageFileGetSCSIKey(const char *path, * backing element is not a file). If PARENT is not NULL, set *PARENT * to the preferred name of the parent (or to NULL if NAME matches * START). Since the results point within CHAIN, they must not be - * independently freed. */ + * independently freed. Reports an error and returns NULL if NAME is + * not found. */ const char * virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, const char *name, virStorageFileMetadataPtr *meta, @@ -1570,15 +1571,12 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, STREQ(name, owner->backingStore)) { break; } else if (virStorageIsFile(owner->backingStore)) { - char *absName = NULL; - if (virFindBackingFile(owner->directory, name, - NULL, &absName) < 0) + int result = virFileRelLinkPointsTo(owner->directory, name, + owner->backingStore); + if (result < 0) goto error; - if (absName && STREQ(absName, owner->backingStore)) { - VIR_FREE(absName); + if (result > 0) break; - } - VIR_FREE(absName); } *parent = owner->backingStore; owner = owner->backingMeta; @@ -1590,6 +1588,9 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, return owner->backingStore;
error: + virReportError(VIR_ERR_INVALID_ARG, + _("could not find image '%s' in chain for '%s'"), + name, start);
Looking at the context of the code expanded out a bit... there's a few checks for !name = can we get here with name == NULL? Looking ahead to patch 4 this will be more important...
*parent = NULL; if (meta) *meta = NULL;
ACK - seems reasonable, just address the possible NULL name... John