Re: [libvirt] [PATCH v2 5/7] qemu: driver: Move checkpoint-related code to qemu_checkpoint.c
On 9/25/19 7:54 AM, Peter Krempa wrote:
Move all extensive functions to a new file so that we don't just
pile
everything in the common files. This obviously isn't possible with
straight code movement as we still need stubs in qemu_driver.c
Additionally some functions e.g. for looking up a checkpoint by name
were so short that moving the impl didn't make sense.
Note that in the move the new file also doesn't use
virQEMUMomentReparent but rather an stripped down copy. As I plan to
s/an/a/
split out snapshot code into a separate file the unification
doesn't
make sense any more.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/Makefile.inc.am | 2 +
src/qemu/qemu_checkpoint.c | 483 +++++++++++++++++++++++++++++++++++++
src/qemu/qemu_checkpoint.h | 50 ++++
src/qemu/qemu_driver.c | 382 +----------------------------
4 files changed, 540 insertions(+), 377 deletions(-)
create mode 100644 src/qemu/qemu_checkpoint.c
create mode 100644 src/qemu/qemu_checkpoint.h
With the double-free fix you posted in the followup,
+++ b/src/qemu/Makefile.inc.am
@@ -68,6 +68,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_vhost_user.h \
qemu/qemu_vhost_user_gpu.c \
qemu/qemu_vhost_user_gpu.h \
+ qemu/qemu_checkpoint.c \
+ qemu/qemu_checkpoint.h \
$(NULL)
Is it worth keeping this list sorted alphabetically?
+/* Called inside job lock */
+static int
+qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr caps,
Worth splitting these parameters to separate lines? (I know you're just
moving code, and it's my fault they weren't split in my commit...)
+ virDomainObjPtr vm,
+ virDomainCheckpointDefPtr def)
+{
+
+struct virQEMUCheckpointReparent {
+ const char *dir;
+ virDomainMomentObjPtr parent;
+ virDomainObjPtr vm;
+ virCapsPtr caps;
+ virDomainXMLOptionPtr xmlopt;
+ int err;
+};
+
+
+static int
+qemuCheckpointReparentChildren(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *data)
+{
+ virDomainMomentObjPtr moment = payload;
+ struct virQEMUCheckpointReparent *rep = data;
There's a double space here we could fix.
+int
+qemuCheckpointDelete(virDomainObjPtr vm,
+ virDomainCheckpointPtr checkpoint,
+ unsigned int flags)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ virQEMUDriverPtr driver = priv->driver;
+ VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
+ int ret = -1;
+ virDomainMomentObjPtr chk = NULL;
+ virQEMUMomentRemove rem;
+ struct virQEMUCheckpointReparent rep;
You explained why you unshared virQEMUMomentReparent, but why not
instead move it to be alongside virQEMUMomentRemove, which is still
shared? Yes, I know it's odd that we had two callback structs, split
between two different files declaring those structs, but rather than
duplicating things for snapshot vs. checkpoint, keeping the two shared
structs side-by-side might make more sense.
+++ b/src/qemu/qemu_driver.c
static virDomainCheckpointPtr
qemuDomainCheckpointCreateXML(virDomainPtr domain,
const char *xmlDesc,
unsigned int flags)
{
- virQEMUDriverPtr driver = domain->conn->privateData;
virDomainObjPtr vm = NULL;
if (!(vm = qemuDomainObjFromDomain(domain)))
goto cleanup;
- if (virDomainSnapshotObjListNum(vm->snapshots, NULL, 0) > 0) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot create checkpoint while snapshot exists"));
- goto cleanup;
- }
-
- priv = vm->privateData;
- cfg = virQEMUDriverGetConfig(driver);
-
if (virDomainCheckpointCreateXMLEnsureACL(domain->conn, vm->def, flags) <
0)
goto cleanup;
Didn't you need to fix this separately, for security reasons?
Otherwise, the code motion makes sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org