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@xxxxxxxxxx>
> > ---
> > 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
