Commit 0c56d94318 forgot to return false in the cmdConnect command
after the clean up made there.
Before (assuming you don't have uri alias for 'asdf'):
$ virsh connect asdf
error: failed to connect to the hypervisor
$ echo $?
0
After (with the same assumption):
$ virsh connect asdf
error: failed to connect to the hypervisor
error: no connection driver available for asdf
$ echo $?
1
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1356461
Signed-off-by: Martin Kletzander <mkletzan@redhat.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>
A new hidden command for virsh that will iterate over
all command groups and commands and print help for every single one.
This involves running vshCmddefOptParse so we can get an error if
one of the command's option structure is invalid.
The man page says: "(Re)-Connect to the hypervisor. When the shell is
first started, this is automatically run with the URI parameter
requested by the "-c" option on the command line." However, if you run:
virsh -c 'test://default' 'connect; uri'
the output will not be 'test://default'. That's because the 'connect'
command does not care about any virsh-only related settings and if it is
run without parameters, it connects with @uri == NULL. Not only that
doesn't comply to what the man page describes, but it also doesn't make
sense. It also means you aren't able to reconnect to whatever you are
connected currently.
So let's fix that in both virsh and virt-admin add a test case for it.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
virsh # list --all
Id Name State
----------------------------------------------------
1 test running
virsh # connect frob
error: Failed to connect to the hypervisor
error: no connection driver available for frob
virsh # list --all
error: failed to connect to the hypervisor
error: no valid connection
error: no connection driver available for frob
Seems sensible IMO to just not clear out the old connection state
until the new virConnectOpen succeeds.
https://bugzilla.redhat.com/show_bug.cgi?id=829160
Currently, if a connection URI was specified on the command line by the
'-c' switch, virsh connects to it, but after connecting overrides its
value with the one it tries to obtain from the VIRSH_DEFAULT_CONNECT_URI
environment variable.
This makes virsh connecting to the wrong URI if it disconnects from the
hypervisor and then tries to reconnect, and also leaks the original connname.
Fix by calling virGetEnvBlockSUID() before virshParseArgv().
https://bugzilla.redhat.com/show_bug.cgi?id=872166
When the login session doesn't have an ssh -X type display agent in
order for libvirtd to run the polkit session authentication, attempts
to run 'virsh -c qemu:///system list' from an unauthorized user (or one
that isn't part of the libvirt /etc/group) will fail with the following
error from libvirtd:
error: authentication unavailable: no polkit agent available to
authenticate action 'org.libvirt.unix.manage'
In order to handle the local authentication, we will use the new
virPolkitAgentCreate API in order to create a text based authentication
agent for our non readonly session to authenticate with.
The new code will execute in a loop allowing 5 failures to authenticate
before failing out.
With this patch in place, the following occurs:
$ virsh -c qemu:///system list
==== AUTHENTICATING FOR org.libvirt.unix.manage ===
System policy prevents management of local virtualized systems
Authenticating as: Some User (SUser)
Password:
==== AUTHENTICATION COMPLETE ===
Id Name State
----------------------------------------------------
1 somedomain running
$
The *event --loop commands would keep running even though a connection
to libvirtd is lost. This doesn't make a lot of sense since clearly we
won't get any new events from the closed connection.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Commmit df8192aa introduced admin related rename and some minor
(caused by automated approach, aka sed) and some more severe isues along with
it. First reason to revert is the inconsistency with libvirt library.
Although we deal with the daemon directly rather than with a specific
hypervisor, we still do have a connection. That being said, contributors might
get under the impression that AdmDaemonNew would spawn/start a new daemon
(since it's admin API, why not...), or AdmDaemonClose would do the exact
opposite or they might expect DaemonIsAlive report overall status of the daemon
which definitely isn't the case.
The second reason to revert this patch is renaming virt-admin client. The
client tool does not necessarily have to reflect the names of the API's it's
using in his internals. An example would be 's/vshAdmConnect/vshAdmDaemon'
where noone can be certain of what the latter function really does. The former
is quite expressive about some connection magic it performs, but the latter does
not say anything, especially when vshAdmReconnect and vshAdmDisconnect were
left untouched.
virAdmConnect was named after virConnect, but after some discussions,
most of the APIs called will be working with remote daemon and starting
them virAdmDaemon will make more sense. Only possibly controversal name
is CloseCallback (de)registration, and connecting to the daemon (which
will still be Open/Close), but even this makes sense if one thinks about
the daemon being opened and closed, e.g. as file, etc.
This way all the APIs working with the daemon will start with
virAdmDaemon prefix, they will accept virAdmDaemonPtr as first parameter
and that will better suit with other namings as well (virDomain*,
virAdmServer*, etc.).
Because in virt-admin, the connection name does not refer to a struct
that would have a connect in its name, also adjust 'connname' in
clients. And because it is not used anywhere in the vsh code, move it
from there into each client.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
After my "client rpc: Report proper error for keepalive disconnections"
patch, virsh would no long print a warning when it closes a connection
to a daemon after a keepalive timeout. Although the warning
virsh # 2015-09-15 10:59:26.729+0000: 642080: info :
libvirt version: 1.2.19
2015-09-15 10:59:26.729+0000: 642080: warning :
virKeepAliveTimerInternal:143 : No response from client
0x7efdc0a46730 after 1 keepalive messages in 2 seconds
was pretty ugly, it was still useful. This patch brings the useful part
back while making it much nicer:
virsh # error: Disconnected from qemu:///system due to keepalive timeout
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Currently, we set interactive mode as default possibly reverting the
setting after we parse the command line arguments. There's nothing
particulary wrong with that, but a call to vshReadlineInit is performed
always in the global initializer just because the default mode is interactive.
Rather than moving vshReadlineInit call somewhere else (because another client
might want to implement interactive mode only), we could make the decision
if we're about to run in interactive mode once the command line is parsed.
Commit a0b6a36f separated vshInitDebug from the original vshInit
(before virsh got split and vshInit became virshInit - commit 834c5720)
in order to be able to debug command line parsing.
After the parsing is finished, debugging is reinitialized to work properly.
There might as well be other features that require re-initialization as
the command line could specify parameters that override our defaults which
had been set prior to calling vshArgvParse.
As part of the effort to stay consistent, change the vshInit signature
from returning int to returning bool. Moreover, remove the
unnecessary error label as there is no cleanup that would make use of
it.
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.
Some virsh commands have a size parameter, which is handled as scaled
integer. We don't have any *feature* that would allow to use '-1' as
maximum size, so it's safe to reject any negative values for those
commands.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1159171
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Commit a0670ae caused a regression in 'virsh event' and
'virsh qemu-monitor-event' - if a user tries to filter the
command to a specific domain, an error message is printed:
$ virsh event dom --loop
error: internal error: virsh qemu-monitor-event: no domain VSH_OT_DATA option
and then the command continues as though no domain had been
supplied (giving events for ALL domains, instead of the
requested one). This is because the code was incorrectly
assuming that all "domain" options would be supplied via a
mandatory VSH_OT_DATA, even though "domain" is optional for
these two commands, so we had changed them to VSH_OT_STRING
to quit failing for other reasons (ever since it was decided
that VSH_OT_DATA and VSH_OT_STRING should no longer be
synonyms).
In looking at the situation, though, the code for looking up
a domain was making a pointless check for whether the option
exists prior to finding the option's string value, as
vshCommandOptStringReq does just fine at reporting any errors
when looking up a string whether or not the option was present.
So this is a case of regression fixing by pure code deletion :)
* tools/virsh-domain.c (vshCommandOptDomainBy): Drop useless filter.
* tools/virsh-interface.c (vshCommandOptInterfaceBy): Likewise.
* tools/virsh-network.c (vshCommandOptNetworkBy): Likewise.
* tools/virsh-nwfilter.c (vshCommandOptNWFilterBy): Likewise.
* tools/virsh-secret.c (vshCommandOptSecret): Likewise.
* tools/virsh.h (vshCmdHasOption): Drop unused function.
* tools/virsh.c (vshCmdHasOption): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
When editing a domain with 'virsh edit' and failing validation, the
usual message pops up:
Failed. Try again? [y,n,f,?]:
Turning off validation can be useful, mainly for testing (but other
purposes too), so this patch adds support for relaxing definition in
virsh-edit and makes 'virsh edit <domain>' more usable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
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>
Each command that needs a connection causes a new connection to be
made. Reconnecting after a command failed is pointless, mainly when
there is no other command to run. Removeing three lines of code takes
care of that and keeps virsh working as it should.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Recent commit 12bd207e21 fixed few
VSH_OT_STRING options that should've been VSH_OT_DATA. That lead me to
this commit that enforces people to check that newly added options have
proper type. Thanks to virsh erroring out with error message, this will
immediately show up in 'make check' thanks to our virsh-synopsis test.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Even though vshCmddefOptParse() tried returning -1 if there was an
optional option specification that preceded a required one, it failed to
check that for boolean type options and options with VSH_OFLAG_REQ_OPT
flag set. On the other hand, it makes sense that VSH_OT_ARGV is
specified at the end of the option list.
Returning -1 enforces the proper ordering thanks to virsh-synopsis test
in 'make check'.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
C guarantees that static variables are zero-initialized. Some older
compilers (and also gcc -fno-zero-initialized-in-bss) create larger
binaries if you explicitly zero-initialize a static variable.
* tools/virsh-console.c (got_signal): Drop unused variable.
* tools/virsh-domain.c: Fix initialization.
* tools/virsh.c: Likewise.
* tools/virt-host-validate-common.c (virHostMsgWantEscape):
Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
The parser accepts P and E, so the formatter should too.
* tools/virsh.c (vshPrettyCapacity): Handle larger units.
Signed-off-by: Eric Blake <eblake@redhat.com>
Expose the new flag just added to virDomainGetBlockJobInfo.
With --raw, the presence or absence of --bytes determines which
flag to use in the single API call. Without --raw, the use of
--bytes forces an error if the server doesn't support it,
otherwise, the code tries to silently fall back to scaling the
MiB/s value.
My goal is to eventually also support --bytes in bandwidth mode;
but that's a bit further down the road (and needs a new API flag
added in libvirt.h first).
This changes the human output, but the previous patch added
raw output precisely so that we can have flexibility with the
human output. For this commit, I used qemu-monitor-command to
force an unusual bandwidth, but the same will be possible once
qemu implements virDomainBlockCopy:
Before:
Block Copy: [100 %] Bandwidth limit: 2 MiB/s
After:
Block Copy: [100 %] Bandwidth limit: 1048577 bytes/s (1.000 MiB/s)
The cache avoids having to repeatedly checking whether the flag
works when talking to an older server, when multiple blockjob
commands are issued during a batch session and the user is
manually polling for job completion.
* tools/virsh.h (_vshControl): Add a cache.
* tools/virsh.c (cmdConnect, vshReconnect): Initialize the cache.
* tools/virsh-domain.c (opts_block_job): Add --bytes.
* tools/virsh.pod (blockjob): Document this.
Signed-off-by: Eric Blake <eblake@redhat.com>
resolves https://bugzilla.redhat.com/show_bug.cgi?id=1132305:
The error message for an out-of-range argument was confusing:
virsh -k 9999999999
error: option --k requires a positive numeric argument
After this patch, it is:
error: Invalid value for option -k
Signed-off-by: Eric Blake <eblake@redhat.com>
Let's just open the file right away and deal with errors. Moreover,
there's no reason to forbid logging to, e.g., a pipe.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
To follow the new semantics of the vshCommandOptToU* functions convert
this one to reject negative numbers too. To allow using -1 for "maximum"
semantics for the vol-*load two bandwidth functions that use this helper
introduce vshCommandOptULongLongWrap.
To follow the new semantics of the vshCommandOptToU* functions convert
this one to reject negative numbers too. To allow using -1 for "maximum"
semantics for the two bandwidth functions that use this helper introduce
vshCommandOptULWrap. Although currently the migrate-setspeed function
for the qemu driver will reject -1 as maximum.
Use virStrToLong_uip instead of virStrToLong_ui to reject negative
numbers in the helper. None of the callers expects the wraparound
"feature" for negative numbers.
Also add a function that allows wrapping of negative numbers as it might
be used in the future and be explicit about the new semantics in the
function docs.
