[libvirt] [PATCH] qemu: Stop recursive detection of image chains when an image is missing

[Peter Krempa]

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