Commit 55c2ab3e accidentally introduced an infinite loop while checking whether a redefined snapshot would cause an infinite loop in chasing its parents back to a root. Alas, 'make check' did not catch it, so my next patch will be a testsuite improvement that would have hung and prevented the bug from being checked in to begin with. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 52abafab0f..cc3f71ab6f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -962,6 +962,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, vm->def->name); break; } + otherdef = virDomainSnapshotObjGetDef(other); } } -- 2.20.1

As the previous commit mentioned, argv mode (such as when you feed virsh via stdin with <<\EOF instead of via a single shell argument) didn't permit comments. Do this by treating any command name token that starts with # as a comment which silently eats all remaining arguments to the next newline or semicolon. Note that batch mode recognizes unquoted # at the start of any word as a command as part of the tokenizer, while this patch only treats # at the start of the command word as a comment (any other # remaining by the time vshCommandParse() is processing things was already quoted during the tokenzier, and as such was probably intended as the actual argument to the command word earlier in the line). Now I can do something like: $ virsh -c test:///default <<EOF # setup snapshot-create-as test s1 snapshot-create-as test s2 # check snapshot-list test --name EOF Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/virshtest.c | 1 + tools/virsh.pod | 3 ++- tools/virt-admin.pod | 4 +++- tools/vsh.c | 11 +++++++++-- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 5408db1387..3560e240a4 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -423,6 +423,7 @@ mymain(void) DO_TEST(44, "a # b\n", "echo a '#' b"); DO_TEST(45, "a # b\n", "echo a \\# b"); DO_TEST(46, "a\n", "#unbalanced 'quotes\"\necho a # b"); + DO_TEST(47, "a\n", "\\# ignored;echo a\n'#also' ignored"); # undef DO_TEST diff --git a/tools/virsh.pod b/tools/virsh.pod index 05adb568db..95cab7b57d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -35,7 +35,8 @@ will be clear for each of those commands. Note: it is permissible to give numeric names to domains, however, doing so will result in a domain that can only be identified by domain id. In other words, if a numeric value is supplied it will be interpreted as a domain id, not -as a name. +as a name. Any I<command> starting with B<#> is treated as a comment +and silently ignored, all other unrecognized I<command>s are diagnosed. The B<virsh> program can be used either to run one I<COMMAND> by giving the command and its arguments on the shell command line, or a I<COMMAND_STRING> diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod index 8489325ca9..f06ee9247a 100644 --- a/tools/virt-admin.pod +++ b/tools/virt-admin.pod @@ -18,7 +18,9 @@ The basic structure of most virt-admin usage is: virt-admin [OPTION]... <command> [ARG]... -Where I<command> is one of the commands listed below. +Where I<command> is one of the commands listed below. Any I<command> +starting with B<#> is treated as a comment and silently ignored, all +other unrecognized I<command>s are diagnosed. The B<virt-admin> program can be used either to run one I<COMMAND> by giving the command and its arguments on the shell command line, or a I<COMMAND_STRING> diff --git a/tools/vsh.c b/tools/vsh.c index 9a88ee29b7..d90ce8d102 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1437,8 +1437,15 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial) } if (cmd == NULL) { - /* first token must be command name */ - if (!(cmd = vshCmddefSearch(tkdata))) { + /* first token must be command name or comment */ + if (*tkdata == '#') { + do { + VIR_FREE(tkdata); + tk = parser->getNextArg(ctl, parser, &tkdata, false); + } while (tk == VSH_TK_ARG); + VIR_FREE(tkdata); + break; + } else if (!(cmd = vshCmddefSearch(tkdata))) { if (!partial) vshError(ctl, _("unknown command: '%s'"), tkdata); goto syntaxError; /* ... or ignore this command only? */ -- 2.20.1

