Re: [libvirt] [PATCHv2 2/7] snapshot: virsh snapshot-list and children

Sometimes, we only care about one branch of the snapshot hierarchy. Make it easier to list a single branch, by using the new APIs. Technically, I could emulate these new virsh options on old servers by doing a complete dump, then scraping xml to filter out just the snapshots that I care about, but I didn't want to do that in this patch. * tools/virsh.c (cmdSnapshotList): Add --from, --descendants. * tools/virsh.pod (snapshot-list): Document them. --- v2: minor rebase cleanups tools/virsh.c | 76 ++++++++++++++++++++++++++++++++++++++++++++----------- tools/virsh.pod | 9 ++++++- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 955b8df..adafe86 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13102,6 +13102,8 @@ static const vshCmdOptDef opts_snapshot_list[] = { {"metadata", VSH_OT_BOOL, 0, N_("list only snapshots that have metadata that would prevent undefine")}, {"tree", VSH_OT_BOOL, 0, N_("list snapshots in a tree")}, + {"from", VSH_OT_DATA, 0, N_("limit list to children of given snapshot")}, + {"descendants", VSH_OT_BOOL, 0, N_("with --from, list all descendants")}, {NULL, 0, 0, NULL} }; @@ -13128,25 +13130,36 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char timestr[100]; struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); + const char *from = NULL; + virDomainSnapshotPtr start = NULL; + + if (vshCommandOptString(cmd, "from", &from) < 0) { + vshError(ctl, _("invalid from argument '%s'"), from); + goto cleanup; + } if (vshCommandOptBool(cmd, "parent")) { if (vshCommandOptBool(cmd, "roots")) { vshError(ctl, "%s", - _("--parent and --roots are mutually exlusive")); + _("--parent and --roots are mutually exclusive")); return false; } if (tree) { vshError(ctl, "%s", - _("--parent and --tree are mutually exlusive")); + _("--parent and --tree are mutually exclusive")); return false; } parent_filter = 1; } else if (vshCommandOptBool(cmd, "roots")) { if (tree) { vshError(ctl, "%s", - _("--roots and --tree are mutually exlusive")); + _("--roots and --tree are mutually exclusive")); return false; } + if (from) { + vshError(ctl, "%s", + _("--roots and --from are mutually exclusive")); + } flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; } @@ -13161,16 +13174,29 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - numsnaps = virDomainSnapshotNum(dom, flags); - - /* Fall back to simulation if --roots was unsupported. */ - if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && - (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { - virFreeError(last_error); - last_error = NULL; - parent_filter = -1; - flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + if (from) { + start = virDomainSnapshotLookupByName(dom, from, 0); + if (!start) + goto cleanup; + if (vshCommandOptBool(cmd, "descendants") || tree) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + } + numsnaps = virDomainSnapshotNumChildren(start, flags); + if (numsnaps >= 0 && tree) + numsnaps++; + /* XXX Is it worth emulating --from on older servers? */ + } else { numsnaps = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + parent_filter = -1; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + numsnaps = virDomainSnapshotNum(dom, flags); + } } if (numsnaps < 0) @@ -13196,14 +13222,32 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (VIR_ALLOC_N(names, numsnaps) < 0) goto cleanup; - actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); + if (from) { + /* When mixing --from and --tree, we want to start the tree at the + * given snapshot. Without --tree, only list the children. */ + if (tree) { + if (numsnaps) + actual = virDomainSnapshotListChildrenNames(start, names + 1, + numsnaps - 1, + flags); + if (actual >= 0) { + actual++; + names[0] = vshStrdup(ctl, from); + } + } else { + actual = virDomainSnapshotListChildrenNames(start, names, + numsnaps, flags); + } + } else { + actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); + } if (actual < 0) goto cleanup; if (tree) { char indentBuf[INDENT_BUFLEN]; - char **parents = vshMalloc(ctl, sizeof(char *) * actual); - for (i = 0; i < actual; i++) { + char **parents = vshCalloc(ctl, sizeof(char *), actual); + for (i = (from ? 1 : 0); i < actual; i++) { /* free up memory from previous iterations of the loop */ if (snapshot) virDomainSnapshotFree(snapshot); @@ -13298,6 +13342,8 @@ cleanup: VIR_FREE(state); if (snapshot) virDomainSnapshotFree(snapshot); + if (start) + virDomainSnapshotFree(start); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); diff --git a/tools/virsh.pod b/tools/virsh.pod index 1f7c76f..dd60a64 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1969,7 +1969,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] -[I<--metadata>] +[I<--metadata>] [[I<--from>] B<snapshot> [I<--descendants>]] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -1980,6 +1980,13 @@ the list will be filtered to just snapshots that have no parents. If I<--tree> is specified, the output will be in a tree format, listing just snapshot names. These three options are mutually exclusive. +If I<--from> is provided, filter the list to snapshots which are +children of the given B<snapshot>. When used in isolation or with +I<--parent>, the list is limited to direct children unless +I<--descendants> is also present. When used with I<--tree>, the +use of I<--descendants> is implied. This option is not compatible +with I<--roots>. + If I<--metadata> is specified, the list will be filtered to just snapshots that involve libvirt metadata, and thus would prevent B<undefine> of a persistent domain, or be lost on B<destroy> of