Peter Krempa [Mon, 1 Mar 2021 09:25:18 +0000 (10:25 +0100)]
virPipeImpl: Don't overwrite error
If WITH_PIPE2 is not defined we attempt to set the pipe to nonblocking
operation after they are created. We errorneously rewrote the existing
error message on failure to do so or even reported an error if quiet
mode was requested.
Fixes: ab36f729470c313b9d5b7debdbeac441f7780dec Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Thu, 25 Feb 2021 16:52:47 +0000 (17:52 +0100)]
qemuProcessReportLogError: Don't mark "%s: %s" as translatable
The function is constructing an error message from a prefix and the
contents of the qemu log file. Marking just two string modifiers as
translatable is pointless and will certainly confuse translators.
Remove the marking and add a comment which bypasses the
sc_libvirt_unmarked_diagnostics check.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 14:27:07 +0000 (15:27 +0100)]
qemuProcessReportLogError: Remove unnecessary math for max error message
Now that error message formatting doesn't use fixed size buffers we can
drop the math for calculating the maximum chunk of log to report in the
error message and use a round number. This also makes it obvious that
the chosen number is arbitrary.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 14:21:13 +0000 (15:21 +0100)]
util: virprocess: Use local maximum error message size
Use of VIR_ERROR_MAX_LENGTH is actually misleading to the readers
because it implies that the strings in virError are 1024 bytes at most.
That isn't true at least for the 'message' field as it's constructed
from concatenating the detail string which (was) max 1024 bytes with
the string variant of the error code without limiting to 1024.
Use a local copy for declaring the struct for error transport with a
comment so that's obvious that it's a local decision to use 1k buffers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Thu, 25 Feb 2021 16:20:12 +0000 (17:20 +0100)]
util: virerror: Avoid a copy of the error messages
Some error message reporting functions already have allocated buffers
which were used to format the error message, so copying the strings is
redundant.
Extract the internals from 'virRaiseErrorFull' to
'virRaiseErrorInternal' which takes allocated strings as arguments and
steals them, so that callers can reuse the buffers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 14:38:02 +0000 (15:38 +0100)]
util: virerror: Don't use stack'd buffers in error report helpers
This was (probably) a relict from times when we cared about OOM
conditions and the possibility to report the error. Nowadays it doesn't
make sense as virRaiseErrorFull will do an allocated copy of the strings
and also concatenate the error message prefix with the detail which
doesn't guarantee that the result will be less than 1024 chars.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Tue, 2 Mar 2021 10:00:23 +0000 (11:00 +0100)]
util: virstring: Always copy string in virStrcpy
15 out of 72 invocations of virStrcpy(Static) ignore the return value as
it's either impossible to fail or in certain cases a truncated copy is
still good enough. Unfortunately virStrcpy doesn't copy anything in
such case as the checks are done first.
Fix this by using g_strlcpy for the implementation and removing
G_GNUC_WARN_UNUSED_RESULT from the function so that callers can decide
when it's okay.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Surprisingly, we never documented the relationship between
connection URI and the location of qemu.conf. Users might wonder
what qemu.conf is loaded when they are connecting to the session
daemon or embed URI. And what to do if the file doesn't exist for
the URI they're using.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Jim Fehlig [Mon, 1 Mar 2021 22:57:05 +0000 (15:57 -0700)]
build: Require glib >= 2.56
There are a few uses of g_autoslist in the qemu driver and likely more
will come throughout the codebase in the future. g_autoslist first
appeared in glib 2.56, so bump the minimum version
Bumping the minimum version is an opportune time to update the list of
minimum glib versions found on the distros targeted by libvirt's
platform support policy
FreeBSD 13.x and newer ship BSD grep which apparently has some
performance issues causing certain syntax check tests to run longer than
the default 30 seconds timeout used by meson.
However, GNU grep is still available through the textproc/gnugrep port,
so require it on FreeBSD if /usr/bin/grep is a BSD grep to make checks
pass in a reasonable time.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Tim Wiederhake [Thu, 4 Mar 2021 08:35:31 +0000 (09:35 +0100)]
domain_conf: Fix check for hyperv stimer
VIR_DOMAIN_HYPERV_STIMER happens to have the same numerical value as
VIR_DOMAIN_FEATURE_HYPERV, resulting in the if-block to always being
executed when a "<hyperv>" tag is found, whether or not it actually
contained a "<stimer>" tag. This had no ill effects, as virXPathNodeSet()
would simply return 0 if that tag does not exist.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Pavel Hrdina [Wed, 3 Mar 2021 13:26:53 +0000 (14:26 +0100)]
cgroup: use virCgroupSetCpuShares instead of virCgroupSetupCpuShares
Now that we enforce the cpu.shares range kernel will no longer silently
change the value that libvirt configures so there is no need to read
the value back to get the actual configuration.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Pavel Hrdina [Wed, 3 Mar 2021 13:10:15 +0000 (14:10 +0100)]
vircgroup: enforce range limit for cpu.shares
Before the conversion to using systemd DBus API to set the cpu.shares
there was some magic conversion done by kernel which was documented in
virsh manpage as well. Now systemd errors out if the value is out of
range.
Since we enforce the range for other cpu cgroup attributes 'quota' and
'period' it makes sense to do the same for 'shares' as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Jim Fehlig [Mon, 1 Mar 2021 22:41:44 +0000 (15:41 -0700)]
libxl: Fix node device detach when driver unspecified
Commit 887dd0d331 caused a small regression in NodeDeviceDetach in the libxl
driver when the 'driver' parameter is not specified. E.g.
# virsh nodedev-detach pci_0000_0a_10_0
error: Failed to detach device pci_0000_0a_10_0
error: An error occurred, but the cause is unknown
If the driver name is not specified, NULL is passed to
virDomainDriverNodeDeviceDetachFlags, in which case virPCIDeviceSetStubDriver
is never called to set the stub to pciback. Fix it by setting the driver to
"xen" if it is not specified when invoking NodeDeviceDetach.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Peter Krempa [Wed, 24 Feb 2021 10:34:00 +0000 (11:34 +0100)]
util: virerror: Remove virReportOOMError
Trying to report an OOM error is pointless since our infrastructure to
report error needs to allocate memory to report the error.
In addition our code mistakenly reported OOM errors even in cases where
a function could fail for another reason, which would make issues harder
to debug.
Remove the virReportOOMError and backend so that programmers are forced
to think about what can happen. In case when there's another failure
possible a specific error should be reported and otherwise a direct
abort() is better since the logger would abort on g_new anyways.
This patch also removes the syntas-check which forces use of
virReportOOMError instead of using VIR_ERR_NO_MEMORY with other
functions. This allows possible future use when we'd end up in a
situation where trying to recover from an OOM would make sense, such as
when attempting to allocate a massive buffer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Wed, 24 Feb 2021 09:22:52 +0000 (10:22 +0100)]
storage: Don't report OOM error on failure of glfs_new
OOM isn't the only failure glfs_new can encounter. Report an error which
might give more insight. libgfapi seems to be setting errno but
reporting a system error migt be misleading.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Wed, 24 Feb 2021 09:13:27 +0000 (10:13 +0100)]
util: json: Report non-OOM error on yajl failure
The yajl library returns a wide range of error codes so reporting OOM on
any failure is wrong. In case the error was really based by memory issue
the error reporting will probably cause an abort anyways. Change the
error message so that we know that it happened in JSON at least.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 17:28:31 +0000 (18:28 +0100)]
virURIFormat: abort() on failure
If the argument of 'xmlSaveUri' is non-NULL the function returns NULL on
OOM failure only. Thus we can directly abort rather than try to do the
impossible recovery.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 16:57:13 +0000 (17:57 +0100)]
util: xml: Add virXMLBufferCreate wrapper
'xmlBufferCreate' returns NULL only on allocation failure. Add a wrapper
which will call 'abort()' in such case in a centralised spot. It doesn't
make much sense to continue execution from here.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Wed, 24 Feb 2021 08:51:19 +0000 (09:51 +0100)]
util: virnetlink: Add wrapper for 'nlmsg_alloc_simple'
The function is used in many places and fails only on allocation
failures. Since trying to recover from allocation failure of a small
buffer by reporting error doesn't make sense add a wrapper for
'nlmsg_alloc_simple' which will 'abort()' on failure and replace all
allocations of netlink message with the new helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Wed, 24 Feb 2021 09:46:59 +0000 (10:46 +0100)]
virDomainDefSetMetadata: Rework memory handling
Switch to use g_autoptr for 'doc' and 'new' local variables.
Additionally report proper error when 'xmlAddChild' fails because OOM is
not the only error it can report.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 07:49:42 +0000 (08:49 +0100)]
virCommandAddArgBuffer: Simplify clearing of @buf
Get the buffer contents into a temporary variable with automatic
clearing so that the error branches don't have to reset the buffer.
Additionally handle the NULL string case before assignment.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Peter Krempa [Tue, 23 Feb 2021 07:43:08 +0000 (08:43 +0100)]
virCommandAddEnv: Make stealing of argument more obvious
The function is supposed to always consume the passed environment
variable string. Use a temp variable with autofree and g_steal_pointer
to prevent having to free it manually.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Michal Privoznik [Thu, 25 Feb 2021 10:58:38 +0000 (11:58 +0100)]
virtpm: Fix @path handling in virTPMEmulatorInit()
This function finds "swtmp", "swtpm_setup" and "swtpm_ioctl"
binaries in $PATH and stores resolved paths in global variables
so that they can be obtainer later. Anyway, the resolved path is
marked as g_autofree and to avoid its freeing later on in the
function the variable is set to NULL manually. Well, we have
g_steal_pointer() for that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Michal Privoznik [Wed, 24 Feb 2021 16:28:42 +0000 (17:28 +0100)]
qemu_tpm: Generate log file path among with storage path
When starting a guest with TPM of type='emulator' an external
process is started with it (swtpm) to emulate TPM. This external
process is passed path to a log file via --logfile. The path to
the log file is generated in qemuTPMEmulatorPrepareHost() which
works, until the daemon is restarted. The problem is that the
path is not stored in private data or anywhere inside live XML
and thus later, when qemuExtTPMStop() is called (when shutting
off the guest) the stored logpath is NULL and thus its seclabel
is not cleaned up (see virSecuritySELinuxRestoreTPMLabels()).
Fortunately, qemuExtDevicesStop() (which calls qemuExtTPMStop()
eventually) does call qemuExtDevicesInitPaths() where the log
path can be generated again.
Basically, tpm->data.emulator.storagepath is generated in
qemuExtTPMInitPaths() and its seclabels are restored properly,
and this commit move logfile onto the same level.
This means, that the log path doesn't have to be generated in
qemuExtDevicesStart() because it was already done in
qemuExtDevicesPrepareHost().
This change also renders @vmname argument of
qemuTPMEmulatorPrepareHost() unused and thus is removed.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1769196 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Michal Privoznik [Wed, 24 Feb 2021 16:17:41 +0000 (17:17 +0100)]
tools: Fix dry run of libvirt_recover_xattrs.sh
The libvirt_recover_xattrs.sh script can be used to remove stale
XATTRs that were left behind by secdrivers (which should happen
only if there's an imbalance between set and restore calls).
Anyway, the script has '-n' switch which is supposed to perform
just a dry run, i.e. just to report which files have XATTRs set
without any attempt to remove them.
But, when rewriting the script a few months ago a typo was
introduced which made the script report no files even if there
were files with XATTRs.
Fixes: 5377177f80da40ee7d47601400b50835f093715a Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
In files: src/lxc/lxc_native: in lxcAddNetworkRouteDefinition(),
src/conf/networkcommon_conf: in virNetDevIPRouteCreate() and
virNetDevIPRouteParseXML()
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Use g_autoptr instead of virNetDevIPRouteFree if possible
In files: src/conf/domain_conf: in virDomainNetIPInfoParseXML(),
src/lxc/lxc_native: in lxcAddNetworkRouteDefinition(),
src/vz/vz_sdk: in prlsdkGetRoutes(), src/conf/networkcommon_conf:
in virNetDevIPRouteCreate()
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Cole Robinson [Mon, 1 Mar 2021 18:15:37 +0000 (13:15 -0500)]
hyperv: Fix 32bit compilation
Example:
../src/hyperv/hyperv_driver.c:3007:54: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 7 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
3007 | virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach serial port %lu"), i);
virhostdev.c: remove missing PCI devs from hostdev manager
virHostdevReAttachPCIDevices() is called when we want to re-attach
a list of hostdevs back to the host, either on the shutdown path or
via a 'virsh detach-device' call. This function always count on the
existence of the device in the host to work, but this can lead to
problems. For example, a SR-IOV device can be removed via an admin
"echo 0 > /sys/bus/pci/devices/<addr>/sriov_numvfs", making the kernel
fire up and eventfd_signal() to the process, asking for the process to
release the device. The result might vary depending on the device driver
and OS/arch, but two possible outcomes are:
1) the hypervisor driver will detach the device from the VM, issuing a
delete event to Libvirt. This can be observed in QEMU;
2) the 'echo 0 > ...' will hang waiting for the device to be unplugged.
This means that the VM process failed/refused to release the hostdev back
to the host, and the hostdev will be detached during VM shutdown.
Today we don't behave well for both cases. We'll fail to remove the PCI device
reference from mgr->activePCIHostdevs and mgr->inactivePCIHostdevs because
we rely on the existence of the PCI device conf file in the sysfs. Attempting
to re-utilize the same device (assuming it is now present back in the host)
can result in an error like this:
$ ./run tools/virsh start vm1-sriov --console
error: Failed to start domain vm1-sriov
error: Requested operation is not valid: PCI device 0000:01:00.2 is in use by driver QEMU, domain vm1-sriov
For (1), a VM destroy/start cycle is needed to re-use the VF in the guest.
For (2), the effect is more nefarious, requiring a Libvirtd daemon restart
to use the VF again in any guest.
We can make it a bit better by checking, during virHostdevReAttachPCIDevices(),
if there is any missing PCI device that will be left behind in activePCIHostdevs
and inactivePCIHostdevs lists. Remove any missing device found from both lists,
unconditionally, matching the current state of the host. This change affects
the code path in (1) (processDeviceDeletedEvent into qemuDomainRemoveDevice, all
the way back to qemuHostdevReAttachPCIDevices) and also in (b) (qemuProcessStop
into qemuHostdevReAttachDomainDevices).
NB: Although this patch enables the possibility of 'outside Libvirt' SR-IOV
hotunplug of PCI devices, if the hypervisor and the PCI driver copes with it,
our goal is to mitigate what it is still considered a user oopsie. For all
supported purposes, the admin must remove the SR-IOV VFs from all running domains
before removing the VFs from the host.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/72 Reviewed-by: Laine Stump <laine@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFindIndex()
We're going to need a way to remove a PCI Device from a list without having
a valid virPCIDevicePtr, because the device is missing from the host. This
means that virPCIDevicesListDel() must operate with a PCI Device address
instead.
Turns out that virPCIDevicesListDel() and its related functions only use
the virPCIDeviceAddressPtr of the virPCIDevicePtr, so this change is
simple to do and will not cause hassle in all other callers. Let's
start adapting virPCIDeviceListFindIndex() and crawl our way up to
virPCIDevicesListDel().
Reviewed-by: Laine Stump <laine@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
projects / thirdparty / libvirt.git / log