Had this been in place earlier, I would have avoided the bugs in commit 0baf6945 and 55c2ab3e. Writing the test required me to extend the power of virsh - creating enough snapshots to cause fanout requires enough input in a single session that adding comments and markers makes it easier to check that output is correct. It's still a bit odd that with test:///default, reverting to a snapshot changes the domain from running to paused (possibly a bug in how the test driver copied from the qemu driver) - but the important part is that the test is reproducible, and any future tweaks we make to snapshot code have less chance of breaking successful command sequences. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/Makefile.am | 3 +- tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) create mode 100755 tests/virsh-snapshot diff --git a/tests/Makefile.am b/tests/Makefile.am index 29f1fe2d2a..d3cdbff8bb 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,6 +1,6 @@ ## Process this file with automake to produce Makefile.in -## Copyright (C) 2005-2015 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 @@ -410,6 +410,7 @@ libvirtd_test_scripts = \ virsh-schedinfo \ virsh-self-test \ virt-admin-self-test \ + virsh-snapshot \ virsh-start \ virsh-undefine \ virsh-uriprecedence \ diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot new file mode 100755 index 0000000000..3e6ff39f5f --- /dev/null +++ b/tests/virsh-snapshot @@ -0,0 +1,212 @@ +#!/bin/sh +# simple testing of snapshot APIs on test driver + +# Copyright (C) 2019 Red Hat, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 2 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see +# <http://www.gnu.org/licenses/>;. + +. "$(dirname $0)/test-lib.sh" + +if test "$VERBOSE" = yes; then + set -x + $abs_top_builddir/tools/virsh --version +fi + +fail=0 + +# The test driver loses states between restarts, so we perform a script +# with some convenient markers for later post-processing of output. +$abs_top_builddir/tools/virsh --connect test:///default >out 2>err ' + # Create a series of snapshots, with names that intentionally sort + # differently by topology than by name. Use revert to create fanout. + snapshot-create-as test s1 + snapshot-create-as test s3 + snapshot-create-as test s2 + snapshot-revert test s3 + snapshot-create-as test s6 + snapshot-create-as test s5 + snapshot-revert test s6 + snapshot-create-as test s4 + snapshot-revert test s1 + snapshot-create-as test s7 + snapshot-create-as test s8 + # Checking tree view (siblings sorted alphabetically) + snapshot-list test --tree + # Current was last one created, but we can change that + snapshot-current test --name + snapshot-current test s1 + snapshot-current test --name + # Deleting current root leads to multiple roots, demonstrate list filtering + snapshot-delete test --current + echo --err marker + snapshot-current test --name + echo --err marker + snapshot-list test --roots + snapshot-list test --leaves + snapshot-list test --parent --no-leaves + snapshot-list test --from s3 + snapshot-list test --from s3 --descendants --name + # More fun with delete flags, current node moves up to remaining parent + snapshot-current test s4 + snapshot-delete test --children-only s6 + snapshot-current test --name + snapshot-delete test --children s7 + snapshot-current test --name + snapshot-delete test s6 + snapshot-current test --name + # Now the tree is linear, so we have an unambiguous topological order + snapshot-list test --name + snapshot-list test --name --topological + # Capture some XML for later redefine + echo "<!--MarkerA-->" + snapshot-dumpxml test s3 + echo "<!--MarkerB-->" + snapshot-dumpxml test s2 + echo "<!--MarkerC-->" + # All done +' || fail=1 + +# First part is expected output, --tree results in trailing spaces, +# and --list produces timestamps +sed 's/ *$//; s/[0-9-]\{10\} [0-9:.]* .[0-9]\{4\}/TIMESTAMP/; + /MarkerA/,/MarkerC/d' < out > out.cooked || fail=1 +# Second part holds domain XMLs +sed -n '/MarkerA/,/MarkerB/p' < out > s3.xml || fail=1 +sed -n '/MarkerB/,/MarkerC/p' < out > s2.xml || fail=1 + +cat <<\EOF > exp || fail=1 +Domain snapshot s1 created +Domain snapshot s3 created +Domain snapshot s2 created + +Domain snapshot s6 created +Domain snapshot s5 created + +Domain snapshot s4 created + +Domain snapshot s7 created +Domain snapshot s8 created +s1 + | + +- s3 + | | + | +- s2 + | +- s6 + | | + | +- s4 + | +- s5 + | + +- s7 + | + +- s8 + + +s8 +Snapshot s1 set as current +s1 +Domain snapshot s1 deleted + + + + + Name Creation Time State +--------------------------------------------- + s3 TIMESTAMP running + s7 TIMESTAMP paused + + Name Creation Time State +--------------------------------------------- + s2 TIMESTAMP running + s4 TIMESTAMP paused + s5 TIMESTAMP paused + s8 TIMESTAMP paused + + Name Creation Time State Parent +------------------------------------------------------ + s3 TIMESTAMP running + s6 TIMESTAMP paused s3 + s7 TIMESTAMP paused + + Name Creation Time State +--------------------------------------------- + s2 TIMESTAMP running + s6 TIMESTAMP paused + +s2 +s4 +s5 +s6 + +Snapshot s4 set as current +Domain snapshot s6 children deleted + +s6 +Domain snapshot s7 deleted + +s6 +Domain snapshot s6 deleted + +s3 +s2 +s3 + +s3 +s2 + +EOF +compare exp out.cooked || fail=1 + +cat <<EOF > exp || fail=1 +error: marker +error: domain 'test' has no current snapshot +error: marker +EOF +compare exp err || fail=1 + +# Restore state with redefine +$abs_top_builddir/tools/virsh -c test:///default >out 2>err <<EOF || fail=1 + # Redefine must be in topological order; this will fail + snapshot-create test --redefine s2.xml + echo --err marker + # This is the right order + snapshot-create test --redefine s3.xml + snapshot-create test --redefine s2.xml --current + snapshot-info test --current +EOF + +cat <<\EOF > exp || fail=1 + + +Domain snapshot s3 created from 's3.xml' +Domain snapshot s2 created from 's2.xml' +Name: s2 +Domain: test +Current: yes +State: running +Location: internal +Parent: s3 +Children: 0 +Descendants: 0 +Metadata: yes + +EOF + +cat <<EOF > exp || fail=1 +error: invalid argument: parent s3 for snapshot s2 not found +error: marker +EOF +compare exp err || fail=1 + +(exit $fail); exit $fail -- 2.20.1

On 3/27/19 10:02 AM, Daniel P. Berrangé wrote: I don't want to force this on our docker and other CI environments, but we can do it via: This seems reasonable enough; could even be done as part of the virsh-snapshot test in isolation rather than globally in the Makefile for all tests. POSIX does not require HOME to stay constant, or to always match getent output (of course, when HOME and getent disagree, it's easier to get confused - but as long as we document it, we should be okay). This also seems nice - a cache is just that, after all; and failure to have a cache just means slower performance, but shouldn't mean extra noise. Since my test broke it, I'll play with ways to get rid of it before DV cuts the -rc2 build later this week. What is virsh trying to store in the cache, anyway? Is there a way to run virsh with a command line flag that says no caching is desired, so we don't even have to worry about where HOME points or whether it is writable? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org