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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list