[libvirt] [PATCH] qemu: don't check for backing chains for formats w/o snapshot support
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1019926 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868673 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 3 +++ src/util/virstoragefile.c | 15 +++++++++++++++ src/util/virstoragefile.h | 2 ++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0e81f2f..42c0185 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1834,6 +1834,7 @@ virStorageFileIsClusterFS; virStorageFileProbeFormat; virStorageFileProbeFormatFromBuf; virStorageFileResize; +virStorageFormatMaySupportSnapshots; virStorageIsFile; virStorageNetHostDefClear; virStorageNetHostDefCopy; @@ -1850,7 +1851,6 @@ virStorageSourcePoolModeTypeToString; virStorageTypeFromString; virStorageTypeToString; - # util/virstring.h virArgvToString; virAsprintfInternal; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cdd4601..abc2a68 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2250,6 +2250,9 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, if (!virDomainDiskGetSource(disk)) continue; + if (!virStorageFormatMaySupportSnapshots(disk->src.format)) + continue; + if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 && qemuDiskChainCheckBroken(disk) >= 0) continue; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ea80c1d..c781a6b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1730,3 +1730,18 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageSourceAuthClear(def); } + +bool +virStorageFormatMaySupportSnapshots(enum virStorageFileFormat format) +{ + if (format == VIR_STORAGE_FILE_AUTO || + format == VIR_STORAGE_FILE_AUTO_SAFE) + return true; + + /* Better safe than sorry */ + if (format <= VIR_STORAGE_FILE_NONE || + format >= VIR_STORAGE_FILE_LAST) + return false; + + return !!fileTypeInfo[format].getBackingStore; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1b8b14f..bcbfb88 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -339,4 +339,6 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); +bool virStorageFormatMaySupportSnapshots(enum virStorageFileFormat format); + #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.2
On 04/17/2014 04:20 AM, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1019926 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868673
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 3 +++ src/util/virstoragefile.c | 15 +++++++++++++++ src/util/virstoragefile.h | 2 ++ 4 files changed, 21 insertions(+), 1 deletion(-)
@@ -1850,7 +1851,6 @@ virStorageSourcePoolModeTypeToString; virStorageTypeFromString; virStorageTypeToString;
- # util/virstring.h
Spurious whitespace change.
+ +bool +virStorageFormatMaySupportSnapshots(enum virStorageFileFormat format) +{ + if (format == VIR_STORAGE_FILE_AUTO || + format == VIR_STORAGE_FILE_AUTO_SAFE) + return true; + + /* Better safe than sorry */ + if (format <= VIR_STORAGE_FILE_NONE || + format >= VIR_STORAGE_FILE_LAST) + return false; + + return !!fileTypeInfo[format].getBackingStore;
Hmm, how does this compare with the recent commit db7d7c0e which added the VIR_STORAGE_FILE_BACKING marker? I made that separation in order to state that all formats less than the marker do not have a getBackingStore callback (well, other than the exceptions of the _AUTO and _AUTO_SAFE formats which are less than 0), and all formats >= that marker DO have a potential for a backing store. Should this code be using that new constant instead of probing the existence of getBackingStore? Or conversely, should the domain_conf.c code that I touched in that patch instead be using this new function instead of relying on the VIR_STORAGE_FILE_BACKING marker? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Apr 21, 2014 at 03:22:43PM -0600, Eric Blake wrote:
On 04/17/2014 04:20 AM, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1019926 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868673
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 3 +++ src/util/virstoragefile.c | 15 +++++++++++++++ src/util/virstoragefile.h | 2 ++ 4 files changed, 21 insertions(+), 1 deletion(-)
@@ -1850,7 +1851,6 @@ virStorageSourcePoolModeTypeToString; virStorageTypeFromString; virStorageTypeToString;
- # util/virstring.h
Spurious whitespace change.
+ +bool +virStorageFormatMaySupportSnapshots(enum virStorageFileFormat format) +{ + if (format == VIR_STORAGE_FILE_AUTO || + format == VIR_STORAGE_FILE_AUTO_SAFE) + return true; + + /* Better safe than sorry */ + if (format <= VIR_STORAGE_FILE_NONE || + format >= VIR_STORAGE_FILE_LAST) + return false; + + return !!fileTypeInfo[format].getBackingStore;
Hmm, how does this compare with the recent commit db7d7c0e which added the VIR_STORAGE_FILE_BACKING marker? I made that separation in order to state that all formats less than the marker do not have a getBackingStore callback (well, other than the exceptions of the _AUTO and _AUTO_SAFE formats which are less than 0), and all formats >= that marker DO have a potential for a backing store. Should this code be using that new constant instead of probing the existence of getBackingStore? Or conversely, should the domain_conf.c code that I touched in that patch instead be using this new function instead of relying on the VIR_STORAGE_FILE_BACKING marker?
TBH, I haven't noticed the change, this function just works. I first tried to enumerate all the VIR_STORAGE_FILE_* formats, but that looked terrible and it haven't met the goal I had. The aim in commit db7d7c0e differs a bit, I guess. My point was that we are opening files just to close them in a while, because we have no getBackingStore() function (the bug is opened about /dev/sr0 without media in the drive, which should "just work" and it doesn't). Since all formats >= VIR_STORAGE_FILE_BACKING have .getBackingStore specified, this function and the approach in db7d7c0e currently differs only for VIR_STORAGE_FILE_AUTO(_SAFE), but those can't be specified in the backing chain, right? One more thing in which this differs is a format change in future, but IIUC, the order of the elements in the enum can be changed, so if new format without backing support is added or backing support is implemented for QED, re-ordering of those elements will solve it. Having said all that, I think changing either approach to the different one is fine, but changing my patch to the following one will probably be the cleanest way. Martin diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index cdd4601..5bde917 100644 --- i/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -2246,11 +2246,14 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, VIR_DEBUG("Checking for disk presence"); for (i = vm->def->ndisks; i > 0; i--) { disk = vm->def->disks[i - 1]; + enum virStorageFileFormat format = virDomainDiskGetFormat(disk); if (!virDomainDiskGetSource(disk)) continue; - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 && + if ((format < VIR_STORAGE_FILE_NONE || + format >= VIR_STORAGE_FILE_BACKING) && + qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 && qemuDiskChainCheckBroken(disk) >= 0) continue; --
participants (2)
-
Eric Blake -
Martin Kletzander