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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list