12:54 p.m.
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
12:54 p.m.
New subject: [libvirt] [PATCHv2 1/2] DUPLICATE: util: Add function to check if a virStorageSource is "empty"
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
1:06 p.m.
New subject: [libvirt] [PATCHv2 1/2] DUPLICATE: util: Add function to check if a virStorageSource is "empty"
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
2:43 a.m.
New subject: [libvirt] [PATCHv2 1/2] DUPLICATE: util: Add function to check if a virStorageSource is "empty"
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
8:13 a.m.
New subject: [libvirt] [PATCHv2 1/2] DUPLICATE: util: Add function to check if a virStorageSource is "empty"
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
12:54 p.m.
New subject: [libvirt] [PATCHv2 2/2] conf: snapshot: Don't default-snapshot empty drives
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(a)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
1:07 p.m.
New subject: [libvirt] [PATCHv2 2/2] conf: snapshot: Don't default-snapshot empty drives
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(a)redhat.com>
---
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:43 a.m.
New subject: [libvirt] [PATCHv2 2/2] conf: snapshot: Don't default-snapshot empty drives
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(a)redhat.com>
> ---
ACK.
Pushed; Thanks.
Peter
3724
days inactive
3725
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa