Re: [libvirt] [PATCH v7 15/23] backup: Implement virsh support for checkpoints

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(a)redhat.com&gt;

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

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

> +        const char *chk_name; > + > +        /* free up memory from previous iterations of the loop */ Sounds like a job for VIR_AUTOFREE

> +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,

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