[libvirt] bug: try to take disk snapshot for LVM2 Volume

[Eric Blake]

On 11/15/2011 04:18 PM, Eric Blake wrote:
>> 2. Do the 'virsh snapshot-create abc --disk-only' with libvirt-0.9.6 or
>> upper.
> 
> But here, since you failed to use the --xmlfile option with XML
> describing the new qcow2 file name (or used snapshot-create-as with the
> --diskspec option), you are asking libvirt to create the name for the
> new qcow2 file, and to create it in the same directory as the existing
> disk image.  That is, you asked libvirt to create /dev/sdc.1317357844
> (with that suffix being the timestamp of your attempt).
> 

>>
>> And 'snapshot-create ... --disk-only' works well for image format file,
>> e.g. qcow2, raw and etc.
> 
> It works for any backing file format.  Where it needs help is knowing
> what file name to use - if your backing file is a block device instead
> of a regular file, then it is up to you to help libvirt out by giving it
> a sensible file name for the new qcow2 image, either with the --xmlfile
> option of snapshot-create, or the --diskspec option of snapshot-create-as.
> 
>>
>> So, I think that the restriction is needed for the taking snapshot
>> disk-only for the disk volume.
> 
> This is where I disagree with your conclusion.  Taking a disk-only
> snapshot of block devices works just fine, provided that you tell
> libvirt what file name to use for the qcow2 file it creates.

After working on this some more, I think that identifying problematic
file systems, like devtmpfs, is too tricky to be portable.  But I think
we can meet halfway - right now, the libvirt code blindly generates a
filename if one was not provided, regardless of the file type of the
backing file it will be wrapping.  But maybe it would be sufficient to
make the command error out if no qcow2 filename is provided and the
backing file is not a regular file.  As in this patch:

From 099080c24eb1ed78c836e5823d351ab2980dc523 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake at redhat.com>
Date: Tue, 15 Nov 2011 17:19:20 -0700
Subject: [PATCH] snapshot: refuse to generate names for non-regular backing
 files

For whatever reason, the kernel allows you to create a regular
file named /dev/sdc.12345; although this file will disappear the
next time devtmpfs is remounted.  If you let libvirt generate
the name of the external snapshot for a disk image originally
using the block device /dev/sdc, then the domain will be rendered
unbootable once the qcow2 file is lost on the next devtmpfs
remount.  In this case, the user should have used 'virsh
snapshot-create --xmlfile' or 'virsh snapshot-create-as --diskspec'
to specify the name for the qcow2 file in a sane location, rather
than relying on libvirt generating a name that is most likely to
be wrong.  We can help avoid naive mistakes by enforcing that
the user provide the external name for any backing file that is
not a regular file.

* src/conf/domain_conf.c (virDomainSnapshotAlignDisks): Only
generate names if backing file exists as regular file.
Reported by MATSUDA Daiki.
---
 src/conf/domain_conf.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6b78d97..1cef2be 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12203,7 +12203,8 @@
virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,

     qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter);

-    /* Generate any default external file names.  */
+    /* Generate any default external file names, but only if the
+     * backing file is a regular file.  */
     for (i = 0; i < def->ndisks; i++) {
         virDomainSnapshotDiskDefPtr disk = &def->disks[i];

@@ -12211,14 +12212,24 @@
virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
             !disk->file) {
             const char *original = def->dom->disks[i]->src;
             const char *tmp;
+            struct stat sb;

             if (!original) {
                 virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                     _("cannot generate external backup
name "
+                                     _("cannot generate external
snapshot name "
                                        "for disk '%s' without source"),
                                      disk->name);
                 goto cleanup;
             }
+            if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) {
+                virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                     _("source for disk '%s' is not a
regular "
+                                       "file; refusing to generate
external "
+                                       "snapshot name"),
+                                     disk->name);
+                goto cleanup;
+            }
+
             tmp = strrchr(original, '.');
             if (!tmp || strchr(tmp, '/')) {
                 ignore_value(virAsprintf(&disk->file, "%s.%s",
-- 
1.7.7.1



-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111115/1f4b9597/attachment-0001.sig>