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

[Peter Krempa]

On 11/21/12 15:14, Eric Blake wrote:
> On 11/21/2012 04:05 AM, Peter Krempa wrote:
>> Commit e0c469e58b93f852a72265919703cb6abd3779f8 that fixes the detection
>> of image chain wasn't complete. Iteration through the backing image
>> chain has to stop at the last existing image if some of the images are
>> missing otherwise the backing chain that is cached contains entries with
>> paths being set to NULL resulting to:
>>
>> error: Unable to allow access for disk path (null): Bad address
>>
>> Fortunately stat() is kind enough not to crash when it's presented with
>> a NULL argument.
>
> That's only true for stat() on Linux; other OSs are fully entitled to
> crash, so this definitely needs fixing :)
>
>> ---
>>   src/util/storage_file.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
>> index 2249212..e7cbea7 100644
>> --- a/src/util/storage_file.c
>> +++ 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);
>> +                    meta->backingStoreIsFile = false;
>>                       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

But in the other case, we are gathering incomplete information. If we 
don't fix this here, the backing chain will contain the entry 
representing the missing image and the previous part of the chain will 
be a raw image.

> later client of the chain at fault for not recognizing
> VIR_STORAGE_FILE_NONE coupled with non-NULL backingStoreRaw as the key

Hm we could use this to report a broken chain before qemu dies with a 
not very helpful message.

> for whether a backing file is missing?  I'm guessing
> domain_conf.c:virDomainDiskDefForeachPath would be the real function to

That was the second place I was thinking of fixing this but the approach 
depends on if we want to check if the chain is OK before qemu does (In 
that case I think it's OK to have that bogus entry of the missing file 
in the chain but we should report that as a broken chain and not as an 
raw image at the end of the chain that actually isn't the end of the 
chain) or we leave the chain enumeration on qemu (and we will not need 
to store the indicator of the broken chain as we don't care in that moment).

> fix here.
>

Which road are we going to take?

Peter