Re: [libvirt] [PATCH] qemu: don't check for backing chains for formats w/o snapshot support
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(a)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;
--
Attachments:
- signature.asc (application/pgp-signature — 836 bytes)