Right now, we display the message before actually attempting
to connect to the VM console. That operation, however, can
fail for a number of reasons: for example, is the VM doesn't
have a serial device, the output ends up looking like
$ virsh console cirros
Connected to domain 'cirros'
Escape character is ^] (Ctrl + ])
error: internal error: cannot find character device <null>
The initial message is misleading. Change things so that it's
only printed if we actually successfully connected to the VM
console.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The self-test command for both virsh and virt-admin is self contained
and directly reports success, thus we don't actually need to run a shell
wrapper around it.
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>
Until now when '--name' was used the parent was not printed and the
option was ignored. One option would be to declare the options mutually
exclusive, but for testing it may come handy to print both the snapshot
name and parent. Adjust the code to print them tab-separated and adjust
the docs.
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>
Historically the command parser in virsh parses/fills even optional
arguments with values as if they were positional unless opted out using
VSH_OFLAG_REQ_OPT. This creates unexpected situations when commands can
break in this unwanted semantics:
$ virsh snapshot-create-as --print-xml 1 2 3
<domainsnapshot>
<name>2</name>
<description>3</description>
</domainsnapshot>
To prevent any further addition annotate the rest of the arguments with
the 'unwanted_positional' flag, so that the parser can keep parsing them
as such but any further optional argument will not have this behaviour.
Certain arguments where it makes sense are annotated as 'positional' too
in this patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Historically the command parser in virsh parses/fills even optional
arguments with values as if they were positional unless opted out using
VSH_OFLAG_REQ_OPT. This creates unexpected situations when commands can
break in this unwanted semantics:
$ virsh snapshot-create-as --print-xml 1 2 3
<domainsnapshot>
<name>2</name>
<description>3</description>
</domainsnapshot>
To prevent any further addition annotate the rest of the arguments with
the 'unwanted_positional' flag, so that the parser can keep parsing them
as such but any further optional argument will not have this behaviour.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Annotate arguments which can be unintentionally parsed positionally.
(See previous commits for explanation.)
All of these options were added in order thus we must declare all of
them as 'unwanted_positional'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Annotate arguments which can be unintentionally parsed positionally.
(See previous commits for explanation.)
The pool name is optional but in all cases it can be promoted to an
optional positional argument so that it can be properly aligned with the
expectations of the parser.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Make certain optional arguments truly positional in cases when it makes
semantic sense.
Previously it wasn't possible to have optional positional arguments, but
the parser filled them regardless, thus this preserves functionality.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Mark the 'backupxml' as positional optional and the 'checkpointxml' as
'unwanted_positional' to preserve the positional parsing quirk.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The argument is optional thus couldn't be marked as positional until now,
despite being parsed positionally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The 'snapshotname' argument is optional as by default "current" snapshot
is considered. Regardless of that we should treat it as positional as
it's the common usage. This is now possible as we can have one optional
positional argument.
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>
The argument was being parsed positionally due to the command parser
quirk as we didn't opt out of it.
Since the code in virshLookupCheckpoint requires that the checkpointname
is present we can mark all the options as positional and required and
remove the redundant check from virshLookupCheckpoint.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Annotate arguments which can be unintentionally parsed positionally.
(See previous commits for explanation.)
Currently virsh accepts the arguments such as:
$ virsh attach-disk --print-xml 1 2 3 4 5 6 7 8 9 10
<disk type='file' device='10'>
<driver name='5' type='6' iothread='7' cache='8' io='9'/>
<source file='2'/>
<target dev='3' bus='4'/>
</disk>
While making virsh require the flags is technically a breaking change,
there were multiple instances where arguments were added to the argument
list thus changing the order the positional arguments would be
interpreted as. Examples are commits: 7e157858b4, bc5a8090af,
ca21d75d25. As of such there are multiple breaks of compatibility for
the positional arguments.
As of such, require the option flag for all optional arguments with
value for 'virsh attach-disk'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Annotate arguments which can be unintentionally parsed positionally.
(See previous commits for explanation.)
Annotate '--migrateuri', '--graphicsuri', '--listen-address', '-dname',
'--timeout', '--xml', '--migrate-disks' and '--disks port' as
'unwanted_positional'. These were declared in chronological order per
git history.
All others are annotated with VSH_OFLAG_REQ_OPT which makes the parser
require the '--optionname'. This is due to the fact that '--disks-uri'
was introduced later and put in front of others declared earlier
breaking the order they would be accepted, thus changing the behaviour
between versions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Make all of the tunable parameter flags require the option name (don't
parse them positionally).
While techically this would be a breaking change if anyone were to
specify the tunable values positionally this is not the case as the
first two tunables are not compatible with each other:
$ virsh blkdeviotune cd vda 4 5
error: Unable to change block I/O throttle
error: invalid argument: total and read/write of bytes_sec cannot be set at the same time
The above is produced by all implementations of the API (qemu and test
drivers). It is true that the first tunable can be specified
positionally (--total-bytes-sec) but it is misleading and shoud not be
allowed either.
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>
In most cases it's the usual/recommended way to use those commands:
$ virsh qemu-monitor-command VMNAME cmd args args args
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Our documentation in most places explicitly mentions --diskspec and it
was never meant to be positional, although we can't change the parser
any more. Annotate them as 'unwanted_positional'.
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>
The macro is used in just one place and the definition of the option is
going to be modified. Inline the macro.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The macro is used in one place only and the command definition will be
altered. Inline it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Upcoming patches will need to tweak some of the properties of the
command. Since the macro is used in just two places expand it inline.
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>
As this command was introduced in this release add the flag requiring to
pass optionname.
This is needed to actually disallow positional parsing of the value
despite documenting that the flag name is required.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
vshAdmCatchDisconnect requires non-NULL structure vshControl for
getting connection name (stored at opaque), but
virAdmConnectRegisterCloseCallback at vshAdmConnect called it
with NULL.
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-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>
There's just one command taking a list of domains as argument, thus
declare it inline.
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>
Some description of virsh commands referenced itself in a multi-line
example of usage, which is pointless as virsh help already shows how to
use the command:
.data = N_("Get or set the current memory parameters for a guest"
" domain.\n"
" To get the memory parameters use following command: \n\n"
" virsh # memtune <domain>")
Change it to just state what the command does and leave the example for
the help printer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
