The qemuBuildMachineCommandLine() function is needlessly long.
Separate out parts that generate arguments based on
domainDef->features[] into
qemuAppendDomainFeaturesMachineParam(). Unfortunately, expected
outputs for some qemuxml2argvdata cases needed to be updated
because the order in which features are generated is changed. But
there's no functional change.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Almost all of memory models we currently support allow setting
virDomainMemoryDef::targetNode so that the memory module is
associated with given guest NUMA node. And we do have a check
whether the requested node is within bounds, but it's executed
only when building QEMU's cmd line. Move it into validation
phase.
While this commit is moving the validation to a place that does
not validate all the possible code paths, it's okay, because only
the explicit memory device has user-configurable target node
which could break the assumption.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous commit, when memory-hotplug-dimm-addr.xml file was
fixed, we can also introduce the test case to qemuxml2xmltest.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, we are testing memory-hotplug-dimm-addr against a set of
explicitly listed capabilities. While this works, lets switch it
to DO_TEST_CAPS_LATEST() so that the latest capabilities are
used. This in turn means, we have to update the <emulator/>
because the latest capabilities don't contain caps for
qemu-system-i386.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Currently, virJSONValueObjectHasKey() can return one of three
values:
-1 if passed object type is not VIR_JSON_TYPE_OBJECT,
0 if the key is not present, and finally
1 if the key is present.
But, neither of callers is interested in the -1 case. In fact,
some callers call this function treating -1 and 1 cases the same.
Therefore, make the function return just true/false and fix few
callers that explicitly checked for == 1 case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In my recent comnmit v8.5.0-188-gc47f1abb81 I accidentally moved
qemuMigrationParamsResetTLS after qemuDomainObjEnterMonitorAsync not
noticing qemuMigrationParamsResetTLS will try to enter the monitor
again. The second call will time out and return with a domain object
locked. But we're still in monitor section and the object should be
unlocked which means qemuDomainObjExitMonitor will deadlock trying to
lock it again.
Fixes: c47f1abb81
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This call to qemuMigrationSrcIsAllowedHostdev() (which does a
hardcoded fail of the migration if there is any PCI or mdev hostdev
device in the domain) while doing the destination side of migration
prep was found once the call to that same function was removed from
the source side migration prep (commit 25883cd5).
According to jdenemar, for the V2 migration protocol, prep of the
destination is the first step, so this *was* the proper place to do
the check, but for V3 migration this is in a way redundant (since we
will have already done the check on the source side (updated by
25883cd5 to query QEMU rather than do a hardcoded fail)).
Of course it's possible that the source could support migration of a
particular VFIO device, but the destination doesn't. But the current
check on the destination side is worthless even in that case, since it
is just *always* failing rather than querying QEMU; and QEMU can't be
queried at the point where the destination check is happening, since
it isn't yet running.
Anyway QEMU should complain when it's started if it's going to fail,
so removing this check should just move the failure to happen a bit
later. So the best solution to this problem is to simply remove the
hardcoded check/fail from qemuMigrationDstPrepareFresh() and rely on
QEMU to fail if it needs to.
Fixes: 25883cd5f0
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Commit v8.4.0-287-gd4d3bb8130 tried to make sure the original
pre-migration memory locking limit is restored at the end of migration,
but it missed the case when libvirt daemon is restarted during
migration which needs to be aborted on reconnect.
And if this was not enough, I forgot to actually save the status XML
after setting the field in priv (in the commit mentioned above and also
in v8.4.0-291-gd375993ab3).
https://bugzilla.redhat.com/show_bug.cgi?id=2107424
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The function would fail to release the job in case
qemuMigrationSrcIsAllowed failed.
Fixes v8.5.0-157-g69e0e33873
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
When the <loader stateless='yes'/> attribute is set, the QEMU driver
needs to do three things
- Avoid looking for an NVRAM template
- Avoid auto-populating an <nvram/> path
- Find firmware descriptors with mode=stateless instead of mode=split
Note, the first thing happens automatically when we solve the second
thing.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Normally when an UEFI firmware is marked as read-only, an associated
NVRAM file will be created. Some builds of UEFI firmware, however, wish
to remain stateless and so will be read-only, but never have any NVRAM
file. To represent this concept a 'stateless' tristate bool attribute
is introduced on the <loader/> element.
There are rather a large number of permutations to consider.
With default firmware selection
* <os/>
=> Historic default, no change
* <os>
<loader stateless='yes'/>
</os>
=> Explicit version of historic default, no change
* <os>
<loader stateless='no'/>
</os>
=> Invalid, bios is always stateless
With manual legacy BIOS selection
* <os>
<loader>/path/to/seabios</loader>
...
</os>
=> Historic default, no change
* <os>
<loader stateless='yes'>/path/to/seabios</loader>
...
</os>
=> Explicit version of historic default, no change
* <os>
<loader stateless='no'>/path/to/seabios</loader>
...
</os>
=> Invalid, bios is always stateless
With manual UEFI selection
* <os>
<loader type='pflash'>/path/to/edk2</loader>
...
</os>
=> Historic default, no change
* <os>
<loader type='pflash' stateless='yes'>/path/to/edk2</loader>
...
</os>
=> Skip auto-filling NVRAM / template
* <os>
<loader type='pflash' stateless='no'>/path/to/edk2</loader>
...
</os>
=> Explicit version of historic default, no change
With automatic firmware selection
* <os firmware='bios'/>
=> Historic default, no change
* <os firmware='bios'>
<loader stateless='yes'/>
</os>
=> Explicit version of historic default, no change
* <os firmware='bios'>
<loader stateless='no'/>
</os>
=> Invalid, bios is always stateless
* <os firmware='uefi'/>
=> Historic default, no change
* <os firmware='uefi'>
<loader stateless='yes'/>
</os>
=> Skip auto-filling NVRAM / template
* <os firmware='uefi'>
<loader stateless='no'/>
</os>
=> Explicit version of historic default, no change
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Outline how upstream issues are triaged and explain what the states of
the issue means.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Add a note outling best practices around review and responding to it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@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>
Because qemuMigrationParamsReset used to call qemuMigrationParamsApply
for resetting migration capabilities and parameters, it did not work
well since commit v5.1.0-83-ga1dec315c9 which only allowed capabilities
to be set from an async job. However, when reconnecting to running
domains after daemon restart we do not have an async job. Thus the
capabilities were not properly reset in case the daemon was restarted
during an ongoing migration. We need to avoid calling
qemuMigrationParamsApply to make sure both parameters and capabilities
can be reset by a normal job.
https://bugzilla.redhat.com/show_bug.cgi?id=2107892
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
qemuMigrationParamsApply restricts when capabilities can be set, but
this is not useful in all cases. Let's create new helpers for setting
migration capabilities and parameters which can be reused in more places
without the restriction.
https://bugzilla.redhat.com/show_bug.cgi?id=2107892
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We keep original values of migration parameters so that we can restore
them at the end of migration to make sure later migration does not use
some random values. However, this does not really work when libvirt
daemon is restarted on the source host because we failed to explicitly
save the status XML after getting the migration parameters from QEMU.
Actually it might work if the status XML is written later for some other
reason such as domain state change, but that's not how it should work.
https://bugzilla.redhat.com/show_bug.cgi?id=2107892
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
While we all understand that excessive use of ternary operator
may worsen code readability (e.g. nested, multi-line expression),
there are few cases where using it actually improves code
readability. For instance, when a function takes a long list of
arguments out of which one depends on a boolean expression, or
when formatting "yes"/"no" or "on"/"off" values based on a
boolean variable (although one can argue that the latter is a
subset of the former). Just consider alternatives to:
virBufferAsprintf(buf, "<elem>%s</elem>\n", boolVar ? "yes" : "no");
In fact, this pattern occurs plenty in our code. Exempt it from
our "no ternary operators" rule.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The VIR_ENUM_IMPL macros directly above them list one string per line.
Use the same also for qemuMonitorMigrationStatus and
qemuMonitorVMStatus.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It's no longer possible for libvirt to connect over the ssh transport
from RHEL 9 to RHEL 5. This is because SHA1 signatures have been
effectively banned in RHEL 9 at the openssl level. They are required
to check the RHEL 5 host key. Note this is a separate issue from
openssh requiring additional configuration in order to connect to
older servers.
Connecting from a RHEL 9 client to RHEL 5 server:
$ cat ~/.ssh/config
Host 192.168.0.91
KexAlgorithms +diffie-hellman-group14-sha1
MACs +hmac-sha1
HostKeyAlgorithms +ssh-rsa
PubkeyAcceptedKeyTypes +ssh-rsa
PubkeyAcceptedAlgorithms +ssh-rsa
$ virsh -c 'qemu+ssh://root@192.168.0.91/system' list
error: failed to connect to the hypervisor
error: Cannot recv data: ssh_dispatch_run_fatal: Connection to 192.168.0.91 port 22: error in libcrypto: Connection reset by peer
"error in libcrypto: Connection reset by peer" is the characteristic
error of openssl having been modified to disable SHA1 by default.
(You will not see this on non-RHEL-derived distros.)
You could enable the legacy crypto policy which downgrades security on
the entire host, but a more fine-grained way to do this is to create
an alternate openssl configuration file that enables the "forbidden"
signatures. However this requires passing the OPENSSL_CONF
environment variable through to ssh to specify the alternate
configuration. Libvirt filters out this environment variable, but
this commit allows it through. With this commit:
$ cat /var/tmp/openssl.cnf
.include /etc/ssl/openssl.cnf
[openssl_init]
alg_section = evp_properties
[evp_properties]
rh-allow-sha1-signatures = yes
$ OPENSSL_CONF=/var/tmp/openssl.cnf ./run virsh -c 'qemu+ssh://root@192.168.0.91/system' list
root@192.168.0.91's password:
Id Name State
--------------------
Essentially my argument here is that OPENSSL_CONF is sufficiently
similar in nature to KRB5CCNAME, SSH* and XAUTHORITY that we should
permit it to be passed through.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2062360
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
'cmdQemuMonitorCommandQMPWrap' is checking whether the user provided
string is not valid JSON to avoid wrapping it. In cases where it's not
JSON we ignore the error and add the wrapper.
If the caller then reports a different non-libvirt error the error from
the JSON parsing would be printed as well. Reset errors we ignore:
# virsh qemu-monitor-command cd --pass-fds a asdf
error: Unable to parse FD number 'a'
error: internal error: cannot parse json asdf: lexical error: invalid char in json text.
asdf
(right here) ------^
In the above case 'asdf' is not valid JSON, but the code did wrap it
into '{"execute":"asdf"}', the only problem is the argument for
--pass-fds.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
'lcitool' dropped alpine-314
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Historically, the dumpxml command reject any unknown arguments,
for instance:
virsh dumpxml fedora xxx
However, after v8.5.0-rc1~31 the second argument ('xxx') is
treated as an XPath, but it's not that clearly visible.
Therefore, require the --xpath switch, like this:
virsh dumpxml fedora --xpath xxx
Yes, this breaks already released virsh, but I think we can argue
that the pool of users of this particular function is very small.
We also document the argument being mandatory:
dumpxml [--inactive] [--security-info] [--update-cpu] [--migratable]
[--xpath EXPRESSION] [--wrap] domain
The sooner we do this change, the better.
The same applies for other *dumpxml functions (net-dumpxml,
pool-dumpxml, vol-dumpxl to name a few).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2103524
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Note that we can only do this for intel-iommu and virtio-iommu,
which are configured using -device; smmuv3 is configured using
a machine type property, so there's no room on the command line
for an alias in that case.
https://bugzilla.redhat.com/show_bug.cgi?id=2108483
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When hotplugging a chardev, Libvirt opens corresponding
file/binds to a socket/does whatever necessary to obtain an FD
that is later passed to QEMU. However, due to wrong placement of
the function that does all of this
(qemuProcessPrepareHostBackendChardevHotplug()) it may happen
that a file is set seclabel on, only to be unlink()-ed and
created again (the former is done by
qemuSecuritySetChardevLabel(), the latter by aforementioned
function). The unlink()-ing is done for UNIX sockets with
mode='bind' and happens inside qemuOpenChrChardevUNIXSocket().
However, these steps can be swapped simply.
Fixes: ad81aa8ad0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
When hotplugging a chardev, Libvirt opens corresponding
file/binds to a socket/does whatever necessary to obtain an FD
that is later passed to QEMU. However, if something fails after
the FDs were transferred to QEMU and before chardev is actually
added via monitor, these FDs are never closed in QEMU. This is
rather suboptimal.
Fixes: 15bdced9b3
Fixes: ad81aa8ad0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
Pattern of using switch instead of a long if else construction is
used everywhere, so I used it here as well to make the code more
consistent (and remove that else after return). I also included
all the values from the enum.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The else branches are redundant because the execution will never
reach them if the conditions in the previous 'if' branches are
true.
I think this looks cleaner and is more readable, because having
'else' branch indicates that no return / break / goto is in the
previous branch and the function can reach it.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This patch rewrites conditions to make the code easier to read and less
nested.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
