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>
Commit 6262752460 added the acquiring of a job, but it is not always
VIR_ASYNC_JOB_MIGRATION_OUT, so the code fails when doing save or anything else.
Correct the async job by passing it from the caller as another parameter.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Add a new flag VIR_LXC_PROCESS_CLEANUP_AUTODESTROY to
virLXCProcessCleanupFlags for skipping removal of the autodestroy
callback so that fake reboot of the container doesn't need to fetch the
connection and re-register it.
Since virLXCProcessReboot is defined before virLXCProcessCleanupFlags,
this patch also moves the flag enum typedef to the beginning of the
file.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Use automatic unlocking of the 'vm' object, so that we can return early
when no autostart is needed and avoid passing of the 'driver' object
which is already present in 'vm's' private data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
The connection object is not needed when autostarting containers so we
can remove the machinery for it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Remove the pointless 'cleanup' section and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
The function doesn't really need the connect object for anything besides
registering the autodestroy callback for it. If we merge it certain
callers can be simplified.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Add possibility for the caller to set the flags for the call to
'virLXCProcessCleanup'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
libvirt currently will block migration for any vfio-assigned device
unless it is a network device that is associated with a virtio-net
failover device (ie. if the hostdev object has a teaming->type ==
VIR_DOMAIN_NET_TEAMING_TYPE_TRANSIENT).
In the future there will be other vfio devices that can be migrated,
so we don't want to rely on this hardcoded block. QEMU 6.0+ will
anyway inform us of any devices that will block migration (as a part
of qemuDomainGetMigrationBlockers()), so we only need to do the
hardcoded check in the case of old QEMU that can't provide that
information.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The new code that queries QEMU about migration blockers was put at the
top of qemuMigrationSrcIsAllowed(), but that function can also be
called in the case of offline migration (ie when the domain is
inactive / QEMU isn't running). This check should have been put inside
the "if (!(flags & VIR_MIGRATE_OFFLINE))" conditional, so let's move
it there.
Fixes: 156e99f686
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
