8:05 a.m.
Patches 1 and 2 are new in the series and add an option to override the check so
that users can use the feature despite being unsafe.
See patch 3/3 for clarification of the motiovation to do this.
Peter Krempa (3):
virerror: Introduce VIR_ERR_OPERATION_UNSAFE
snapshot: Introduce flag VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE
qemu: snapshot: Forbid internal snapshots with pflash firmware
include/libvirt/libvirt-domain-snapshot.h | 3 +++
include/libvirt/virterror.h | 1 +
src/qemu/qemu_driver.c | 18 +++++++++++++++++-
src/util/virerror.c | 5 +++++
tools/virsh-snapshot.c | 6 ++++++
tools/virsh.pod | 6 +++++-
6 files changed, 37 insertions(+), 2 deletions(-)
--
2.12.1
8:05 a.m.
New subject: [libvirt] [PATCH v3 1/3] virerror: Introduce VIR_ERR_OPERATION_UNSAFE
Similarly to VIR_ERR_MIGRATION_UNSAFE add a error code which will be
reported when an operation is possible with the hypervisor but may lead
to data loss or other problems in certain cases. This error code
notifies the user that the operation can be forced using a specific
flag.
---
include/libvirt/virterror.h | 1 +
src/util/virerror.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 2efee8f0c..04fea2e34 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -319,6 +319,7 @@ typedef enum {
VIR_ERR_AGENT_UNSYNCED = 97, /* guest agent replies with wrong id
to guest-sync command */
VIR_ERR_LIBSSH = 98, /* error in libssh transport driver */
+ VIR_ERR_OPERATION_UNSAFE = 99, /* unsafe operation requiring override */
} virErrorNumber;
/**
diff --git a/src/util/virerror.c b/src/util/virerror.c
index ef17fb5e6..1437cf02b 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1407,6 +1407,11 @@ virErrorMsg(virErrorNumber error, const char *info)
else
errmsg = _("libssh transport error: %s");
break;
+ case VIR_ERR_OPERATION_UNSAFE:
+ if (info == NULL)
+ errmsg = _("operation unsafe");
+ else
+ errmsg = _("operation unsafe: %s");
}
return errmsg;
}
--
2.12.1
8:05 a.m.
New subject: [libvirt] [PATCH v3 2/3] snapshot: Introduce flag VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE
Introduce a flag that will allow to override safety checks in certain
snapshot configurations.
---
include/libvirt/libvirt-domain-snapshot.h | 3 +++
src/qemu/qemu_driver.c | 3 ++-
tools/virsh-snapshot.c | 6 ++++++
tools/virsh.pod | 6 +++++-
4 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h
b/include/libvirt/libvirt-domain-snapshot.h
index 0f73f24b2..9d1da8710 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -70,6 +70,9 @@ typedef enum {
VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot
while the guest is
running */
+ VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE = (1 << 9), /* override safety checks
+ for certain
+ configurations */
} virDomainSnapshotCreateFlags;
/* Take a snapshot of the current VM state */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 676295208..02cdd2f6b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14431,7 +14431,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
- VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
+ VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
+ VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE, NULL);
VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 5c844a5ea..48ad6f2ef 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -364,6 +364,10 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
.flags = VSH_OFLAG_REQ_OPT,
.help = N_("memory attributes: [file=]name[,snapshot=type]")
},
+ {.name = "unsafe",
+ .type = VSH_OT_BOOL,
+ .help = N_("override unsafe operation")
+ },
{.name = "diskspec",
.type = VSH_OT_ARGV,
.help = N_("disk attributes:
disk[,snapshot=type][,driver=type][,file=name]")
@@ -404,6 +408,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
if (vshCommandOptBool(cmd, "live"))
flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
+ if (vshCommandOptBool(cmd, "unsafe"))
+ flags |= VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index ee7904611..62bca4b45 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4104,7 +4104,7 @@ used to represent properties of snapshots.
=item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine>
[I<--current>]]
| [I<--no-metadata>] [I<--halt>] [I<--disk-only>]
[I<--reuse-external>]
-[I<--quiesce>] [I<--atomic>] [I<--live>]}
+[I<--quiesce>] [I<--atomic>] [I<--live>] [I<--unsafe>]}
Create a snapshot for domain I<domain> with the properties specified in
I<xmlfile>. Normally, the only properties settable for a domain snapshot
@@ -4163,6 +4163,10 @@ the guest is running. Both disk snapshot and domain memory snapshot
are
taken. This increases the size of the memory image of the external
checkpoint. This is currently supported only for external checkpoints.
+Certain snapshot configurations may be unsafe but historically used to work. You
+can specify the I<--unsafe> flag if a snapshot operation is forbidden due to
+being unsafe.
+
Existence of snapshot metadata will prevent attempts to B<undefine>
a persistent domain. However, for transient domains, snapshot
metadata is silently lost when the domain quits running (whether
--
2.12.1
8:05 a.m.
New subject: [libvirt] [PATCH v3 3/3] qemu: snapshot: Forbid internal snapshots with pflash firmware
If the variable store (<nvram>) file is raw qemu can't do a snapshot of
it and thus the snapshot would be incomplete. QEMU does no reject such
snapshot.
Additionally allowing to use a qcow2 variable store backing file would
solve this issue but then it would become eligible to become target of
the memory dump.
Offline internal snapshot would be incomplete too with either storage
format since libvirt does not handle the pflash file in this case.
Forbid such snapshot so that we can avoid problems.
---
Notes:
v3:
- allow overriding of the check by specifying VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE
- report VIR_ERR_OPERATION_UNSAFE (instead of VIR_ERR_OPERATION_UNSUPPORTED)
- tweaked commend in code (since it's not forbidden completely)
- tweaked error message
v2:
- changed error code to OPERATION_UNSUPPORTED (from CONFIG_UNSUPPORTED)
- dropped mention of QEMU from the error message
- dropped mentions of OVMF or the firmware itself altoghether, the culprit is
the pflash device regardless of the software it contains
- mentioned all the stuff in the commit message and comment
We also will need to introduce a way to snapshot the pflash for external
snapshots which is currently impossible as well, but fortunately does not
have inherent drawbacks as internal snapshots.
src/qemu/qemu_driver.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 02cdd2f6b..2ca839f1c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13754,6 +13754,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
bool active = virDomainObjIsActive(vm);
bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
+ bool unsafe = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE) != 0;
bool found_internal = false;
bool forbid_internal = false;
int external = 0;
@@ -13873,6 +13874,20 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
goto cleanup;
}
+ /* internal snapshots + pflash based loader have the following problems:
+ * - if the variable store is raw, the snapshot is incomplete
+ * - alowing a qcow2 image as the varstore would make it eligible to receive
+ * the vmstate dump, which would make it huge
+ * - offline snapshot would not snapshot the varstore at all
+ */
+ if (!unsafe && found_internal &&
+ vm->def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) {
+ virReportError(VIR_ERR_OPERATION_UNSAFE, "%s",
+ _("internal snapshots of a VM with pflash based "
+ "firmware can corrupt the nvram data"));
+ goto cleanup;
+ }
+
/* Alter flags to let later users know what we learned. */
if (external && !active)
*flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
--
2.12.1
8:25 a.m.
On Fri, Mar 24, 2017 at 14:05:39 +0100, Peter Krempa wrote:
Patches 1 and 2 are new in the series and add an option to override
the check so
that users can use the feature despite being unsafe.
See patch 3/3 for clarification of the motiovation to do this.
Peter Krempa (3):
virerror: Introduce VIR_ERR_OPERATION_UNSAFE
snapshot: Introduce flag VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE
qemu: snapshot: Forbid internal snapshots with pflash firmware
Self NACK series. Turns out that our HMP error detection for 'savevm'
sucks. Snapshots with PFLASH are not supported at all, but libvirt
thinks they are. V2 is the way to go with mentioning that actually it's
a libvirt bug.
2831
days inactive
2831
days old
4 comments
1 participants
participants (1)
-
Peter Krempa