Currently if cmd->skipChecks is set (done only from completers)
some basic checks are skipped because we're working over
partially parsed command. See a26ff63ae4 for more detailed
explanation. Anyway, the referenced commit was too aggressive in
disabling checks and effectively returned success even in clear
case of failure. For instance:
# domif-getlink --interface <TAB><TAB>
causes virshDomainInterfaceCompleter() to be called, which calls
virshDomainGetXML() which eventually calls
vshCommandOptStringReq(.., name = "domain"); The --domain
argument is required for the command and if not present -1 should
be returned to tell the caller the argument was not found. Well,
zero is returned meaning the argument was not found but it's not
required either.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Unfortunately, we have a number of aliases in virsh and even though
these are not visible any more, we have to support them. The problem is
that when trying to print help for the alias, we get SIGSEGV because
there isn't any @def structure anymore and we need to query the command
being aliased instead.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1538570
Signed-off-by: Erik Skultety <eskultet@redhat.com>
These helpers are called from a single place only - cmdHelp wrapper and
just before the wrapper invokes the helpers, it performs the search,
either for command group or for the command itself, except the result is
discarded and the helper therefore needs to do it again. Drop this
inefficient handling and pass the @def structure rather than a name,
thus preventing the helper from needing to perform the search again.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
When building without readline, this function does nothing but
return false. Without touching any of its arguments which
triggers a build error. Therefore, provide a stub that has
arguments marked as unused.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The current state of art is as follows:
1) vshReadlineOptionsGenerator() generate all possible --options
for given command, and then
2) vshReadlineOptionsPrune() clears out already provided ones
from the list.
Not only this brings needless memory complexity it is also not
trivial to get right. We can switch to easier approach: just
don't add already specified --options in the first step.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This command is going to be called from bash completion script in
the following form:
virsh complete -- start --domain
Its only purpose is to return list of possible strings for
completion. Note that this is a 'hidden', unlisted command and
therefore there's no documentation to it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Similarly to the previous commit, once we've presented an
--option for a command to the user it makes no sense to offer it
again. Therefore, we can prune all already specified options. For
instance, after this patch:
virsh # migrate --verbose <TAB><TAB>
will no longer offer --verbose option.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Instead of having completers prune returned string list based on
user's input we can do that right after the callback is called.
Only strings matching the prefix will be presented to the user
then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Now that we have everything prepared we can call options'
completer again. At the same time, pass partially parsed input to
the completer callback - it will help the callbacks to narrow
down the list of returned options based on user's input. For
instance, if the completer is supposed to return list of
interfaces depending on user input it may return just those
interfaces defined for already specified domain. Of course,
completers might ignore this parameter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In the future, completer callbacks will receive partially parsed
command (and thus possibly incomplete). However, we still want
them to use command options fetching APIs we already have (e.g.
vshCommandOpt*()) and at the same time don't report any errors
(nor call any asserts).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
It's better to fetch list of either commands or options just once
and then iterate over it. Moreover, it makes future completers
way simpler as they will return string lists too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When returning a string that needs escaping there are two
scenarios that can happen. Firstly, user already started the
string with a quote (or double quote) in which case we don't need
to do anything - readline takes care of that. However, if they
haven't typed anything yet, we need to escape the string
ourselves.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Now that we have a way of retrieving partly parsed command we
don't need duplicate code that parses the user's input.
Yes, this code removes call of opt's completer, but:
a) current implementation is broken anyway, and
b) it will be added back shortly
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In the future, this function is going to be called from
vshReadlineParse() to provide parsed input for completer
callbacks. The idea is to allow the callbacks to provide more
specific data. For instance, for the following input:
virsh # domifaddr --domain fedora --interface <TAB><TAB>
the --interface completer callback is going to be called. Now, it
is more user friendly if the completer offers only those
interfaces found in 'fedora' domain. But in order to do that it
needs to be able to retrieve partially parsed result.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When parsing cmd line which has "--" on it, this is leaked.
Problem is, parser->getNextArg() allocates new string and stores
it into tkdata. But as soon as "--" is detected 'continue' is
issued without any free of the allocated memory.
==5304== 3 bytes in 1 blocks are definitely lost in loss record 1 of 782
==5304== at 0x4C2AF50: malloc (vg_replace_malloc.c:299)
==5304== by 0x8BB5AA9: strdup (strdup.c:42)
==5304== by 0x55842CA: virStrdup (virstring.c:941)
==5304== by 0x172B21: _vshStrdup (vsh.c:162)
==5304== by 0x175E8E: vshCommandArgvGetArg (vsh.c:1622)
==5304== by 0x17551D: vshCommandParse (vsh.c:1418)
==5304== by 0x175F25: vshCommandArgvParse (vsh.c:1638)
==5304== by 0x130940: virshParseArgv (virsh.c:820)
==5304== by 0x130C49: main (virsh.c:922)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
My latest commit of a785186446 uncovered a problem we fixed
in 9eb23fe2 but then reverted in 834c5720e4. Turns out, some
systems (I'm looking at you OS X) have ancient readline with
broken header file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There are couple of limitations when it comes to option types and
flags for the options. For instance, VSH_OT_STRING cannot have
VSH_OFLAG_REQ set (commit c7543a728). For some reason this is
checked in vshCmddefHelp() but not in vshCmddefCheckInternals().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Avoid the annoying issue where the public object freeing APIs overwrite
the error set by helper functions, since they don't invoke the callback.
The new helper remembers the error only if no previous error was set.
Some arguments in vshErrorHandler, vshReadlineCompletion and
cmdSelfTest functions are not used. Mark them as such.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
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>
There were a few places in our virsh* code where instead of calling vshError
on failure we called vshPrint.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
First, since commit 834c5720 the error reporting within the vshErrorHandler
doesn't work because there was a lot of renaming going on (dull mechanical
renaming without much thinking about it, yep - shame on me) and so the original
env variable VIRSH_DEBUG got renamed to VSH_DEBUG which we don't support nor
document anywhere. Second, by specifying this env variable, the last libvirt
error gets reported twice despite the fact we say the error reporting should be
deferred until the command finishes, and last but not least the vintage code's
logic is a bit 'odd', since the error would get reported iff the env variable
is set, even if the value should be equal to our DEFAULT value in which case it
doesn't make sense that we behave differently when an env variable is set to
some value and when there's no env variable at all but we use the same value
automatically as default.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Unlike the other error messages in vshInitDebug, this one relied on a hardcoded
name of a variable instead of using the prefix of the tool calling the init
routine.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1393854
Signed-off-by: Erik Skultety <eskultet@redhat.com>
After 06a7b1ff4 the @&opts_need_arg is not used anywhere. Well,
it is set but never read:
vsh.c: In function 'vshReadlineParse':
vsh.c:2658:14: warning: variable 'opts_need_arg' set but not used [-Wunused-but-set-variable]
uint64_t opts_need_arg, opts_seen;
^~~~~~~~~~~~~
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Commit id 'dcfdf341' passes 'opts_need_arg' and 'opts_seen' to
vshCmddefGetData, but that seems to be incorrect as those values
are not initialized properly (something at least one compiler found).
Instead the static 'const_opts_need_arg' and 'const_opts_seen' values
should be passed.
By passing unitialized values leads to not finding possible options
for simpler commands (domfsfreeze for example), where if you're in
a virsh shell using command line completion - you'll get a list of
files in your current directory instead of two options --domain and
--mountpoint (as would happen with this patch applied.
1. Move the declaration of const vshCmdDef *help - it should be at the
top of the "if" rather than in the middle.
2. Change a comparison from && to || - without doing so we could crash
on commands like 'virsh list' which would allow completion of some
non -- option based on whatever was found in the current working
directory and then as soon as that was completed, the next <tab>
would crash since "opt" would be returned as NULL, but the check
was dereferencing "&& opt->type"
3. Before dereferencing opt->completer, be sure opt isn't NULL.
GCC complained that
vsh.c: In function 'vshReadlineOptionsGenerator':
vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable]
const vshCmdOptDef *opt = &cmd->opts[list_index];
^
vsh.c: In function 'vshReadlineParse':
vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function
[-Wmaybe-uninitialized]
completed_list = opt->completer(autoCompleteOpaque,
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1379895
Introduced by commit id '834c5720'.
During the code motion and creation of vsh.c, the function 'vshDeinit()'
in the (new) vsh.c was altered from whence it came in virsh.c such that
calling 'vshReadlineDeinit(ctl)' was conditional on "ctl->imode".
This causes a problem for the interactive running if the "quit" and "exit"
commands are used because 'cmdQuit' will clear ctl->imode, thus when the
interactive loop in main() of virsh.c exits because ctl->mode is clear and
virshDeinit is called which calls vshDeinit, the history file is now not
written. Conversely, if one had exited the interactive loop via pressing
<ctrl>D the file would be created because loop control is broken on EOF
and ctl->imode is not set to false.
This patch will remove the conditional call to vshReadlineDeinit and
restore the former behaviour.
Change the logic in a way, so that VSH_CMD_FLAG_ALIAS behaves similarly to
how VSH_OT_ALIAS for command options, i.e. there is no need for code duplication
for the alias and the aliased command structures. Along with that change,
switch any existing VSH_CMD_FLAG_ALIAS occurrences to this new format. Also,
since this patch introduces a new command structure element, adjust the
virsh-self-test test to make sure we won't ever miss to specify the '.alias'
member for an aliased command because doing that would lead to an internal
error.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Recent changes extracted the command internals validation routine from
vshCmddefOptParse method which now just calls vshCmddefOptFill. Therefore, make
vshCmddefOptFill the new vshCmddefOptParse and drop the unnecessary name.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Originally introduced by commit 2432521e which correctly split
vshCmddefOptParse into command's options validation and options parsing.
However, command's 'internals' are not tied solely to .options, rather it
should be about the overall structure, therefore the validation should be
extracted from vshCmddefOptParse and performed only within our test suite, i.e.
in vshSelfTest.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
If the initial check is true the function immediately returns so there's no
need to enclose the code following the check within an 'else' block.
Also, by removing the 'else' block, the declarations need to be moved to
beginning of the function block to conform with our guidelines.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Since it's used on a single place only, it can easily be replaced by the right
side of the original assignment.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
The intention is to move vshCmddefCheckInternals out of vshCmddefOptParse to
our test suite. First step to do that is to enforce checking for an existing
help string (that also means it's non-empty) in a command because a command
without a help is not much of a use.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
This command should be exposed to other shells of ours.
They are gonna need it as soon as we want to test them too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This patch changes the signature of vshCompleters, allowing to pass along
some data that we might want to along with the completers; for example,
we might want to pass the autocomplete vshControl along with the
completer, in case the completer requires a connection to libvirtd.
Signed-off-by: Nishith Shah <nishithshah.2211@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Call option completers if argument completion is requested using the
corresponding option completer, if it is defined.
Signed-off-by: Nishith Shah <nishithshah.2211@gmail.com>
Since commit 834c5720 which extracted the generic functionality out of virsh
and made it available for other clients like virt-admin to make use of it, it
also introduced a bug when it renamed the original VIRSH_ environment variables
to VSH_ variables. Virt-admin of course suffers from the same bug, so this
patch modifies the generic module vsh.c to construct the correct name for
environment variables of each client from information it has.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1357363
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Well, the reason behind this change is that if the function is extended in some
way that e.g. would involve allocation we do not have a way of telling it to
the caller. More specifically, vshInitDebug only relies on some hardcoded
environment variables (by a mistake) that aren't documented anywhere so neither
virsh's nor virt-admin's documented environment variables take effect. One
possible solution would be duplicate the code for each CLI client or leave the
method be generic and provide means that it could figure out, which client
called it, thus initializing the proper environment variables but that could
involve operations that might as well fail in certain circumstances and the
caller should know that an error occurred.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
My compiler identified some variables that were set, but never
actually used. For instance, opts_required, and data_acomplete.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This function tries to look up desired option for a given parsed
command. Upon successful return it also stores option position
into passed *opt_index. Now, this variable is type of int, even
though it is never ever used to store negative value. Moreover,
the variable is set from a local variable which is type of
size_t.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The 'res' variable was only being initialized to NULL in the
if (!state) path; however, that path never used res and evenutally
res is assigned one of two results based on a pair of if then else if
conditions. If for some reason neither of those paths was taken and
the (!state) path wasn't taken, then 'res' would be indeterminate.
Found by Coverity, probably a false positive based on code paths, but
better safe than sorry for the future.
Signed-off-by: John Ferlan <jferlan@redhat.com>
The new function works as expected, and matches the current level of
autocomplete offered, along with several other improvements like quotes
handling, multiple command completion and space handling. Now, it is easy
to introduce options completer here.
Signed-off-by: Nishith Shah <nishithshah.2211@gmail.com>
A bool 'report' has been introduced in various functions, which when set
to true will produce the error it is suppposed to produce, and when
false, will suppress the error. These functions are used in the next
patch for auto-completion.
Signed-off-by: Nishith Shah <nishithshah.2211@gmail.com>
