On 11/22/12 14:44, Eric Blake wrote:
On 11/22/2012 04:26 AM, Peter Krempa wrote:
+++ b/src/util/storage_file.c
@@ -729,6 +729,8 @@ virStorageFileGetMetadataFromBuf(int format,
if (meta->backingStore == NULL) {
/* the backing file is (currently) unavailable,
treat this
* file as standalone */
+ VIR_FREE(meta->backingStoreRaw);
Also I should explain the following thing:
This freeing of meta->backingStoreRaw actualy doesn't fix the bug. It
just felt a right thing to do when the backing file is missing.
Hmm, actually, keeping this around might be nice if someone inspecting
the chain for missing backing files wants to know that the file was
missing. But if the same information is also found in
meta->backingStore, then I guess freeing it is okay (that is, the idea
is that for a missing backing file, we should be leaving enough
breadcrumbs for later clients to diagnose what is missing, but also to
know that it is missing).
The backingStoreRaw would be ideal for this purpose ... well as long as
the callers recognize what's going on.
+ meta->backingStoreIsFile = false;
This line is the actual fix. The code that recursively detects the image
chain uses this value as a marker if another iteration of the recursion
is needed (on the non-existing file).
Ah - this line indeed makes sense, then. If we are going to set the
format as NONE, then we should also mark that it is not a regular file
(it is a missing file). Keep this change.
backingFormat = VIR_STORAGE_FILE_NONE;
Hmm, I'm wondering if this is the right place to fix it, or if we are
throwing away information too early. That is, are we sure it is not the
later client of the chain at fault for not recognizing
VIR_STORAGE_FILE_NONE coupled with non-NULL backingStoreRaw as the key
for whether a backing file is missing? I'm guessing
domain_conf.c:virDomainDiskDefForeachPath would be the real function to
fix here.
I will add the check also here, but I'd like to discuss the creating of
the chain in the first place.
So it sounds like the real fix needs to touch both places - the creation
of the chain to make sure it leaves enough breadcrumbs, and the
iteration over the chain to make sure that it handles a missing backing
file according to the user's wishes (since virDomainDiskDefForeachPath
has bool ignoreOpenFailure that says whether to ignore a missing file or
to fail because the chain is incomplete).
I knew this area of code would be tricky when I first started touching it :(
It is :) I spent quite a few brain cycles trying to comprehend it. I'll
post a v2 soon.
Peter
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list