[libvirt] [PATCH] qemu: Stop recursive detection of image chains when an image is missing
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. --- 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; } } -- 1.8.0
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 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. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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);
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.
+ 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).
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.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
participants (2)
-
Eric Blake -
Peter Krempa