10:44 a.m.
On Thu, Sep 26, 2019 at 10:38:11 -0500, Eric Blake wrote:
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.
I plan also to split everything snapshot related out so I thought of
getting completely rid of the code sharing here since it's not
significant.
> +++ 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?
Oops, I didn't notice this one. I'll post it separately probably just to
have a commit to reference.