[libvirt] [PATCH v2 5/7] qemu: driver: Move checkpoint-related code to qemu_checkpoint.c

[Eric Blake]

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 at 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