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>
XML metadata for snapshot contains only single list of disk overlays
from the moment when the snapshot was taken. When user creates multiple
branches of snapshots the parent snapshot will still list only the
original disk overlays. This may cause an issue in a specific scenario:
s1
|
+- s2
+- s3 (active)
For this snapshot topology when we delete s2 metadata for s1 are not
updated. Now when we delete s1 the code operated with incorrect
overlays from s1 metadata in order to update s3 metadata resulting in no
changes to s3 metadata.
Now when user tries to delete s3 it fails with following error:
error: Failed to delete snapshot s3
error: operation failed: snapshot VM disk source and parent disk source are not the same
For the actual deletion there is a code to figure out the correct disk
source but it was not used to update metadata as well. Due to reasons
how block commit in libvirt works we need to create a copy of that disk
source in order to have it available when updating metadata as the
original source will be freed at that point.
Resolves: https://issues.redhat.com/browse/RHEL-26276
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Calling this function when deleting internal snapshot isn't required
because with internal snapshots all changes are done within the file
itself so there is no file deletion and no need to update snapshot
metadata.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The PCI VPD (Vital Product Data) may be missing or the kernel can report
presence but not actually have the data. Also the data is specified by
the device vendor and thus may be invalid in some cases.
To avoid log spamming, since the only usage in the node device driver is
ignoring errors, remove all error reporting.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/607
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The VPD parsing is fragile and depends on hardware vendor's adherance to
standards. Since libvirt only ever uses this data to report it in the
nodedev XML which ignores any errors there's no much point in having
error reporting which I've added recently.
Turn the errors into VIR_DEBUG statements in preparation for upcoming
patch which completely removes the expectation to report errors.
This effectively reverts commits dfc85658bd and f85a382a0e.
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>
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.
Translation: libvirt/libvirt
Translate-URL: https://translate.fedoraproject.org/projects/libvirt/libvirt/
Co-authored-by: Weblate <noreply@weblate.org>
Signed-off-by: Fedora Weblate Translation <i18n@lists.fedoraproject.org>
If the original code detected a missing or null boot index in the
new XML, it automatically added the current value. This
autocompletion was incorrect because it was impossible to
distinguish between user intent and user error - changing the
boot order itself is forbidden and should always be an error.
Resolves: https://issues.redhat.com/browse/RHEL-23416
Fixes: aa3e07caec
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Change the log level for pauses of guests due to watchdog timeouts
or io errors from debug to warn to enhance the visibility of such
events.
Signed-off-by: Lennart Fricke <lennart.fricke@drehpunkt.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add domaincapstest qemuxml2argvtest qemuxml2xmltest
related test cases for loongarch.
Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Implement method for loongarch to get host info, such as
cpu frequency, system info, etc.
Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Implement support for loongarch64 in the QEMU driver.
Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Add loongarch cpu support, Define new cpu type 'loongarch64'
and implement it's driver functions.
Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
While the C API entry points will validate non-negative lengths
for various parameters, the RPC server de-serialization code
will need to allocate memory for arrays before entering the C
API. These allocations will thus happen before the non-negative
length check is performed.
Passing a negative length to the g_new0 function will usually
result in a crash due to the negative length being treated as
a huge positive number.
This was found and diagnosed by ALT Linux Team with AFLplusplus.
CVE-2024-2494
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Found-by: Alexandr Shashkin <dutyrok@altlinux.org>
Co-developed-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Current entries should always be listed before obsolete ones.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
The single caller for each function passes the same value
for @src and @parent, which means that we don't really need
the additional API.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
