[libvirt] [PATCH] conf: snapshot: Don't default-snapshot empty floppy 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@redhat.com> --- src/conf/snapshot_conf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c53a66b..cbaff74 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -561,7 +561,14 @@ 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 floppy drives */ + if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + !virDomainDiskGetSource(def->dom->disks[i])) + 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 09:47 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.
Do we need the same for cdrom drives?
Reported-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c53a66b..cbaff74 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -561,7 +561,14 @@ 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 floppy drives */ + if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + !virDomainDiskGetSource(def->dom->disks[i]))
If we are worried about ALL empty drives, it would be simpler to just drop the left side of the &&, making it solely a test of whether there is currently a defined host source. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/14 18:16, Eric Blake wrote:
On 09/11/2014 09:47 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.
Do we need the same for cdrom drives?
CDROMs are automatically read only and thus get the _NONE target right when parsing the configuration.
Reported-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c53a66b..cbaff74 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -561,7 +561,14 @@ 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 floppy drives */ + if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + !virDomainDiskGetSource(def->dom->disks[i]))
If we are worried about ALL empty drives, it would be simpler to just drop the left side of the &&, making it solely a test of whether there is currently a defined host source.
Since only CDROMs and floppies can be empty and cdroms are already exempted from here it should be functionally equivalent to do that. The only limitation is that the check for the empty source probably needs to be stronger (NBD disks may have the disk->src->path NULL even if they have backing.) I'll post a v2. Peter
On 09/11/14 18:25, Peter Krempa wrote:
On 09/11/14 18:16, Eric Blake wrote:
On 09/11/2014 09:47 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.
Do we need the same for cdrom drives?
CDROMs are automatically read only and thus get the _NONE target right when parsing the configuration.
Reported-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c53a66b..cbaff74 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -561,7 +561,14 @@ 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 floppy drives */ + if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + !virDomainDiskGetSource(def->dom->disks[i]))
If we are worried about ALL empty drives, it would be simpler to just drop the left side of the &&, making it solely a test of whether there is currently a defined host source.
Since only CDROMs and floppies can be empty and cdroms are already exempted from here it should be functionally equivalent to do that. The only limitation is that the check for the empty source probably needs to be stronger (NBD disks may have the disk->src->path NULL even if they have backing.)
Which reminds me that snapshots of NBD disks are forbidden, so it should be fine even without tweaking the emptiness check.
I'll post a v2.
Your call whether I should try to improve the check or leave it as-is. Peter
On 09/11/2014 10:28 AM, Peter Krempa wrote:
On 09/11/14 18:25, Peter Krempa wrote:
On 09/11/14 18:16, Eric Blake wrote:
On 09/11/2014 09:47 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.
Do we need the same for cdrom drives?
CDROMs are automatically read only and thus get the _NONE target right when parsing the configuration.
+ + /* Don't snapshot empty floppy drives */ + if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + !virDomainDiskGetSource(def->dom->disks[i]))
If we are worried about ALL empty drives, it would be simpler to just drop the left side of the &&, making it solely a test of whether there is currently a defined host source.
Since only CDROMs and floppies can be empty and cdroms are already exempted from here it should be functionally equivalent to do that. The only limitation is that the check for the empty source probably needs to be stronger (NBD disks may have the disk->src->path NULL even if they have backing.)
<disk type='block' device='lun'> also allows for a NULL src, if I remember correctly.
Which reminds me that snapshots of NBD disks are forbidden, so it should be fine even without tweaking the emptiness check.
Still feels fragile.
I'll post a v2.
Your call whether I should try to improve the check or leave it as-is.
I'd feel more comfortable with the generic check that all source-less disks are explicitly tweaked to not have a snapshot taken, rather than relying on side checks like readonly saving the day. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa