Thing about vshReadlineInit() is - it's called multiple times.
The first time from vshInit(), when @ctl was filled only
partially (most notably, before any argv parsing is done, hence
ctl->imode is set to false). The second time after argv parsing,
from virshInit() -> vshInitReload(). In here, ctl->imode might
have changed and thus vshReadlineInit() can't exit early - it
needs to set up stuff for interactive mode (history basically).
To allow vshReadlineInit() to be called again,
vshReadlineDeinit() must set @autoCompleteOpaque to NULL.
Fixes: cab1e71f01
Resolves: https://issues.redhat.com/browse/RHEL-53560
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
While __attribute((sentinel)) (exposed by glib under
G_GNUC_NULL_TERMINATED macro) is a gcc extension, it's supported
by clang too. It's already being used throughout our code but
some functions that take variadic arguments and expect NULL at
the end were lacking such annotation. Fill them in.
After this, there are still some functions left untouched because
they expect a different sentinel than NULL. Unfortunately, glib
does not provide macro for different sentinels. We may come up
with our own, but let's save that for future work.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This can happen only for cmdComplete() in interactive mode (which
I'm still not convinced is any useful for users and whether we
should support it). Anyway, running plain 'complete' command with
no additional arguments boils down to @text being NULL in
vshReadlineParse() which handles the case just right but is then
subsequently passed to vshCompleterFilter() which isn't prepared
for this case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Problem with readline is its API. It's basically a bunch of
global variables with no clear dependencies between them. In this
specific case that I'm seeing: in interactive mode the
cmdComplete() causes instant crash of virsh/virt-admin:
==27999== Invalid write of size 1
==27999== at 0x516EF71: _rl_init_line_state (readline.c:742)
==27999== by 0x5170054: rl_initialize (readline.c:1192)
==27999== by 0x516E5E4: readline (readline.c:379)
==27999== by 0x1B7024: vshReadline (vsh.c:3048)
==27999== by 0x140DCF: main (virsh.c:905)
==27999== Address 0x0 is not stack'd, malloc'd or (recently) free'd
This is because readline keeps a copy of pointer to
rl_line_buffer and the moment cmdComplete() returns and readline
takes over, it accesses the copy which is now a dangling pointer.
To fix this, just keep the original state of rl_line_buffer and
restore it.
Fixes: 41400ac1dd
Fixes: a0e1ada63c
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Our completer callbacks must refrain from printing anything onto
stderr, but unfortunately that's not how service code around
behaves. It may call vshError() and what not. Rather trying to
fix all possible paths (just consider opening a connection), just
close the stderr. We're already closing stdin.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The refactor of the libvirt tools command parser introduced a bug where
the '--help' option would cause an error:
$ virsh list --help
error: command 'list' doesn't support option --help
rather than printing the help for the command as the help option is
supposed to be handled separately from the real options.
Re-introduce the separate handling to the new parser code.
Fixes: 5540c3d241
Resolves: https://issues.redhat.com/browse/RHEL-36565
Reported-by: Lili Zhu <lizhu@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Recent rework of virshtest uncovered a subtle bug that was
dormant in now vsh but before that even in monolithic virsh.
In vsh.c there's this vshReadlineInit() function that's supposed
to initialize readline library, i.e. set those global rl_*
pointers. But it also initializes history library. Then, when
virsh/virt-admin quits, vshReadlineDeinit() is called which
writes history into a file (ensuring the parent directory
exists). So far no problem.
Problem arises when cmdComplete() is called (from a bash
completer, for instance). It does not guard call to
vshReadlineInit() with check for interactive shell (and it should
not), but it sets ctl->historyfile which signals to
vshReadlineDeinit() the history should be written.
Now, no real history is written, because nothing was entered on
the stdin, but the parent directory is created nevertheless. With
recent movement in virshtest.c this means some test cases might
create virsh history file which breaks our promise of not
touching user's data in test suite.
Resolves: https://bugs.gentoo.org/931109
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Refactor the existing logic using two nested loops with a jump into the
middle of both with 3 separate places fetching next token to a single
loop using a state machine with one centralized place to fetch next
tokens and add explanation comments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As we now have a centralized point to assign values to options move the
debugging logic there.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This check was needed due to the use "unsigned long long" as bitmap
which was refactored recently.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Refactor the very old opaque logic (using multiple bitmaps) by
fully-allocating vshCmdOpt for each possible argument and then filling
them as they go rather than allocating them each time after it's parsed.
This simplifies the checkers and removes the need to cross-reference
multiple arrays.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Neither of them is used outside of vsh.c. 'vshCmddefSearch' needed to be
rearranged as it was called earlier in vsh.c than it was defined.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Remove the old helpers which were used previously to pick which field to
complete.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In preparation for internal parser refactor introduce new accessors for
the VSH_OT_ARGV type which will return a NULL-terminated string list or
even a concatenated string for the given argument.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Currently the code decides which option to complete by looking into the
input string and trying to infer it based on whether we are at the
end position as we truncate the string to complete to the current cursor
position.
That basically means that only the last-parsed option will be up for
completion.
Replace the logic by remembering which is the last option rather than
using two different position checks and base the completion decision on
that and the actual value of the last argument (see comment).
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The argument will be used for testing the command/option completer
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
While the 'complete' command is meant to be hidden and used only for
the completion script, there's nothing preventing it being used in all
virsh modes.
This poses a problem as the command tries to close 'stdin' to avoid the
possibility that an auth callback would want to read the password.
In interactive mode this immediately terminates virsh and in
non-interactive mode it attempts to close it multiple times if you use
virsh in batch mode.
Fix the issues by using virOnce() to close it exactly once and do so
only in non-interactive mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Shorten the function name as there isn't any vshCommandOptString.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
'vshReadlineInit' is called when interactive virsh is started but also
on each call to 'cmdComplete'. Calling it repeatedly (using the
'complete' command interactively, or multiple times in batch mode) leaks
the buffers for history file configuration.
Avoid multiple setups of this function by returning success in case the
history file config is already present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The buffer which we assign to the 'rl_line_buffer' variable of readline
would be overwritten and thus leaked on multiple invocations of
cmdComplete in one session.
Free/clear it after it's used.
Hitting this leak was until recenly possible only in non-interactive
batch mode and recently also in interactive mode as 'complete' can be
used multiple times now interactively.
Fixes: a0e1ada63c
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
For testing purposes it will come handy to change the directory from a
batch-mode script. Remove the check forbidding use of the 'cd' command
in batch mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Drop the last enum member VSH_OFLAG_NONE and remove the 'flags' variable
from vshCmdOptDef.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Switch the command parser from using the VSH_OFLAG_REQ_OPT flag
opting out from positional parsing of arguments to a combination of the
'positional' flags for truly positional arguments and
'unwanted_positional' preserving semantics for the existing arguments
where the parser did it due to bad design.
This patch retires VSH_OFLAG_REQ_OPT along with the infrastructure that
was needed to refactor all uses properly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The intended use of those commands is to use the argument directly
without the flag. Since the argument is optional in all cases we
couldn't declare them as positional until now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We already allow a optional positional _ARGV argument but there's no
reason why any other argument type could not be allowed this way.
Add checks that there's just one such argument and it's placed after
required positional arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While previous fixes kept the help output unchanged as base for the
refactors it turns out that the formatting of help for argv options is
wrong.
Specifically in SYNOPSIS the non-positional _ARGV would have the option
name in square brackets (which in other cases means that given thing is
optional) despite being required.
Similarly in the DESCRIPTION section positional versions would not show
the optional argument name and also didn't use the three dots to signal
that it can be used multiple times.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Based on the rationale in previous commit, all commands which were
parsed as positional but not documented as such will be annotated with
this flag.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While the virsh option definitions specify (either explicitly after
recent refactors, or implicitly before) whether an argument is
positional or not, the actual parser is way more lax and actually and
allows also arguments which were considered/documented as non-positional
to be filled positionally unless VSH_OFLAG_REQ_OPT is used in the flags.
This creates situations such as 'snapshot-create-as' which has the
following docs:
SYNOPSIS
snapshot-create-as <domain> [--name <string>] [--description <string>]
[--print-xml] [--no-metadata] [--halt] [--disk-only]
[--reuse-external] [--quiesce] [--atomic] [--live] [--validate]
[--memspec <string>] [[--diskspec] <string>]...
Thus showing as if '--name' and '--description' required the option, but
in fact the following happens when only positionals are passed:
$ virsh snapshot-create-as --print-xml 1 2 3 4 5
<domainsnapshot>
<name>2</name>
<description>3</description>
<disks>
<disk name='4'/>
<disk name='5'/>
</disks>
</domainsnapshot>
In the above example e.g. '--memspec' is not populated.
This disconnect makes it impossible to refactor the parser itself and
allows users to write buggy interactions with virsh.
In order to address this we'll be annotating every single of these
unwanted positional options as such so that this doesn't happen in the
future, while still preserving the quirk in the parser.
This patch introduces a tool which outputs list of options which are not
marked as positional but are lacking the VSH_OFLAG_REQ_OPT flag.
This tool will be removed once all the offenders found by it will be
addressed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
- move the check that completer_flags are 0 if no completer is set
into a common place and remove duplication
- add check that _BOOL arguments are not positional
- add missing checks to _ALIAS
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All virsh commands in non-quiet mode append another separator line thus
having two is unnecessary and in quiet mode it still has a trailing
blank line. Remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The flag was replaced by the 'required' field in the option definition.
Remove last few uses and all assignments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the new 'positional' field to do decisions rather than have a
special type for positional strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the new flags to do the decisions which will also fix the case when
an _INT option is required but non-positional.
This fixes the help for the 'timeout' argument of 'daemon-timeout'
virt-admin command:
SYNOPSIS
- daemon-timeout <timeout>
+ daemon-timeout --timeout <number>
[...]
OPTIONS
- [--timeout] <number> number of seconds the daemon will run without any active connection
+ --timeout <number> number of seconds the daemon will run without any active connection
Resolves: https://issues.redhat.com/browse/RHEL-25993
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that the code was refactored and proved identical, remove the checks
so that they don't impede further refactors.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is logically enforced by existing checks, thus we can formalize it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In at least one case we've wanted a mandatory argument which requires
the explicit flag. Fix the assumption before converting everything over
to the new flags.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add 'positional' and 'required' fields to vshCmdOptDef, which will
explicitly track the two properties of arguments.
To ensure that we have proper coverage, add checks to
vshCmddefCheckInternals validating the state of the above flags by
infering it from existing data.
This conversion will allow us:
- remove VSH_OT_DATA in favor of VSH_OT_STRING
- use VSH_OT_INT when required both as positional and non-positional
- properly annotate which VSH_OT_ARGV are positional and which are not
(currently inferred by whether an previous positional option exists)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Declare one argument per line, separate disticnt conditions by newline,
move some checks earlier.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Extract flag check to a separate variable and replace ternary operators
by normal conditions and use allocated buffer instead of a static one
to improve readability.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Extract flag check to a separate variable and replace ternary operators
by normal conditions and directly output the text rather than using
extra variable to improve readability.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Store the pointers to 'help' and 'description' information in the struct
directly rather than in a key-value list.
The generic approach never got any extra use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The new option dumps the full help outputs for every command so that
it's possible to conveniently check that subsequent refactors will not
impact any of the external functionality.
No man page entry is needed as the command is internal/undocumented.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use a switch statement to cover all cases and check for missing
completers for arguments declared as VSH_OT_ARGV.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's obvious that a command is an alias when the 'alias' property is
set, thus an extra flag is redundant. Remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add a check that the default 0 assignment will not mean that an option
is considered to be VSH_OT_BOOL.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The command invoking the code is internal and meant for developers,
there's no point in translating the errors.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
None of the clients use the 'command set' approach and other pieces of
code such as the command validator already assume that command groups
are in use. Remove the unused 'command set' stuff.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove some code repetition between desc and net-desc commands.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
