8:35 a.m.
Peter Krempa (4):
qemu: snapshot: Use typecasted switch in qemuDomainSnapshotPrepare()
qemu: snapshot: Forbid partial internal snapshots
qemu: snapshot: Forbid empty snapshots
qemu: snapshot: Fix return value of external checkpoint with no disks
src/qemu/qemu_driver.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
--
1.9.3
8:35 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: snapshot: Use typecasted switch in qemuDomainSnapshotPrepare()
Convert the switch to a typecasted value so that the compiler tracks
additions for us.
---
src/qemu/qemu_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c8a0029..aa07ef6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12635,7 +12635,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
virDomainSnapshotDiskDefPtr disk = &def->disks[i];
virDomainDiskDefPtr dom_disk = vm->def->disks[i];
- switch (disk->snapshot) {
+ switch ((virDomainSnapshotLocation) disk->snapshot) {
case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
found_internal = true;
@@ -12692,7 +12692,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
break;
case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT:
- default:
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unexpected code path"));
goto cleanup;
--
1.9.3
9:13 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: snapshot: Use typecasted switch in qemuDomainSnapshotPrepare()
On 05/20/2014 07:35 AM, Peter Krempa wrote:
Convert the switch to a typecasted value so that the compiler tracks
additions for us.
---
src/qemu/qemu_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:36 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: snapshot: Forbid partial internal snapshots
qemu's savevm command does a snapshot of all non readonly disks of a VM.
Libvirt though allowed disabling snapshot for certain disk of a VM.
---
src/qemu/qemu_driver.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index aa07ef6..e6b6648 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12621,6 +12621,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
bool found_internal = false;
+ bool forbid_internal = false;
int external = 0;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -12689,6 +12690,9 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
break;
case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE:
+ /* Remember seeing a disk that has snapshot disabled */
+ if (!dom_disk->readonly)
+ forbid_internal = true;
break;
case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT:
@@ -12699,12 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
}
}
- /* internal snapshot requires a disk image to store the memory image to */
- if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL &&
- !found_internal) {
+ /* internal snapshot requires a disk image to store the memory image to and
+ * also disks can't be excluded from a internal snapshot*/
+ if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL &&
!found_internal) ||
+ (found_internal && forbid_internal)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("internal checkpoints require at least "
- "one disk to be selected for snapshot"));
+ _("internal snapshots and checkpoints require all "
+ "disks to be selected for snapshot"));
goto cleanup;
}
--
1.9.3
9:03 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: snapshot: Forbid partial internal snapshots
On 05/20/2014 03:36 PM, Peter Krempa wrote:
qemu's savevm command does a snapshot of all non readonly disks
of a VM.
Libvirt though allowed disabling snapshot for certain disk of a VM.
---
src/qemu/qemu_driver.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index aa07ef6..e6b6648 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12699,12 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr
conn,
}
}
- /* internal snapshot requires a disk image to store the memory image to */
- if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL &&
- !found_internal) {
+ /* internal snapshot requires a disk image to store the memory image to and
+ * also disks can't be excluded from a internal snapshot*/
s/a internal snapshot/an internal snapshot /
+ if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL
&& !found_internal) ||
+ (found_internal && forbid_internal)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("internal checkpoints require at least "
- "one disk to be selected for snapshot"));
+ _("internal snapshots and checkpoints require all "
+ "disks to be selected for snapshot"));
goto cleanup;
}
Jan
9:17 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: snapshot: Forbid partial internal snapshots
On 05/20/2014 07:36 AM, Peter Krempa wrote:
qemu's savevm command does a snapshot of all non readonly disks
of a VM.
Libvirt though allowed disabling snapshot for certain disk of a VM.
---
src/qemu/qemu_driver.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
I think this may even be a regression introduced when we added
checkpoint snapshots, because I seem to recall trying to prevent just
this back when doing external disk snapshots. But I didn't bother to
crawl git history to confirm or deny my suspicion.
@@ -12699,12 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr
conn,
}
}
- /* internal snapshot requires a disk image to store the memory image to */
- if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL &&
- !found_internal) {
+ /* internal snapshot requires a disk image to store the memory image to and
s/to and/to, and/
+ * also disks can't be excluded from a internal snapshot*/
s/a internal/an internal/
+ if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL
&& !found_internal) ||
+ (found_internal && forbid_internal)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("internal checkpoints require at least "
- "one disk to be selected for snapshot"));
+ _("internal snapshots and checkpoints require all "
+ "disks to be selected for snapshot"));
goto cleanup;
}
ACK with the comment grammar fixes
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:36 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: snapshot: Forbid empty snapshots
If neither disks nor memory are selected for snapshot we'd record
metadata in case of external snapshot and do a disk snapshot in case of
external disk snapshot. Forbid this as it doesn't make much sense.
---
Notes:
There's a slightly usable case where this would be used to backup domain's
config.
src/qemu/qemu_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e6b6648..6442b3f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12703,6 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
}
}
+ if (!found_internal && !external &&
+ def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nothing selected for snapshot"));
+ goto cleanup;
+ }
+
/* internal snapshot requires a disk image to store the memory image to and
* also disks can't be excluded from a internal snapshot*/
if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL &&
!found_internal) ||
--
1.9.3
9:03 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: snapshot: Forbid empty snapshots
On 05/20/2014 03:36 PM, Peter Krempa wrote:
If neither disks nor memory are selected for snapshot we'd
record
metadata in case of external snapshot and do a disk snapshot in case of
external disk snapshot. Forbid this as it doesn't make much sense.
s/external/internal/
---
Notes:
There's a slightly usable case where this would be used to backup domain's
config.
src/qemu/qemu_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
Jan
9:24 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: snapshot: Forbid empty snapshots
On 05/20/2014 07:36 AM, Peter Krempa wrote:
If neither disks nor memory are selected for snapshot we'd
record
metadata in case of external snapshot and do a disk snapshot in case of
external disk snapshot. Forbid this as it doesn't make much sense.
---
Notes:
There's a slightly usable case where this would be used to backup domain's
config.
Hmm. Down the road, when we start integrating chain operations (pull,
commit) more closely with snapshots, we may hit the case where a
blockpull goes through and modifies a snapshot to record that the disk
snapshot taken at that point of time was invalidated by the chain
operation. Perhaps we could still leave the empty snapshot in the list
of snapshots, and if so, then this code should also allow creation of
empty snapshots. But I tend to agree that it sounds rather pointless to
track empty snapshots, so I'd rather have chain operations delete
snapshots that are otherwise rendered empty, and agree with the approach
of this patch in forbidding an empty snapshot. Someone wanting to
preserve configuration can use virsh dumpxml.
+++ b/src/qemu/qemu_driver.c
@@ -12703,6 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
}
}
+ if (!found_internal && !external &&
+ def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nothing selected for snapshot"));
+ goto cleanup;
+ }
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:36 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
When doing an external checkpoint of a VM with no disk selected we'd
return failure but not set error code. This was a result of ret not
being set to 0 during walking of the disk array.
Rework early failure checking to avoid the problem.
Fixes the following symptom (or without --diskspec for diskless VMs)
$ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec hda,snapshot=no
error: An error occurred, but the cause is unknown
---
src/qemu/qemu_driver.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6442b3f..460c680 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13010,17 +13010,17 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virJSONValuePtr actions = NULL;
- int ret = -1;
+ int ret = 0;
size_t i;
bool persist = false;
bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
- virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ virQEMUDriverConfigPtr cfg = NULL;
virErrorPtr orig_err = NULL;
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is not running"));
- goto cleanup;
+ return -1;
}
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
@@ -13030,9 +13030,11 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("live disk snapshot not supported with this "
"QEMU binary"));
- goto cleanup;
+ return -1;
}
+ cfg = virQEMUDriverGetConfig(driver);
+
/* No way to roll back if first disk succeeds but later disks
* fail, unless we have transaction support.
* Based on earlier qemuDomainSnapshotPrepare, all
--
1.9.3
9:28 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/2014 07:36 AM, Peter Krempa wrote:
When doing an external checkpoint of a VM with no disk selected
we'd
return failure but not set error code. This was a result of ret not
being set to 0 during walking of the disk array.
Rework early failure checking to avoid the problem.
Fixes the following symptom (or without --diskspec for diskless VMs)
$ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec
hda,snapshot=no
error: An error occurred, but the cause is unknown
---
Technically, this is equivalent to a 'virsh save'. Would it be better
to avoid an error altogether and allow the operation as a longhand
spelling of the same action?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:29 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/14 16:28, Eric Blake wrote:
On 05/20/2014 07:36 AM, Peter Krempa wrote:
> When doing an external checkpoint of a VM with no disk selected we'd
> return failure but not set error code. This was a result of ret not
> being set to 0 during walking of the disk array.
>
> Rework early failure checking to avoid the problem.
>
> Fixes the following symptom (or without --diskspec for diskless VMs)
>
> $ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec
hda,snapshot=no
> error: An error occurred, but the cause is unknown
> ---
Technically, this is equivalent to a 'virsh save'. Would it be better
to avoid an error altogether and allow the operation as a longhand
spelling of the same action?
virsh save kills the VM after it finishes. This allows to preserve the
VM running and also allows snapshots of diskless VMs.
Peter
11:16 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/2014 08:29 AM, Peter Krempa wrote:
On 05/20/14 16:28, Eric Blake wrote:
> On 05/20/2014 07:36 AM, Peter Krempa wrote:
>> When doing an external checkpoint of a VM with no disk selected we'd
>> return failure but not set error code. This was a result of ret not
>> being set to 0 during walking of the disk array.
>>
>> Rework early failure checking to avoid the problem.
>>
>> Fixes the following symptom (or without --diskspec for diskless VMs)
>>
>> $ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec
hda,snapshot=no
>> error: An error occurred, but the cause is unknown
>> ---
>
> Technically, this is equivalent to a 'virsh save'. Would it be better
> to avoid an error altogether and allow the operation as a longhand
> spelling of the same action?
>
virsh save kills the VM after it finishes. This allows to preserve the
VM running and also allows snapshots of diskless VMs.
Adding an option to 'virsh save' to not kill the guest doesn't make much
sense in general, but might make sense for a diskless VM. Meanwhile,
don't we already have the option to kill or keep the guest alive after
an external memory snapshot?
So I'm not quite sure where you are heading with this patch - is the
idea that we WANT to allow an external checkpoint with no disks, and
this is fixing a bug that gave an unknown error, or is this a case where
we want to forbid diskless external checkpoints, and you are just
improving the error message?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:21 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/2014 10:16 AM, Eric Blake wrote:
So I'm not quite sure where you are heading with this patch - is
the
idea that we WANT to allow an external checkpoint with no disks, and
this is fixing a bug that gave an unknown error, or is this a case where
we want to forbid diskless external checkpoints, and you are just
improving the error message?
More concretely, I _want_ to allow a memory-only external snapshot
(particularly of a diskless VM). If this patch is fixing a bogus error
message into allowing that case, then please touch up the commit message
to make that a bit more clear, and I'm fine with including the patch (my
initial read was that this patch is just improving error message quality
but still leaving it an error, but on reflection it looks like I
mis-read the intent because of confusion about the commit message).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:55 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/14 18:21, Eric Blake wrote:
On 05/20/2014 10:16 AM, Eric Blake wrote:
> So I'm not quite sure where you are heading with this patch - is the
> idea that we WANT to allow an external checkpoint with no disks, and
> this is fixing a bug that gave an unknown error, or is this a case where
> we want to forbid diskless external checkpoints, and you are just
> improving the error message?
More concretely, I _want_ to allow a memory-only external snapshot
Well, this patch exactly does that. That's why there's no "after" error
message.
The commit message isn't probably clear enough.
How about changing one of the sentences to:
Rework early failure checking and set the error code to success before
iterating the array of disks so that we return success if no disks are
snapshotted.
(particularly of a diskless VM). If this patch is fixing a bogus
error
message into allowing that case, then please touch up the commit message
to make that a bit more clear, and I'm fine with including the patch (my
initial read was that this patch is just improving error message quality
but still leaving it an error, but on reflection it looks like I
mis-read the intent because of confusion about the commit message).
Peter
12:23 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/2014 10:55 AM, Peter Krempa wrote:
How about changing one of the sentences to:
Rework early failure checking and set the error code to success before
iterating the array of disks so that we return success if no disks are
snapshotted.
Works for me.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:29 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/14 19:23, Eric Blake wrote:
Rework early failure checking and set the error code to success
before
> iterating the array of disks so that we return success if no disks are
> snapshotted.
Thanks; Series pushed with suggested fixes.
Peter
11:25 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: snapshot: Fix return value of external checkpoint with no disks
On 05/20/2014 10:16 AM, Eric Blake wrote:
Adding an option to 'virsh save' to not kill the guest doesn't make much
sense in general,
because memory state without matching disk state can't be used to resume
a guest.
but might make sense for a diskless VM. Meanwhile,
don't we already have the option to kill or keep the guest alive after
an external memory snapshot?
Here, I was thinking of VIR_DOMAIN_SNAPSHOT_CREATE_HALT.
Thus, 'virsh save $dom $file' appears to be shorthand for diskless
'virsh snapshot-create-as $dom --halt --memspec $file'
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:03 a.m.
On 05/20/2014 03:35 PM, Peter Krempa wrote:
Peter Krempa (4):
qemu: snapshot: Use typecasted switch in qemuDomainSnapshotPrepare()
qemu: snapshot: Forbid partial internal snapshots
qemu: snapshot: Forbid empty snapshots
qemu: snapshot: Fix return value of external checkpoint with no disks
src/qemu/qemu_driver.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
ACK series.
Jan
3869
days inactive
3869
days old
18 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa