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).
>
>>> + 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 :(
--
Eric Blake eblake@xxxxxxxxxx +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