[libvirt] [PATCHv2 0/2] conf: snapshot: Don't default-snapshot empty drives
Series contains a duplicate submission of patch 1/2 with my ohter series. Peter Krempa (2): util: Add function to check if a virStorageSource is "empty" conf: snapshot: Don't default-snapshot empty drives src/conf/snapshot_conf.c | 8 +++++++- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 20 ++++++++++++++++++++ src/util/virstoragefile.h | 1 + 4 files changed, 29 insertions(+), 1 deletion(-) -- 2.1.0
To express empty drive we historically use storage source with empty path. Unfortunately NBD disks may be declared without a path. Add a helper to wrap this logic. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 20 ++++++++++++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 22 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf4548..e819049 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1941,6 +1941,7 @@ virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; +virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 299edcd..ca8be7f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1976,6 +1976,26 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src) /** + * virStorageSourceIsEmpty: + * + * @src: disk source to check + * + * Returns true if @src points to an empty storage source. + */ +bool +virStorageSourceIsEmpty(virStorageSourcePtr src) +{ + if (virStorageSourceIsLocalStorage(src) && !src->path) + return true; + + if (src->type == VIR_STORAGE_TYPE_NONE) + return true; + + return false; +} + + +/** * virStorageSourceBackingStoreClear: * * @src: disk source to clear diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index eccbf4e..2583e10 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -353,6 +353,7 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); +bool virStorageSourceIsEmpty(virStorageSourcePtr src); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); -- 2.1.0
On 09/11/2014 11:54 AM, Peter Krempa wrote:
To express empty drive we historically use storage source with empty path. Unfortunately NBD disks may be declared without a path.
Add a helper to wrap this logic. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 20 ++++++++++++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 22 insertions(+)
ACK. If I remember right from my virDomainBlockCopy code (commit 37588b25), empty paths are allowed for device types of cdrom, floppy, and lun, but forbidden at parse time for type='block'<disk device='cdrom'> and <disk device='floppy'> but notionally forbidden for <disk device='disk'>.
+ * Returns true if @src points to an empty storage source. + */ +bool +virStorageSourceIsEmpty(virStorageSourcePtr src)
Maybe the comment is better as: Returns true if the guest disk has no associated host storage source (such as an empty cdrom drive). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/14 20:06, Eric Blake wrote:
On 09/11/2014 11:54 AM, Peter Krempa wrote:
To express empty drive we historically use storage source with empty path. Unfortunately NBD disks may be declared without a path.
Add a helper to wrap this logic. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 20 ++++++++++++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 22 insertions(+)
ACK.
If I remember right from my virDomainBlockCopy code (commit 37588b25), empty paths are allowed for device types of cdrom, floppy, and lun, but forbidden at parse time for type='block'<disk device='cdrom'> and <disk device='floppy'> but notionally forbidden for <disk device='disk'>.
+ * Returns true if @src points to an empty storage source. + */ +bool +virStorageSourceIsEmpty(virStorageSourcePtr src)
Maybe the comment is better as:
Returns true if the guest disk has no associated host storage source (such as an empty cdrom drive).
Sounds better. I've changed the comment and pushed this patch. Peter
On 09/12/2014 01:43 AM, Peter Krempa wrote:
+ * Returns true if @src points to an empty storage source. + */ +bool +virStorageSourceIsEmpty(virStorageSourcePtr src)
Maybe the comment is better as:
Returns true if the guest disk has no associated host storage source (such as an empty cdrom drive).
Sounds better. I've changed the comment and pushed this patch.
Even more bikeshedding ideas (but don't worry about it, since you already pushed): virStorageSourceNoMedia() Returns true if a guest disk has no media (that is, no associated host storage source) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
If a (floppy) drive isn't selected for snapshot explicitly and is empty don't try to snapshot it. For external snapshots this would fail as we can't generate a name for the snapshot from an empty drive. Reported-by: Pavel Hrdina <phrdina@redhat.com> --- Version 2 now doesn't discriminate according to drive type src/conf/snapshot_conf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c53a66b..4f99139 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -561,7 +561,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0) goto cleanup; disk->index = i; - disk->snapshot = def->dom->disks[i]->snapshot; + + /* Don't snapshot empty drives */ + if (virStorageSourceIsEmpty(def->dom->disks[i]->src)) + disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + else + disk->snapshot = def->dom->disks[i]->snapshot; + disk->src->type = VIR_STORAGE_TYPE_FILE; if (!disk->snapshot) disk->snapshot = default_snapshot; -- 2.1.0
On 09/11/2014 11:54 AM, Peter Krempa wrote:
If a (floppy) drive isn't selected for snapshot explicitly and is empty don't try to snapshot it. For external snapshots this would fail as we can't generate a name for the snapshot from an empty drive.
Reported-by: Pavel Hrdina <phrdina@redhat.com> ---
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/14 20:07, Eric Blake wrote:
On 09/11/2014 11:54 AM, Peter Krempa wrote:
If a (floppy) drive isn't selected for snapshot explicitly and is empty don't try to snapshot it. For external snapshots this would fail as we can't generate a name for the snapshot from an empty drive.
Reported-by: Pavel Hrdina <phrdina@redhat.com> ---
ACK.
Pushed; Thanks. Peter
participants (2)
-
Eric Blake -
Peter Krempa