On 04/11/2014 01:12 PM, John Ferlan wrote:
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.
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"...
Eww, I did indeed break the double {} rule.
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...
Indeed, callers can pass NULL; I'll improve it.
ACK - seems reasonable, just address the possible NULL name...
Pushing with this added: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index b8cfe27..fed7d1c 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -15354,11 +15354,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, top_canon, path); goto endjob; } - if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { + 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))) + else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, + base, NULL, NULL))) goto endjob; + /* Note that this code exploits the fact that * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 66a6ab1..77cc878 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1588,9 +1588,14 @@ 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); + if (name) + virReportError(VIR_ERR_INVALID_ARG, + _("could not find image '%s' in chain for '%s'"), + name, start); + else + virReportError(VIR_ERR_INVALID_ARG, + _("could not find base image in chain for '%s'"), + start); *parent = NULL; if (meta) *meta = NULL; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org