virsh: Add snapshot-list --topological

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 <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 025321c58e0aa499c48059f27ba076082aea94bc..6ea6e2744ab269d226fb871a22ac7fd0dd5d99f7 100644 (file)
--- 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 5759a396d4082eec259e39323cf66b171d0801c4..66e2bf24ecd380520ee36b7e20e07cf003eeb064 100644 (file)
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4726,7 +4726,7 @@ Output basic information about a named <snapshot>, or the current snapshot
 with I<--current>.
 
 =item B<snapshot-list> I<domain> [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<snapshot> | 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.