On 2/12/19 1:15 PM, John Ferlan wrote:
>
>
> 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@xxxxxxxxxx>
>> ---
>> 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.
>
Gah. I completely forgot to copy-and-paste that portion of the snapshot
code. Will rectify.
> 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
Yep, on my list of things to clean up where I spot them, to avoid
regressing to the older state-of-the-art when I first copied this code
from snapshots.
>> +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
Particularly since it's not a snapshot. (Too much copy-and-paste...)
>> + /* TODO - worth adding this flag?
>> + {.name = "quiesce",
>> + .type = VSH_OT_BOOL,
>> + .help = N_("quiesce guest's file systems")
>> + },
>> + */
>
> More to do
At least adding it here is easy. Adding it in qemu is harder.
>> +/*
>> + * "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.
Modeled after "snapshot-edit" - but yes, it is a scary-enough command
that splitting it to a separate commit doesn't hurt my feelings (nor
hold up the initial API).
>> +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.
Modeled after the existing 'snapshot-current', which can be used to
redefine WHICH snapshot is marked current (but oddly enough lacks a way
to mark NO snapshot as current). It uses the _REDEFINE flag to do an
in-place edit to the virDomainCheckpointObjList on which checkpoint is
marked current (and based on the use of the _CURRENT flag to state which
checkpoint is the new current one).
>> +
>> + 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
Yeah. And isn't the new norm to use the new table-printer that formats
columns nicely according to content width? I'll have to see if snapshot
code has improved here in the meantime.
>> + /* 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?
Knowing that you have 1 child but 3 descendents (and therefore, 2 of the
3 descendents are grandchildren or below) is interesting, if only to
prove that the filtering API flags work :)
>> + /* 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.
And that snapshot-list has just as 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.
Snapshots can definitely be more than linear. I'm not sure if
checkpoints can easily become more than linear, but I don't want to rule
it out (after all, reverting to prior states can do weird things).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
