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

[Peter Krempa]

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

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.