Do for all other profiles what we already do for the
virt-aa-helper one. In this case we limit the feature to AppArmor
3.x, as it was never implemented for 2.x.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
For AppArmor 3.x we can use 'include if exists', which frees us
from having to create a dummy override. For AppArmor 2.x we keep
things as they are to avoid introducing regressions.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Implement the standard AppArmor 3.x abstraction extension
approach.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
The subprofile can only work by including the abstraction shipped
in the passt package, which we can't assume is present, and
'include if exists' doesn't work well on 2.x.
No distro that's stuck on AppArmor 2.x is likely to be shipping
passt anyway.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Compared to profiles, we only need a single preprocessing step
here, as there is no variable substitution happening.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Perform an additional preprocessing step before the existing
variable substitution. This is the same approach that we already
use to customize systemd unit files based on whether the service
supports TCP connections.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
After v8.1.0-61-g030faee28d it is no longer necessary to make the
/proc/meminfo file nonseekable as our code that fills the file
with spoofed values can handle seeking just fine.
Previously, `free(1)` was okay with failed lseek(), but this was
ages ago and meanwhile the procps project moved to creating a
library and moved the file parsing code under an exported
function. In attempt to make the function callable multiple
times, it can lseek() multiple times and failure to do so is
fatal.
This reverts commit 766495508650bebd5f4ac23224ecd0a2ee2ca9eb
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/492
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Fix the syntax-check failures (which can be seen after
python3-flake8-import-order package is installed) with the help
of isort[1]:
289/316 libvirt:syntax-check / flake8 FAIL 5.24s exit status 2
[1]: https://pycqa.github.io/isort/
Signed-off-by: Han Han <hhan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As it turns out, apparmor 2.x and 3.x behave differently or have differing
levels of support for local customizations of profiles and profile
abstractions. Additionally the apparmor 2.x tools do not cope well with
'include if exists'. Revert this commit until a more complete solution is
developed that works with old and new apparmor.
Reverts: 9b743ee19053db2fc3da8fba1e9cf81915c1e2f4
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
If VIR_ASYNC_JOB_NONE flag is present, job.current is equal
to NULL, which leads to SIGSEGV. Thus, this check should be
moved up.
Fixes: v8.0.0-427-gf304de0df6
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
In one of my previous commits I've introduced @logfd variable
that was supposed to hold FD of passt logfile. But I've forgot to
assign the qemuDomainOpenFile() retval to it.
Fixes: 8511b96a319836700b4829816cdae27c3630060d
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There are a few situations where passt itself is unable to create
a file because it runs under QEMU user (e.g. just like our
example from formatdomain.rst suggests: /var/log/passt.log). If
libvirtd runs with sufficient permissions (e.g. as root) it can
create the file and set seclabels on it so that passt can then
open it.
Ideally, we would just pass pre-opened FD, but this wasn't viewed
as secure enough [1]. So lets just create the file and set
seclabels.
For the case when both libvirtd and passt have the same
permissions, well then we fail before even needing to fork() and
exec().
1: https://archives.passt.top/passt-dev/20230606225836.63aecebe@elisabeth/
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2209191
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
New storage types are not implemented in generators for -drive and the
xen config. Explicitly reject them in case of a programming error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If there are no parameters, there is nothing to validate.
If params == NULL, memcpy below results in memcpy(sorted, NULL, 0),
which is UB.
Found by UBSAN. Example of this codepath: virDomainBlockCopy()
(where nparams == 0 is valid) -> qemuDomainBlockCopy()
Signed-off-by: Oleg Vasilev <oleg.vasilev@virtuozzo.com>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
When detaching a device, the following race condition may happen:
Once qemuDomainSignalDeviceRemoval() marks the device for
removal, it returns true, which means it is the caller
that marked the device for removal is going to remove the
device from domain definition.
But qemuDomainWaitForDeviceRemoval() may still receive
timeout from virDomainObjWaitUntil() which is implemented
by pthread_cond_timedwait() due to an unavoidable race
between the expiration of the timeout and the predicate
state(priv->unplug.alias) change.
And then qemuDomainWaitForDeviceRemoval() will return 0,
thus the caller will not remove the device from domain
definition.
In this situation, the device is still present in the domain
definition but doesn't exist in qemu anymore. Worse, there is
no way to remove it from the domain definition.
Solution is to recheck the value of priv->unplug.alias to
determine who is going to remove the device from domain
definition.
Signed-off-by: zuo boqun <zuoboqun@baidu.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Qemu 8.1.0 will add discard_no_unref option for qcow2 images.
When this option is enabled (default=false), then it will no longer
unreference clusters when guest does a discard, but it will just free
the blocks (useful for incremental backups for example) and pass the
discard to the lower layer.
This was implemented to avoid fragmentation within the qcow2 image.
Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The qcow2 driver allows passing discards to the storage while keeping
the reference of the block, and just marking it as zeroed. This can
decrease the levels of fragmentation of the qcow2 metadata when
discards are enabled.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Memory slots are required only for DIMM-like devices, but the maximum
memory address space is relevant also for other non-DIMM memory devices
such as virtio-mem. Allow configurations where no slots are added.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Memory slots are required only for DIMM-like devices, while other
devices defined via <memory> such as virtio-mem may use the PCI bus and
thus do not require/consume a memory slot.
Fix the validation code to calculate the required count of memory
devices only for DIMMs and NVDIMMs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Specify the memory size by using '-m size=2048k' instead of just '-m 2'.
The new syntax is used when memory hotplug is enabled. To preserve
memory sizing, if memory hotplug is disabled the size is rounded down to
the nearest mebibyte.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This should not be needed, but here's what's happening:
virStrToLong_*() family of functions was switched from strtol*()
to g_ascii_strtol*() in order to handle corner cases on Windows
(most notably parsing hex numbers with base=0) - see
v9.4.0-61-g2ed41d7cd9. But what we did not realize back then, is
the fact that g_ascii_strtol*() family has their own global lock
rendering virStrToLong_*() function unsafe between fork() +
exec(). Worse, if one of the threads has to wait for the lock (or
on its corresponding condition), then errno is mangled and
g_ascii_strtol*() signals an error, even though there's no error.
Read more here:
https://gitlab.gnome.org/GNOME/glib/-/issues/3034
Nevertheless, if we make glib init the g_ascii_strtol*() global
state (by calling one function from g_ascii_strtol*() family),
then there shouldn't be any congestion on the lock and thus no
errno mangling.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The current implementation of virConnectBaselineHypervisorCPU in QEMU
driver can provide a CPU definition that will not work on all hosts in
case they have different maximum physical address size. So when we get
the info from domain capabilities, we need to choose the smallest
physical address size for the computed baseline CPU definition.
https://bugzilla.redhat.com/show_bug.cgi?id=2171860
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We already report the hosts physical address size in host capabilities,
but computing a baseline CPU definition is done from domain
capabilities.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Newer GCC (13.1.1 in my case) wrongly reports "maybe uninitialized"
warning for this variable inside the next condition. Even though this
accusation is wrong (the condition is guarded by the same condition as
the for cycle initializing it), initialize it during the declaration so
compilation errors don't stop others and maybe also future proof the
code for changes.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
This has two main advantages:
- it parses the number with C locale explicitly
- it behaves the same on Windows as on Linux and BSD
both of which are wanted behaviours.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
With the last user gone this function can be abolished. It is
preferable to use _ll instead since that is not a subject to 32/64 bit
scaling.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It is used to fill an unsigned long long anyway and if it is negative
than there is really an issue somewhere.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The @unionMems argument of qemuProcessSetupPid() function is not
necessary really as all callers pass 'true'. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The unit that cpuset CGroups controller works with is a
thread/process, not individual memory allocations. Therefore,
after we've set cpuset.mems for emulator (after previous commit
it's set to union of all host NUMA nodes allowed for given
domain), and as we try to set up cpuset.mems for vCPUs/IOThreads,
memory is migrated to selected NUMA node(s). We are effectively
saying: "this thread (vCPU thread) can have memory only from
these NUMA node(s)".
That's not really what we want though. The cpuset controller
doesn't differentiate memory "belonging" to the emulator thread
and vCPU thread or IOThread even.
Therefore, set union of all allowed host NUMA nodes, just like
we're doing for the emulator thread.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2138150
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
In ideal world, my plan was perfect. We allow union of all host
nodes in cpuset.mems and once QEMU has allocated its memory, we
'fix up' restriction of its emulator thread by writing the
original value we wanted to set all along. But in fact, we can't
do it because that triggers memory movement. For instance,
consider the following <numatune/>:
<numatune>
<memory mode="strict" nodeset="0"/>
<memnode cellid="1" mode="strict" nodeset="1"/>
</numatune>
<numa>
<cell id="0" cpus="0-1" memory="1024000" unit="KiB" />
<cell id="1" cpus="2-3" memory="1048576" unit="KiB"/>
</numa>
This is meant to create 1:1 mapping between guest and host NUMA
nodes. So we start QEMU with cpuset.mems set to "0-1" (so that it
can allocate memory even for guest node #1 and have the memory
come fro host node #1) and then, set cpuset.mems to "0" (because
that's where we wanted emulator thread to live).
But this in turn triggers movement of all memory (even the
allocated one) to host NUMA node #0. Therefore, we have to just
keep cpuset.mems untouched and rely on .host-nodes passed on the
QEMU cmd line.
The placement still suffers because of cpuset.mems set for vcpus
or iothreads, but that's fixed in next commit.
Fixes: 3ec6d586bc3ec7a8cf406b1b6363e87d50aa159c
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Apparmor profiles in /etc/apparmor.d/ are config files that can and should
be replaced on package upgrade, which introduces the potential to overwrite
any local changes. Apparmor supports local profile customizations via
/etc/apparmor.d/local/<service> [1].
This change makes the support explicit by adding libvirtd, virtqemud, and
virtxend profile customization stubs to /etc/apparmor.d/local/. The stubs
are conditionally included by the corresponding main profiles.
[1] https://ubuntu.com/server/docs/security-apparmor
See "Profile customization" section
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In one of its commits [1] libssh2 changed the 'text' member of
LIBSSH2_USERAUTH_KBDINT_PROMPT struct from 'char' to 'unsigned
char'. But we g_strdup() the member in order to fill 'prompt'
member of virConnectCredential struct. Typecast the value to
avoid warnings. Also, drop @prompt variable, as it's needless.
1: 83853f8aea
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Use automatic memory freeing and modern XML parsers to simplify the
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Format the rule attributes in two passes, first for positive 'match' and
second pass for negative. This removes the crazy logic for switching
between match modes inside the formatter.
The refactor makes it also more clear in which cases we actually do
format something.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLNodeGetSubelementList to get the elements to process.
The new approach documents the complexity of the parser, which is
designed to ignore unknown attributes and parse only a single kind of
them after finding the first valid one.
Note that the XML schema doesn't actually allow having multiple
sub-elements, but I'm not sure how that translates to actual configs
present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use modern parsing. Invalid numbers are now rejected. Semantis for
numbers out of range is preserved.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Convert the fields to the proper types and use virXMLPropEnum for
parsing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to simplify the formatter. Drop return value of
virNWFilterRuleDefFormat as there are no errors to report.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLNodeGetSubelement(List) instead of the looped parser and
simplify the code.
Note that handling of the 'bootp' element now conforms to the schema
where we allow just one and the 'file' attribute is mandatory.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The new helper is similar to virXPathNodeSet list but for cases where we
want to get subelements directly rather than using XPath.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There's nothing to clean up in the 'host' local variable on error as
the function which fills it makes sure to fill it only on success. In
such case it's also directly assigned to the array thus the 'host'
variable is cleared.
Remove the 'cleanup' label and 'ret' variable as we can now directly
return -1 on error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Extract the 'inbound'/'outbound' subelements using
virXMLNodeGetSubelement to simplify the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove the unnecessary check for valid arguments and use
virXMLPropULongLong instead of hand-written property parsers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Every caller will pass 'qdevid' as it's populated in the data
mandatorily with qemu-4.2 and onwards due to mandatory -blockdev use.
Thus we can drop compatibility with the old way of matching the disk via
alias.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Every caller will pass 'qdevid' as it's populated in the data
mandatorily with qemu-4.2 and onwards due to mandatory -blockdev use.
Thus we can drop compatibility with the old way of matching the disk via
alias.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Historically this didn't work with any supported qemu version as we
don't set the alias of the device, and thus qemu uses a different alias
resulting in a failure to startup the VM:
internal error: unable to execute QEMU command 'block_set_io_throttle': Device 'drive-sd-disk0' not found
Refuse setting throttling as this is unlikely to be needed and proper
fix requires using -device instead of -drive if=sd.
Note that this was broken when I moved the setup of throttling as a
command at startup for blockdev integration quite a while ago. Until
then throttling was passed as arguments for -drive.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
