Re: [libvirt] [PATCH v4 13/20] backup: Implement virsh support for checkpoints

On 2/6/19 2:18 PM, Eric Blake wrote: > Introduce a bunch of new virsh commands for managing checkpoints > in isolation. More commands are needed for performing incremental > backups, but these commands were easy to implement by modeling > heavily after virsh-snapshot.c (no need for checkpoint-revert, > and checkpoint-list was a lot easier since we don't have to cater > to older libvirt API). > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > tools/virsh-checkpoint.h | 29 + > tools/virsh-completer.h | 4 + > tools/virsh-util.h | 3 + > tools/virsh.h | 1 + > po/POTFILES | 1 + > tools/Makefile.am | 3 +- > tools/virsh-checkpoint.c | 1329 ++++++++++++++++++++++++++++++++++++++ > tools/virsh-completer.c | 52 +- > tools/virsh-util.c | 11 + > tools/virsh.c | 2 + > 10 files changed, 1433 insertions(+), 2 deletions(-) > create mode 100644 tools/virsh-checkpoint.h > create mode 100644 tools/virsh-checkpoint.c virsh.pod would be useful in order to figure out what everything means.

This is just too much being added here to really give this a proper review. This really is a case where too much has been built up. Start with the basics and build up from there. I have similar things as previously w/r/t 2 blank lines between functions and one argument per line per method - all newer norms than when -snapshot was created. Also may as well use the VIR_AUTOFREE type functions for (char *) VIR_FREE's. And VIR_AUTOPTR(virString) as approriate

> +static bool > +virshCheckpointCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, > + unsigned int flags, const char *from) > +{ > + bool ret = false; > + virDomainCheckpointPtr checkpoint; > + const char *name = NULL; > + > + checkpoint = virDomainCheckpointCreateXML(dom, buffer, flags); > + > + if (checkpoint == NULL) > + goto cleanup; > + > + name = virDomainCheckpointGetName(checkpoint); > + if (!name) { > + vshError(ctl, "%s", _("Could not get snapshot name")); Could not find domain checkpoint '%s' would be better

> + /* TODO - worth adding this flag? > + {.name = "quiesce", > + .type = VSH_OT_BOOL, > + .help = N_("quiesce guest's file systems") > + }, > + */ More to do

> +/* > + * "checkpoint-edit" command > + */ Oh and this is *really* a scary thing to allow! Especially since IIRC the Assign function would just create a new one if a name didn't exist... There seems to be a few things that could happen without locks that could really mess things up.

> +static bool > +cmdCheckpointCurrent(vshControl *ctl, const vshCmd *cmd) > +{ > + virDomainPtr dom = NULL; > + bool ret = false; > + int current; > + virDomainCheckpointPtr checkpoint = NULL; > + char *xml = NULL; > + const char *checkpointname = NULL; > + unsigned int flags = 0; > + const char *domname; > + > + if (vshCommandOptBool(cmd, "security-info")) > + flags |= VIR_DOMAIN_CHECKPOINT_XML_SECURE; > + if (vshCommandOptBool(cmd, "no-domain")) > + flags |= VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN; > + if (vshCommandOptBool(cmd, "size")) > + flags |= VIR_DOMAIN_CHECKPOINT_XML_SIZE; > + > + VSH_EXCLUSIVE_OPTIONS("name", "checkpointname"); > + > + if (!(dom = virshCommandOptDomain(ctl, cmd, &domname))) > + return false; > + > + if (vshCommandOptStringReq(ctl, cmd, "checkpointname", &checkpointname) < 0) > + goto cleanup; > + > + if (checkpointname) { > + virDomainCheckpointPtr checkpoint2 = NULL; > + flags = (VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE | > + VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT); > + > + if (!(checkpoint = virDomainCheckpointLookupByName(dom, > + checkpointname, 0))) > + goto cleanup; > + > + xml = virDomainCheckpointGetXMLDesc(checkpoint, > + VIR_DOMAIN_CHECKPOINT_XML_SECURE); > + if (!xml) > + goto cleanup; > + > + if (!(checkpoint2 = virDomainCheckpointCreateXML(dom, xml, flags))) > + goto cleanup; I need a better map. When creating a checkpoint a couple patches ago, the AssignDef would fail if the name already existed. This code seems to take existing XML with a name that would already exist.

> + > + dom = virshCommandOptDomain(ctl, cmd, NULL); > + if (dom == NULL) > + return false; > + > + if (virshLookupCheckpoint(ctl, cmd, "checkpointname", true, dom, > + &checkpoint, &name) < 0) > + goto cleanup; > + > + vshPrint(ctl, "%-15s %s\n", _("Name:"), name); > + vshPrint(ctl, "%-15s %s\n", _("Domain:"), virDomainGetName(dom)); Meaning we could have partial printing if the subequent fails

> + /* Children, Descendants. */ > + flags = 0; > + count = virDomainCheckpointListChildren(checkpoint, NULL, flags); > + if (count < 0) { > + if (last_error->code == VIR_ERR_NO_SUPPORT) { > + vshResetLibvirtError(); > + ret = true; > + } > + goto cleanup; > + } > + vshPrint(ctl, "%-15s %d\n", _("Children:"), count); > + flags = VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS; > + count = virDomainCheckpointListChildren(checkpoint, NULL, flags); > + if (count < 0) > + goto cleanup; > + vshPrint(ctl, "%-15s %d\n", _("Descendants:"), count); Is delineating children and desendants necessary?

> + /* When mixing --from and --tree, we also want a copy of from > + * in the list, but with no parent for that one entry. */ Still mumbling about too many options.

That's a lot of different options... I'm confused over multiple checkpoints without a parent. I would figure a checkpoint without a parent is the root or first checkpoint and children are essentially linear. But this tree concept just complicates things so much.