Separate the algorithm for which list members to vist (which is
generic and can be shared with checkpoints, provided that common
filtering bits are either declared with the same value or have a
mapping from public API to common value) from the decision on which
members to return (which is specific to snapshots). The typedef for
the callback function feels a bit heavy here, but will make it easier
to move the common portions in a later patch.
As part of the refactoring, note that the macros for selecting filter
bits are specific to listing functionality, so they belong better in
virdomainsnapshotobjlist.h (missed in commit 9b75154c).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
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>
In these cases the check that is removed has been done a few
lines above already (as can even be seen in the context). Drop
them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Upcoming patches plan to introduce virDomainCheckpointPtr as a new
object for use in incremental backups, along with documentation on
how incremental backups differ from snapshots. But first, we need
to rename any existing mention of a 'system checkpoint' to instead
be a 'full system snapshot', so that we aren't overloading
the term checkpoint.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The root snapshot does not have a parent.
Use NULLSTR_EMPTY to pass an empty string instead of putting
too few columns in the table.
https://bugzilla.redhat.com/show_bug.cgi?id=1662849
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Use the newly introduced macro in the few places that open-code it.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
We have this very handy macro called VIR_STEAL_PTR() which steals
one pointer into the other and sets the other to NULL. The
following coccinelle patch was used to create this commit:
@ rule1 @
identifier a, b;
@@
- b = a;
...
- a = NULL;
+ VIR_STEAL_PTR(b, a);
Some places were clean up afterwards to make syntax-check happy
(e.g. some curly braces were removed where the body become a one
liner).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
In 600462834f we've tried to remove Author(s): lines
from comments at the beginning of our source files. Well, in some
files while we removed the "Author" line we did not remove the
actual list of authors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The helper function virshSnapshotCreate (formerly vshSnapshotCreate)
has had dead variables since commit a00c37f2 (Sep 2011).
Signed-off-by: Eric Blake <eblake@redhat.com>
centralize the definition of macro VIRSH_COMMON_OPT_DOMAIN_FULL to virsh.h
to avoid unnecessary duplicated definition
Signed-off-by: Lin Ma <lma@suse.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Now that we have everything prepared let the fun begin. This
completer is very simple and returns domain names. Moreover,
depending on the command it can return just a subset of domains
(e.g. only running/paused/transient/.. ones).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In some cases there's dangling backward slash at the end of multi
line macros. While technically the code works, it will stop if
some empty lines are removed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Right-aligning backslashes when defining macros or using complex
commands in Makefiles looks cute, but as soon as any changes is
required to the code you end up with either distractingly broken
alignment or unnecessarily big diffs where most of the changes
are just pushing all backslashes a few characters to one side.
Generated using
$ git grep -El '[[:blank:]][[:blank:]]\\$' | \
grep -E '*\.([chx]|am|mk)$$' | \
while read f; do \
sed -Ei 's/[[:blank:]]*[[:blank:]]\\$/ \\/g' "$f"; \
done
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
virDomainFree has it's quirks (does not like NULL pointers, resets
libvirt errors). Replace it by a virsh helper which will allow us to
centrally fix issues with it.
The syntax-check rule will prohibit new uses of virDomainFree.
Move virshLookupDomainBy, virshCommandOptDomainBy and
virshCommandOptDomainBy to the helper file. Additionally turn the
virshCommandOptDomainBy macro into a function.
We have couple of functions that operate over NULL terminated
lits of strings. However, our naming sucks:
virStringJoin
virStringFreeList
virStringFreeListCount
virStringArrayHasString
virStringGetFirstWithPrefix
We can do better:
virStringListJoin
virStringListFree
virStringListFreeCount
virStringListHasString
virStringListGetFirstWithPrefix
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Although there already was an effort (b620bdee) to replace vshPrint occurrences
with vshPrintExtra due to '--quiet' flag, there were still some leftovers. So
this patch fixes them, hopefully for good.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1356881
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Turn various vshPrint() informative messages into vshPrintExtra(), so
they are not printed when requesting the quiet mode; neither XML/info
outputs nor the results of commands are affected.
Also change the expected outputs of the virsh-undefine test, since virsh
is invoked in quiet mode there.
Some informative messages might still be converted (and thus silenced
when in quiet mode), but this is an improvements nonetheless.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1358179
Rather than continually cut-n-paste the strings into each command,
create a common macro to be used generically. The macro will take a
single argument _helpstr which for many options in virsh-domain.c
is simply "affect current domain". So, create a second macro within that
file in order to define the more common use as a revector to the
common macro with the common _helpstr.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Rather than continually cut-n-paste the strings into each command,
create a common macro to be used generically. The macro will take a
single argument _helpstr which for many options in virsh-domain.c
is simply "affect running domain". So, create a second macro within that
file in order to define the more common use as a revector to the
common macro with the common _helpstr.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Rather than continually cut-n-paste the strings into each command,
create a common macro to be used generically. The macro will take a
single argument _helpstr which will be used to pass the translatable
helpstr since not all domain options can take the same string.
The majority of the options take 'N_("domain name, id or uuid")', so
create a separate macro with a _FULL suffix while those that do not
take the same string will use the VIRSH_COMMON_OPT_DOMAIN macro.
Signed-off-by: John Ferlan <jferlan@redhat.com>
In order to share as much virsh' logic as possible with upcomming
virt-admin client we need to split virsh logic into virsh specific and
client generic features.
Since majority of virsh methods should be generic enough to be used by
other clients, it's much easier to rename virsh specific data to virshX
than doing this vice versa. It moved generic virsh commands (including info
and opts structures) to generic module vsh.c.
Besides renaming methods and structures, this patch also involves introduction
of a client specific control structure being referenced as private data in the
original control structure, introduction of a new global vsh Initializer,
which currently doesn't do much, but there is a potential for added
functionality in the future.
Lastly it introduced client hooks which are especially necessary during
client connecting phase.
This will allow us to use vshError() to report errors from inside
vshCommandOpt*(), instead of replicating the same logic and error
messages all over the place.
We also have more context inside the vshCommandOpt*() functions,
for example the actual value used on the command line, which means
we can produce more detailed error messages.
vshCommandOptBool() is the exception here, because it's explicitly
designed not to report any error.
Commit 6b9964 enforces checking invalid use of VSH_OT_STRING with
VSH_OFLAG_REQ. This commit tries to do the same thing to stop using
VSH_OT_DATA without VSH_OFLAG_REQ and also fix existing misuse.
Signed-off-by: Hao Liu <hliu@redhat.com>
Since 0766783abb
Coverity complains that the EDIT_FREE definition results in DEADCODE.
As it turns out with the change to use the EDIT_FREE macro the call to
vir*Free() wouldn't be necessary nor would it happen...
Prior code to above commitid would :
vir*Ptr foo = NULL;
...
foo = vir*GetXMLDesc()
...
vir*Free(foo);
foo = vir*DefineXML()
...
And thus the free was needed. With the change to use EDIT_FREE the
same code changed to:
vir*Ptr foo = NULL;
vir*Ptr foo_edited = NULL;
...
foo = vir*GetXMLDesc()
...
if (foo_edited)
vir*Free(foo_edited);
foo_edited = vir*DefineXML()
...
However, foo_edited could never be set in the code path - even with
all the goto's since the only way for it to be set is if vir*DefineXML()
succeeds in which case the code to allow a retry (and thus all the goto's)
never leaves foo_edited set
All error paths lead to "cleanup:" which causes both foo and foo_edited
to call the respective vir*Free() routines if set.
Signed-off-by: John Ferlan <jferlan@redhat.com>
At a slightly larger memory expense allow stealing of items from the
string array returned from vshStringToArray and turn the result into a
string list compatible with virStringSplit. This will allow to use the
common dealloc function.
This patch also fixes a few forgotten checks of return from
vshStringToArray and one memory leak.
Noticed that the expected "not supported" error is dropped when
invoking 'virsh snapshot-list dom' on a Xen installation running
the libxl driver
virsh snapshot-list test
error: Invalid snapshot: virDomainSnapshotFree
The error is overwritten by a call to virDomainSnapshotFree
in cleanup code within cmdSnapshotList. Prevent overwritting
the real error by not calling virDomainSnapshotFree with a NULL
virDomainSnapshotPtr.
Convert the type of loop iterators named 'i', 'j', k',
'ii', 'jj', 'kk', to be 'size_t' instead of 'int' or
'unsigned int', also santizing 'ii', 'jj', 'kk' to use
the normal 'i', 'j', 'k' naming
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
These all existed before virfile.c was created, and for some reason
weren't moved.
This is mostly straightfoward, although the syntax rule prohibiting
write() had to be changed to have an exception for virfile.c instead
of virutil.c.
This movement pointed out that there is a function called
virBuildPath(), and another almost identical function called
virFileBuildPath(). They really should be a single function, which
I'll take care of as soon as I figure out what the arglist should look
like.
The source code base needs to be adapted as well. Some files
include virutil.h just for the string related functions (here,
the include is substituted to match the new file), some include
virutil.h without any need (here, the include is removed), and
some require both.
After we switched to C99 initialization, I noticed there were many
places where the specification of .flags parameter differed. After
going through many options and deciding whether to unify the
initialization to be '.flags = 0' or '.flags = VSH_OFLAG_NONE', I
realized both can be removed and it makes the code easier to go
through.
According to the man page, the memspec parameter should have the
'--memspec' option mandatory and this is as close as we can get to
that. What this change does is explained below.
man virsh:
snapshot-create-as ... [[--live] [--memspec memspec]]
virsh help snapshot-create-as before this patch:
SYNOPSIS
snapshot-create-as ... [<memspec>] ...
...
OPTIONS
[--memspec] <string> ...
virsh help snapshot-create-as after this patch:
SYNOPSIS
snapshot-create-as ... [--memspec <string>] ...
...
OPTIONS
--memspec <string> ...
Properly check the return value of vshCommandOptStringReq for xmlfile:
* error out on incorrect input (--xmlfile '')
* use default XML <domainsnapshot/> with no --xmlfile specified
(Broken by commit b2e8585)
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=919826
