Re: [libvirt] [PATCH 3/6] conf: report error on chain lookup failure
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)