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

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(a)redhat.com&gt; 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list