On 3/27/19 9:29 AM, Ján Tomko wrote:
> On Wed, Mar 27, 2019 at 05:10:46AM -0500, 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>
>> + 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));
>
> We have vshTableNew and vshTableRowAppend that can compute the
> indentation at run-time regardless of the locale.
Nice! Those were added after I forked from snapshot code 6 months ago. I
wonder if I can get some code sharing between the two in virsh the way I
did with code sharing in src/conf/.
>> +
>> + if (tree) {
>> + for (i = 0; i < chklist->nchks; i++) {
>> + if (!chklist->chks[i].parent &&
>> + vshTreePrint(ctl, virshCheckpointListLookup, chklist,
>> + chklist->nchks, i) < 0)
>> + goto cleanup;
>> + }
>> + ret = true;
>> + goto cleanup;
>> + }
>> +
>> + for (i = 0; i < chklist->nchks; i++) {
>
> Putting this whole loop in a separate function would make this function
> more readable - both tree and non-tree would share the same 'ret = true'
> assignment and, more importantly, all the XML parsing stuff is only
> needed for non-tree printing.
Technical debt. I should fix snapshots to be smarter first, and then
copy the smarter solution first-try into checkpoints.
>
>> + const char *chk_name;
>> +
>> + /* free up memory from previous iterations of the loop */
>
> Sounds like a job for VIR_AUTOFREE
Yes, quite a few places. Point them out, and I'll gladly use it.
>> +char **
>> +virshCheckpointNameCompleter(vshControl *ctl,
>> + const vshCmd *cmd,
>> + unsigned int flags)
>> +{
>> + virshControlPtr priv = ctl->privData;
>> + virDomainPtr dom = NULL;
>> + virDomainCheckpointPtr *checkpoints = NULL;
>> + int ncheckpoints = 0;
>> + size_t i = 0;
>> + char **ret = NULL;
>> +
>> + virCheckFlags(0, NULL);
>> +
>> + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
>> + return NULL;
>> +
>> + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>> + return NULL;
>> +
>> + if ((ncheckpoints = virDomainListAllCheckpoints(dom, &checkpoints,
>> + flags)) < 0)
>
> I recommend adding an 'int rc' variable and only assigning the result to
> ncheckpoints if it's non-negative,
Makes sense.
>
>> + goto error;
>> +
>> + if (VIR_ALLOC_N(ret, ncheckpoints + 1) < 0)
>> + goto error;
>> +
>> + for (i = 0; i < ncheckpoints; i++) {
>> + const char *name = virDomainCheckpointGetName(checkpoints[i]);
>> +
>> + if (VIR_STRDUP(ret[i], name) < 0)
>> + goto error;
>> +
>> + virshDomainCheckpointFree(checkpoints[i]);
>> + }
>> + VIR_FREE(checkpoints);
>> + virshDomainFree(dom);
>> +
>> + return ret;
>> +
>> + error:
>> + for (; i < ncheckpoints; i++)
>> + virshDomainCheckpointFree(checkpoints[i]);
>
> otherwise this loop will try to run for a long time.
> Also, given that we free these on success too, having a shared 'cleanup'
> path would be nicer.
Snapshots has the same bug, looks like fixing that is obvious 5.2
material, regardless of the decision on the rest of this series.
(I had no idea I'd be encountering so much yak's hair when I used
snapshots as my starting point...)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
