From c615c14246880c91afa3fe1659dc2104447de601 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 7 Mar 2019 22:12:01 -0600 Subject: [PATCH] virsh: Add snapshot-list --topological MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For snapshots, virsh already has a (shockingly naive [1]) client-side topological sorter with the --tree option. But as a series of REDEFINE calls must be presented in topological order, it's worth letting the server do the work for us, especially since the server can give us a topological sorting with less effort than our naive client reconstruction. [1] The XXX comment in virshSnapshotListCollect() about --tree being O(n^3) is telling; https://en.wikipedia.org/wiki/Topological_sorting is an interesting resource describing Kahn's algorithm and other approaches for O(n) topological sorting for anyone motivated to use a more elegant algorithm than brute force - but that doesn't affect this patch. For now, I am purposefully NOT implementing virsh fallback code to provide a topological sort when the flag was rejected as unsupported; we can worry about that down the road if users actually demonstrate that they use new virsh but old libvirt to even need the fallback. (The code we use for --tree could be repurposed to be such a fallback, whether or not we keep it naive or improve it to be faster - but again, no one should spend time on a fallback without evidence that we need it.) The test driver makes it easy to test: $ virsh -c test:///default ' snapshot-create-as test a snapshot-create-as test c snapshot-create-as test b snapshot-list test snapshot-list test --topological snapshot-list test --descendants a snapshot-list test --descendants a --topological snapshot-list test --tree snapshot-list test --tree --topological ' Without any flags, virsh does client-side sorting alphabetically, and lists 'b' before 'c' (even though 'c' is the parent of 'b'); with the flag, virsh skips sorting, and you can now see that the server handed back data in a correct ordering. As shown here with a simple linear chain, there isn't any other possible ordering, so --tree mode doesn't seem to care whether --topological is used. But it is possible to compose more complicated DAGs with multiple children to a parent (representing reverting back to a snapshot then creating more snapshots along those divergent execution timelines), where it is then possible (but not guaranteed) that adding the --topological flag changes the --tree output (the client-side --tree algorithm breaks ties based on alphabetical sorting between two nodes that share the same parent, while the --topological sort skips the client-side alphabetical sort and ends up exposing the server's internal order for siblings, whether that be historical creation order or dependent on a random hash seed). But even if the results differ, they will still be topologically correct. Signed-off-by: Eric Blake Reviewed-by: Ján Tomko Reviewed-by: Daniel P. Berrangé --- tools/virsh-snapshot.c | 14 +++++++++++--- tools/virsh.pod | 7 ++++++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 025321c58e..6ea6e2744a 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1,7 +1,7 @@ /* * virsh-snapshot.c: Commands to manage domain snapshot * - * Copyright (C) 2005, 2007-2016 Red Hat, Inc. + * Copyright (C) 2005-2019 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1351,8 +1351,9 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, } } } - qsort(snaplist->snaps, snaplist->nsnaps, sizeof(*snaplist->snaps), - virshSnapSorter); + if (!(orig_flags & VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL)) + qsort(snaplist->snaps, snaplist->nsnaps, sizeof(*snaplist->snaps), + virshSnapSorter); snaplist->nsnaps -= deleted; VIR_STEAL_PTR(ret, snaplist); @@ -1451,6 +1452,10 @@ static const vshCmdOptDef opts_snapshot_list[] = { .type = VSH_OT_BOOL, .help = N_("list snapshot names only") }, + {.name = "topological", + .type = VSH_OT_BOOL, + .help = N_("sort list topologically rather than by name"), + }, {.name = NULL} }; @@ -1512,6 +1517,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) FILTER("external", EXTERNAL); #undef FILTER + if (vshCommandOptBool(cmd, "topological")) + flags |= VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL; + if (roots) flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; diff --git a/tools/virsh.pod b/tools/virsh.pod index 5759a396d4..66e2bf24ec 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4726,7 +4726,7 @@ Output basic information about a named , or the current snapshot with I<--current>. =item B I [I<--metadata>] [I<--no-metadata>] -[{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}] +[{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}] [I<--topological>] [{[I<--from>] B | I<--current>} [I<--descendants>]] [I<--leaves>] [I<--no-leaves>] [I<--inactive>] [I<--active>] [I<--disk-only>] [I<--internal>] [I<--external>] @@ -4734,6 +4734,11 @@ with I<--current>. List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. +Normally, table form output is sorted by snapshot name; using +I<--topological> instead sorts so that no child is listed before its +ancestors (although there may be more than one possible ordering with +this property). + If I<--parent> is specified, add a column to the output table giving the name of the parent of each snapshot. If I<--roots> is specified, the list will be filtered to just snapshots that have no parents. -- GitLab