IF YOU WOULD LIKE TO GET AN ACCOUNT, please write an
email to Administrator. User accounts are meant only to access repo
and report issues and/or generate pull requests.
This is a purpose-specific Git hosting for
BaseALT
projects. Thank you for your understanding!
Только зарегистрированные пользователи имеют доступ к сервису!
Для получения аккаунта, обратитесь к администратору.
The refactor of 'udevListInterfacesByStatus()' which attempted to make
it usable as backend for 'udevNumOfInterfacesByStatus()' neglected to
consider the corner case of 'g_new0(..., 0)' returning NULL if the user
actually requests 0 elements.
As the code was modified to report the full number of interfaces in the
system when the list of names is NULL, the RPC code would be asked to
serialize a NULL-list of interface names with declared lenth of 1+
causing a crash.
To fix this corner case we make callers pass '-1' as @names_len (it's
conveniently an 'int' due to RPC type usage) if they don't wish to fetch
the actual list and convert all decisions to be done on @names_len being
non-negative instead of @names being non-NULL.
CVE-2024-8235
Fixes: bc596f2751
Resolves: https://issues.redhat.com/browse/RHEL-55373
Reported-by: Yanqiu Zhang <yanqzhan@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This advertises the feature only for the architectures and
machine types where it can actually be used.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We will soon need to use it in a context where we don't have
a virDomainDef handy.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Originally nicindexes were updated only for VIR_DOMAIN_NET_TYPE_BRIDGE
and VIR_DOMAIN_NET_TYPE_DIRECT. The mentioned commit adds support for
NAT network mode and changes the code to update nicindexes for
VIR_DOMAIN_NET_TYPE_ETHERNET and VIR_DOMAIN_NET_TYPE_NETWORK as well.
It doesn't work as intended and after the change nicindexes are updated
only for VIR_DOMAIN_NET_TYPE_ETHERNET and VIR_DOMAIN_NET_TYPE_NETWORK.
Fixes: aa64209073
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Inside of virschematest.c there's testSchemaDir() which iterates
over dentries in given directory but skips some files: those
without ".xml" suffix, hidden files, symlinks, etc.
Now, symlinks are detected as g_lstat() + S_ISLNK() combo which
works, except it fails to compile on mingw where is no concept of
symlinks. Replace the combo with a call to virFileIsLink() which
at least allows us to compile cleanly on mingw.
Fixes: f997fcca71
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
It's possible to hit the following situation during qemu p2p live
migration:
1. qemu has live migrated and exited (making virDomainObjIsActive()
return false)
2. the live migration job is still in progress, waiting for a
confirmation from the remote libvirt daemon. This may last for
a while with a presence of networking issues (up to keepalive
timeout).
Any attempt to start the domain again would fail with "domain is already
being started" message which is misleading in this situation as it
doesn't reflect what's really happening.
Add a check for the migration job and report a different error message
if the migration job is still running.
Signed-off-by: Sergey Dyasli <sergey.dyasli@nutanix.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Mingw build failed after commit af87ee7927
as 'socketpair()' is not available on that platform.
Stub out the function to return failure.
Fixes: af87ee7927
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
'bhyveConnectGetType' (which is called from 'virConnectGetType') returns
'BHYVE' as the type, but the code in 'remoteDispatchConnectOpen'
responsible for selecting the sub-driver URIs in modular deployment
checks for 'bhyve' and thus would not properly fill the URIs to the
sub-daemons.
Signed-off-by: aokblast <aokblast@FreeBSD.org>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
From: Praveen K Paladugu <prapal@linux.microsoft.com>
enable VIR_DOMAIN_NET_TYPE_NETWORK network support for ch guests.
Tested with following config:
<interface type='network'>
<source network="default" bridge='virbr0'/>
<model type='virtio'/>
<driver queues="1"/>
</interface>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
qemu supports this enlightenment since version 7.10.
From the qemu commit:
Hyper-V specification allows to pass parameters for certain hypercalls
using XMM registers ("XMM Fast Hypercall Input"). When the feature is
in use, it allows for faster hypercalls processing as KVM can avoid
reading guest's memory.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
qemu supports this enlightenment since version 7.10.
From the qemu commit:
The newly introduced enlightenment allow L0 (KVM) and L1 (Hyper-V)
hypervisors to collaborate to avoid unnecessary updates to L2
MSR-Bitmap upon vmexits.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Currently, qemuProcessStop() unlocks given domain object right in
the middle of cleanup process. This is dangerous because there
might be another thread which is executing virDomainObjListAdd().
And since the domain object is on the list of domain objects AND
by the time qemuProcessStop() unlocks it the object is also
marked as inactive, the other thread acquires the lock and
switches vm->def pointer.
The unlocking of domain object is needed though, to allow even
processing thread finish its queue. Well, the processing can be
done before any cleanup is attempted.
Therefore, use freshly introduced virEventThreadStop() to join
the event thread and drop lock/unlock from the middle of
qemuProcessStop().
Now, there's a comment being removed that mentions
qemuDomainObjStopWorker() and why it has to be called only after
the domain is marked as dead. This comment is no longed
applicable because call to qemuDomainObjStopWorker() is removed
also. Moreover, priv->beingDestroyed is set to true before
unlocking the domain object, thus any event processing callback
is going to see the domain being destroyed and can chose to
either exit early or finish processing event.
Fixes: 3865410e7f
Resolves: https://issues.redhat.com/browse/RHEL-49607
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The aim is to move parts of vir_event_thread_finalize() that MAY
block into a separate function, so that unrefing the a
virEventThread no longer blocks (or require releasing and
subsequent re-acquiring of a mutex).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Introduce tests to verify that the 'ps2' feature is correctly parsed
when given either 'dirty' XML from a user or 'clean' canonical XML,
as produced by libvirt. This also tests the transformation from libvirt's
internal state to the aforementioned canonical form and to a QEMU
command line.
As a bonus, we also test some known bad configurations:
- When user explicitly adds ps2 bus inputs, but also explicitly disables
the 'ps2' feature.
- When user explicitly enables the 'vmport' feature, but also explicitly
disables the 'ps2' feature. This is not supported by QEMU and will
result in vmport device not being created without emitting any warning
or error.
Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This introduces a new 'ps2' feature which, when disabled, results in
no implicit PS/2 bus input devices being automatically added to the
domain and addition of the 'i8042=off' machine option to the QEMU
command-line.
A notable side effect of disabling the i8042 controller in QEMU is that
the vmport device won't be created. For this reason we will not allow
setting the vmport feature if the ps2 feature is explicitly disabled.
Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This capability tells us whether given QEMU binary supports the
'-machine xxx,i8042=on/off' toggle used to enable/disable PS/2
controller emulation.
A few facts:
- This option was introduced in QEMU 7.0 and defaults to 'on'
- QEMU versions before 7.0 enabled i8042 controller emulation implicitly
- This option (and i8042 controller emulation itself) is only supported
by descendants of the generic PC machine type (e.g. i440fx, q35, etc.)
Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Up until now, we've assumed that all x86 machines have a PS/2
controller built-in. This assumption was correct until QEMU v4.2
introduced a new x86-based machine type - microvm.
Due to this assumption, a pair of unnecessary PS/2 inputs are implicitly
added to all microvm domains. This patch fixes that by whitelisting
machine types which are known to include the i8042 PS/2 controller.
Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Attempting to start qemu with or hotplug an empty 'usb-storage' based
disk results in the following error:
qemu-system-x86_64: -device {"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":true}: drive property not set
Reject such config at validation step and adjust tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Some code paths, such as if hotplug of an empty cdrom fails can cause
that 'qemuBlockStorageSourceChainDetach' will be called with 'NULL'
@data as there is no backend for the disk.
The above case became possible once we allowed hotplug of cdroms and
subsequently fixed the case when users would hotplug an empty cdrom
which ultimately caused the possibility of having no backend in the
hotplug code path which was not possible before (see 'Fixes:' below and
also the commit linked from there).
Make 'qemuBlockStorageSourceChainDetach' tolerate NULL @data by simply
returning early.
Fixes: 894c6c5c16
Resolves: https://issues.redhat.com/browse/RHEL-54550
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
In its commit v9.0.0-rc0~1^2 QEMU started to read
/proc/sys/vm/max_map_count file to set up coroutine limits better
(something about VMAs, mmap(), see the commit for more info).
Allow the file in apparmor profile.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/660
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This function validates whether parsed limits are within range as
defined by 'tc' sources (since we use tc to set QoS; or OVS which
then uses tc too). The 'tc' program stores speeds in 64bit
integers (unit is bytes per second) and sizes in uints (unit is
bytes). We use different units: kilobytes per second and
kibibytes and therefore we can parse values larger than 'tc' can
handle and thus need a function to check if values still fit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Instead of having many if-else statements, each with its own
vshTableRowAppend() call, we can use a simple trick - have an
array of string pointers, set array members in the if bodies and
then call vshTableRowAppend() once.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
All calls to vshTableRowAppend() inside of cmdList() share couple
of same arguments: domain ID, domain name and domain state. While
the first one is stored in a variable and then passed to all
vshTableRowAppend() calls, the others are passed as a function
call. Switch the latter to variables too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There is a family of convenient macros: NULLSTR, NULLSTR_EMPTY,
NULLSTR_STAR, NULLSTR_MINUS which hides ternary operator.
Generated using the following spatch (and its obvious variants):
@@
expression s;
@@
<+...
- s ? s : "<null>"
+ NULLSTR(s)
...+>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Check for the last multipart message right as the first thing. The
presumption probably was that the last message might still contain a
payload we want to parse. However that cannot be true since that would
have to be a type RTM_NEWNEIGH. This was not caught because older
kernels were note sending NLMSG_DONE and probably relied on the fact
that the parsing just stops after all the messages are walked through,
which the NLMSG_OK macro successfully did.
Resolves: https://issues.redhat.com/browse/RHEL-52449
Resolves: https://bugzilla.redhat.com/2302245
Fixes: a176d67cdf
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
The previous check was all wrong since it calculated the how long would
the netlink message be if the netlink header was the payload and then
subtracted that from the whole message length, a variable that was not
used later in the code. This check can fail if there are no additional
payloads, struct rtattr in particular, which we are parsing later,
however the RTA_OK macro would've caught that anyway.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Use convenience macro which does almost the same thing we were doing,
but also pads out the payload length to a multiple of NLMSG_ALIGNTO (4)
bytes.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
This will allow to print full domains info:
Id Name State UUID
---------------------------
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It was released on June 12, 2024.
The update means we no longer have to care about json-c 0.13
present in Leap 15.5, which solves some whitespace issues in
tests.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Some JSON parsers do not like bare types outside of objects or arrays
or do validation of object key uniqueness.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Based on discussion after commit f432114d9c was pushed it was pointed
out that the documentation still mentions the older version.
Fix the documentation to state the new version and introduce ambiguity
for future updates.
Fixes: f432114d9c
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This brings the tool's list of features in sync with qemu
commit 37fbfda8f4145ba1700f63f0cb7be4c108d545de.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This adds an option to use libcpuinfo [1] as data source for
libvirt's list of x86 cpu features. This is purely optional and
does not change the script's behavior if libcpuinfo is not
installed.
libcpuinfo is a cross-vendor, cross-architecture source for CPU
related information that has the capability to replace libvirt's
dependence on qemu's cpu feature list.
[1] https://gitlab.com/twiederh/libcpuinfo
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This mostly reverts commit 65491a2dfe.
There was a bug introduced in glib 2.67.0 which impacted libvirt with
clang causing -Wincompatible-pointer-types-discards-qualifiers warnings.
This was actually fixed quite quickly in 2.67.1 with
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719
Our workaround was then broken with glib 2.81.1 due to commit
14b3d5da9019150d821f6178a075d85044b4c255 changing the signature of the
(private) macro we were overriding.
Since odd-number glib releases are development snapshots, and the
original problem was only present in 2.67.0 and no other releases,
just drop the workaround entirely.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Add test cases for few edge cases which excercise the XML reporting from
libxml2 in anticipation of upcoming changes of behaviour.
'virschematest' must skip parsing of the broken file altogether so this
patch adds infrastructure to allow that.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Upcoming patch will result in having the build directory path in some of
the output files. Replace it by a constant 'ABS_SRCDIR' to avoild
breaking tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Backport the implementation of 'g_string_replace' until we require at
least glib-2.68
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Invoke virCHProcessStop to kill CH process incase of any failures during
restore operation.
Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Instead of curl, use low-level socket connections to make restore api
request to CH. This will enable passing new net FDs to CH while
restoring domains with network configuration.
Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
chSocketRecv fn can be used by operations such as restore, which cannot
have a specific poll timeout. The runtime of these operations at server
side (vmm) cannot be determined or capped as it depends on the guest
configuration. Hence, add a new parameter 'use_timeout' which when set
will pass -1 as timeout to poll, otherwise the default PKT_TIMEOUT_MS is
used.
Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Move monitor socket connection, response handling and closing FDs code into
new functions in preparation for adding restore support for net devices.
Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Pass "net_<index>" as net id to CH. This is to have better control over
the network configs. This id can be further used in performing
operations like restore etc.
Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The response message from CH for vm.add-net api will be more helpful in
debugging. Hence, log the message instead of just response code.
Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Latest qemu will be dropping some very old machine types (2.0 - 2.3) and
some of our tests use them. As in none of the cases the test actually
needs given machine type, switch them to 'pc' instead.
In one case 'numavcpus-topology-mismatch' this caused switch to a more
modern syntax for NUMA memory specification, but the test is testing a
different aspect, thus we can modernize this.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This excercises the old-style NUMA memory commandline used with 5.0 and
older machine types:
-smp 16,sockets=2,dies=1,clusters=1,cores=4,threads=2 \
-numa node,nodeid=0,cpus=0-7,mem=107 \
-numa node,nodeid=1,cpus=8-15,mem=107 \
in contrast to the modern syntax:
-smp 16,sockets=2,dies=1,clusters=1,cores=4,threads=2 \
-object '{"qom-type":"memory-backend-ram","id":"ram-node0","size":112197632}' \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
-object '{"qom-type":"memory-backend-ram","id":"ram-node1","size":112197632}' \
-numa node,nodeid=1,cpus=8-15,memdev=ram-node1 \
which is tested by the 'cpu-numa1' test case where this was copied from.
This test is added so that other irrelevant test can be modernized.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Depending on timing between QEMU and libvirt an attempt to resume failed
post-copy migration could immediately report a failure in post-copy
phase again even though the migration actually resumed and is
progressing just fine.
This is caused by QEMU reporting the original migration state (i.e.,
postcopy-paused) until migration is successfully resumed and QEMU
switches to postcopy-active. QEMU 9.1 introduced a new
postcopy-recover-setup migration state which is entered immediately
after requesting migration to be resumed and we can reliably wait for
the migration to either continue or fail without being confused by the
old state.
https://issues.redhat.com/browse/RHEL-22166
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This patch adds support for recognizing the new migration state reported
by QEMU when post-copy recovery is requested. It is not actually used
for anything yet.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Thing about vshReadlineInit() is - it's called multiple times.
The first time from vshInit(), when @ctl was filled only
partially (most notably, before any argv parsing is done, hence
ctl->imode is set to false). The second time after argv parsing,
from virshInit() -> vshInitReload(). In here, ctl->imode might
have changed and thus vshReadlineInit() can't exit early - it
needs to set up stuff for interactive mode (history basically).
To allow vshReadlineInit() to be called again,
vshReadlineDeinit() must set @autoCompleteOpaque to NULL.
Fixes: cab1e71f01
Resolves: https://issues.redhat.com/browse/RHEL-53560
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The QEMU package in Debian has recently moved the
qemu-bridge-helper binary under /usr/libexec/qemu. Update the
AppArmor profile accordingly.
https://bugs.debian.org/1077915
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Replace the 'misc-acpi' case by testing a bunch of architectures for how
ACPI is handled including a test for the s390 ACPI strip hack added in
previous commit.
The input files are adapted from the corresponding '-minimal.xml' files.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
The s390(x) machines never supported ACPI. That didn't stop users
enabling ACPI in their config. As of libvirt-9.2 (98c4e3d073) with new
enough qemu we reject configs which require ACPI, but qemu can't satisfy
it.
This breaks migration of existing VMs with the old wrong configs to new
libvirt installations.
To address this introduce a post-parse fixup removing the ACPI flag
specifically for s390 machines which do enable it in the definition.
The advantage of doing it in post-parse, rather than simply relaxing the
ABI stability check to allow users providing an fixed XML when migrating
(allowing change of the ACPI flag for s390 in ABI stability check, as it
doesn't impact ABI), is that only the destination installation needs to
be patched in order to preserve migration.
To mitigate the disadvantage of simply stripping it from all s390(x)
configs the hack is not applied when defining or starting a new domain
from the XML, to preserve the error about unsupported configuration.
Resolves: https://issues.redhat.com/browse/RHEL-49516
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
This reverts commit cf934c87cc.
The matching logic is flawed and it would complicate support of
this command.
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The whole point of pstore device is that the guest writes crash
dumps into it. But the way SELinux label is set on the
corresponding file warrants RO access only. This is due to a
copy-paste from code around: kernel/initrd/DTB/SLIC - these are
RO indeed, but pstore MUST be writable too. In a sense it's
closer to NVRAM/disks - hence set imagelabel on it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
So far we are relying on QEMU or sysadmin to create the file for
pstore. This is suboptimal as in the case of the former we can
not set proper seclabels (there's nothing to set seclabels on
until QEMU is started).
Therefore, make sure the file is created before launching QEMU
and that it has the correct size.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
There are some features/improvements/bug fixes I've either
contributed or reviewed/merged. Document them for upcoming
release.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Introduced only a couple of commits ago (in
v10.5.0-84-g90e50e67c6) the pstore device acts as a nonvolatile
storage, where guest kernel can store information about crashes.
This device, however, expects a file in the host from which the
crash data is read. So far, we expected users to provide a path,
but we can autogenerate one if missing. Just put it next to
per-domain's NVRAM stores.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
As can be seen in earlier commits, there can be two OEM strings
with the same index. But since our parser
(virSysinfoParseOEMStrings()) doesn't expect that, it increments
index in each run and thus skips over these strings.
Fortunately, we have the right index at hand - we're just
skipping over it in a loop. Just reconstruct the index back
inside the loop.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
On some systems, there are two or even more 'OEM Strings'
sections in DMI table. Here's an example of dmidecode output on
such system:
# dmidecode -q -t 11
OEM Strings
String 1: Default string
OEM Strings
String 1: ThunderX2 System
String 2: cavium.com
String 3: Comanche
Now, this poses a problem, because when one tries to obtain
individual strings, they get:
# dmidecode -q --oem-string 1
Default string
ThunderX2 System
# dmidecode -q --oem-string 2
No OEM string number 2
cavium.com
NB, the "No OEM string number 2" is printed onto stderr and
everything else onto stdout. Oh, and trying to get OEM strings
from just one section doesn't fly:
# dmidecode -q -H 0x1d --oem-string 2
Options --string, --type, --handle and --dump-bin are mutually exclusive
This means two things:
1) we have no way of distinguishing OEM strings at the same index
but in different sections,
2) because of how virSysinfoDMIDecodeOEMString() is written, we
fail in querying OEM string that exists in one section but not
in the others (for instance string #2 from example above).
While there's not much we can do about 1), there is something
that can be done about 2) - refine the error condition and make
the function return an error iff there's nothing on stdout and
there's something on stderr.
Resolves: https://issues.redhat.com/browse/RHEL-45952
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Introduce a test case for sysinfotest. The data was obtained by
running dmidecode as libvirt would run it:
dmidecode -q -t 0,1,2,3,4,11,17
Now, the expected output fits almost perfectly, except for OEM
strings where the third string looks nothing like in the
dmidecode output. This is because of testDMIDecodeDryRun() which
overwrites the third OEM string (see v6.5.0-rc1~214 for more
info). But that's okay for now.
Speaking of OEM strings, it's worth noticing two 'OEM Strings'
sections in the dmidecode output. This is causing some troubles
and will be fixed in next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
If dry run of a command was requested (virCommandSetDryRun())
then a specified callback is called instead of running actual
command. This is meant to be used in tests. To mimic running the
command as closely as possible the callback can also set exit
status of the command it's implementing. To save some lines
though, the exit status is initialized to 0 so that callback has
to set it only on failures. Now, 0 is not exactly portable value
- that's why stdlib.h has EXIT_SUCCESS (and EXIT_FAILURE) values.
Initialize the exit status (held in dryRunStatus) to EXIT_SUCCESS
then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The acpi-erst backend for pstore device exposes a path in the
host accessible to the guest and as such we must set seclabels on
it to grant QEMU RW access.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
The aim of pstore device is to provide a bit of NVRAM storage for
guest kernel to record oops/panic logs just before the it
crashes. Typical usage includes usage in combination with a
watchdog so that the logs can be inspected after the watchdog
rebooted the machine. While Linux kernel (and possibly Windows
too) support many backends, in QEMU there's just 'acpi-erst'
device so stick with that for now. The device must be attached to
a PCI bus and needs two additional values (well, corresponding
memory-backend-file needs them): size and path. Despite using
memory-backend-file this does NOT add any additional RAM to the
guest and thus I've decided to expose it as another device type
instead of memory model.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
In cases when a QEMU process takes longer than the time sigterm and
sigkill are issued to kill the process do not simply fail and leave the
VM in state VIR_DOMAIN_SHUTDOWN until the daemon stops. Instead set up
an fd on /proc/$pid and get notified when the QEMU process finally has
terminated to cleanup the VM state.
Resolves: https://issues.redhat.com/browse/RHEL-28819
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If there are absent values in an already existing element
specifying rom settings, we simply use the old ones. This
behaviour is not desired, as users might think that deleting the
element from XML would delete the setting (because the hotplug
succeeds) - which does not happen. Because of that, we should not
accept an interface without elements that cannot be changed.
Therefore, we should not allow absent values for already existing
rom setting during hotplug.
Resolves: https://issues.redhat.com/browse/RHEL-7109
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
New element 'openfiles' had confusing name. Since the patch with
this new element wasn't propagate yet, old name ('rlimit_nofile')
was changed.
...
<binary>
<openfiles max='122333'/>
</binary>
...
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The current "building from git" test uses "test -d .git"; however, that
doesn't work when libvirt is used as a submodule, as in that case .git
is a normal file. Use "test -e .git" instead.
Signed-off-by: John Levon <john.levon@nutanix.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
On various occasions, virt-host-validate parses /proc/cpuinfo to
learn about CPU flags (see virHostValidateGetCPUFlags()). It does
so, by reading the file line by line until the line with CPU
flags is reached. Then the line is split into individual flags
(using space as a delimiter) and the list of flags is then
iterated over.
This works, except for cases when the line with CPU flags is too
long. Problem is - the line is capped at 1024 bytes and on newer
CPUs (and newer kernels), the line can be significantly longer.
I've seen a line that's ~1200 characters long (with 164 flags
reported).
Switch to unbounded read from the file (getline()).
Resolves: https://issues.redhat.com/browse/RHEL-39969
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
User feedback has shown that the examples are not clear enough
to illustrate the cli passthrough concept in action.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since libvirt commit 3ef9b51b10,
the pflash storage for the os loader file follows its read-only flag,
and qemu tries to open the file for writing if set so.
This patches virt-aa-helper to generate the VM's AppArmor rules
that allow this, using the same domain definition flag and default.
Signed-off-by: Miroslav Los <mirlos@cisco.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
It's available as part of the edk2-riscv64 Fedora package.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
By definition. Accordingly, filter them out when looking for
a read/write image.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If the configuration explicitly requests a specific type of
firmware image, be it pflash or ROM, we should ignore all images
that are not of that type.
If no specific type has been requested, of course, any type is
considered a match and the selection will be based upon the
other attributes.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This new test case covers the scenario in which the user
specifically asked for a read/write pflash image.
From the output files, we can see that the firmware selection
algorithm has picked a ROM image, which demonstrates the
presence of another bug. We're going to fix it with an upcoming
commit.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Sync with the edk2-20240524-4.fc39 package from Fedora.
The only notable change is that the inteltdx variant now declares
support for Secure Boot and is a ROM image instead of a stateless
pflash one.
The latter causes it to be considered eligible for the
configuration described by the firmware-auto-efi-rw test cases,
which now passes instead of failing.
Of course that doesn't make any sense, because a ROM image by
definition cannot be read/write. So this indicates the presence
of a bug in our firmware selection algorithm, which we're going
to address with an upcoming commit.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add an element to configure the rlimit nofile size:
...
<binary>
<rlimit_nofile size='122333'/>
</binary>
...
Non-positive values are forbidden in 'domaincommon.rng'. Added separate
test file, created by modifying the 'vhost-user-fs-fd-memory.xml'.
Signed-off-by: Adam Julis <ajulis@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
So much can happen in the fileName field of the VMX that the easiest
thing is to silently report a serial type="null".
This effectively reverts commits de81bdb8d4cd and 62c53db0421a, but
keeps the test files to show the fix is still in place.
There is one instance where an error gets reset, but since that is a
rare case on its own and on top of that does not happen in any of our
long-running daemons with a logfile that might get monitored it should
be fine to leave it there.
Resolves: https://issues.redhat.com/browse/RHEL-32182
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This is supposed to unstuck FreeBSD as it switched to
Python-3.11.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Recent commit v10.4.0-87-gd9935a5c4f made a reasonable change to only
reset beingDestroyed back to false when vm->def->id is reset to make
sure other code can detect a domain is (about to become) inactive. It
even added a comment saying any caller of qemuProcessBeginStopJob is
supposed to call qemuProcessStop to clear beingDestroyed. But not every
caller really does so because they first call qemuProcessBeginStopJob
and then check whether a domain is still running. If not the
qemuProcessStop call is skipped leaving beingDestroyed=true. In case of
a persistent domain this may block incoming migrations of such domain as
the migration code would think the domain died unexpectedly (even though
it's still running).
The qemuProcessBeginStopJob function is a wrapper around
virDomainObjBeginJob, but virDomainObjEndJob was used directly for
cleanup. This patch introduces a new qemuProcessEndStopJob wrapper
around virDomainObjEndJob to properly undo everything
qemuProcessBeginStopJob did.
https://issues.redhat.com/browse/RHEL-43309
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
If virt-host-validate is ran on a SEV-SNP capable machine, an
extra "PASS" is printed out. This is because
virHostValidateAMDSev() prints "PASS" and then returns 1
(indicating success) which in turn makes the caller
(virHostValidateSecureGuests()) print "PASS" again. Just drop the
extra printing in the caller and let virHostValidateAMDSev() do
all the printing.
Fixes: 1a8f646f29
Resolves: https://issues.redhat.com/browse/RHEL-46868
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Allow migration if the "migrate-precopy" capability is present or
libvirt is not the one running the virtiofs daemon.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Run the daemon with --print-capabilities first, to see what it supports.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Now that we have a fake virtiofsd json descriptor in our vhost-user
test data, we can remove the explicitly specified binary and our
mocking will ensure this test won't be affected by the host state.
Also remove the locking options, since they were never supported
by the Rust version of virtiofsd.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add the capabilities from the latest virtiofsd main branch and adjust
the order in the priority test accordingly.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As of now, libvirt supports few essential stats as
part of virDomainGetJobStats for Live Migration such
as memory transferred, dirty rate, number of iteration
etc. Currently it does not have support for the vfio
stats returned via QEMU. This patch adds support for that.
Signed-off-by: Kshitij Jha <kshitij.jha@nutanix.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The "modify" command allows to replace an existing record (its
text value). The primary key is the name of the record. If
duplicity or missing record detected, throw error.
Tests in networkxml2xmlupdatetest.c contain replacements of an
existing DNS-text record and failure due to non-existing record.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/639
Signed-off-by: Adam Julis <ajulis@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The "modify" command allows to replace an existing Srv record
(some of its elements respectively: port, priority and weight).
The primary key used to choose the modify record is the remaining
parameters, only one of them is required. Not using some of these
parameters may cause duplicate records and error message. This
logic is there because of the previous implementation (Add and
Delete options) in the function.
Tests in networkxml2xmlupdatetest.c contain replacements of an
existing DNS-Srv record and failure due to non-existing record.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/639
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The "modify" command allows you to replace an existing record
(its hostname, sub-elements). IP address acts as the primary key.
If it is not found, the attempt ends with an error message. If
the XML contains a duplicate address, it will select the last
one.
Tests in networkxml2xmlupdatetest.c contain replacements of an
existing DNS-Host record and failure due to non-existing record.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/639
Signed-off-by: Adam Julis <ajulis@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The outdated comment refers to a non-existent member in the
virDomainObj structure.
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When generating paths for a domain specific AppArmor profile each
path undergoes a validation where it's matched against an array
of well known prefixes (among other things). Now, for
OVMF/AAVMF/... images we have a list and some entries have
comments to which type of image the entry belongs to. For
instance:
"/usr/share/OVMF/", /* for OVMF images */
"/usr/share/AAVMF/", /* for AAVMF images */
But these comments are pretty useless. The path itself already
gives away the image type. Drop them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
This commit removes the redundant call to qemuSecurityGetNested() in
qemuStateInitialize(). In qemuSecurityGetModel(), the first security manager
in the stack is already used by default, so this change helps to
simplify the code.
Signed-off-by: hongmianquan <hongmianquan@bytedance.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Fix libvirtd hang since fork() was called while another thread had
security manager locked.
We have the stack security driver, which internally manages other security drivers,
just call them "top" and "nested".
We call virSecurityStackPreFork() to lock the top one, and it also locks
and then unlocks the nested drivers prior to fork. Then in qemuSecurityPostFork(),
it unlocks the top one, but not the nested ones. Thus, if one of the nested
drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If we always
surround nested locks with top lock, it is always secure. Because we have got top lock
before fork child libvirtd.
However, it is not always the case in the current code, We discovered this case:
the nested list obtained through the qemuSecurityGetNested() will be locked directly
for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the nested list
is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
The problem stack is as follows:
libvirtd thread1 libvirtd thread2 child libvirtd
| | |
| | |
virsh capabilities qemuProcessLanuch |
| | |
| lock top |
| | |
lock nested | |
| | |
| fork------------------->|(nested lock held by thread1)
| | |
| | |
unlock nested unlock top unlock top
|
|
qemuSecuritySetSocketLabel
|
|
lock nested (deadlock)
In this commit, we ensure that the top lock is acquired before the nested lock,
so during fork, it's not possible for another task to acquire the nested lock.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1303031
Signed-off-by: hongmianquan <hongmianquan@bytedance.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In a domain created with an interface with a <driver> subelement,
the device contains a non-NULL virDomainVirtioOptions struct, even
for non-virtio NIC models. The subelement need not be present again
after libvirt restarts, or when the interface is passed to clients.
When clients such as virsh domif-setlink put back the modified
interface XML, the new device's virtio attribute is NULL. This may
fail the equality checks for virtio options in qemuDomainChangeNet,
depending on whether libvird was restarted since define or not.
This patch modifies the check for non-virtio models, to ignore olddev
value of virtio (assumed valid), and to allow either NULL or a struct
with all values ABSENT in the new virtio options.
Signed-off-by: Miroslav Los <mirlos@cisco.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Similarly to commit 2482801608 we can safely ignore connectionId,
portId and portgroupId in both XML and VMX as they are only a blind
pass-through between XML and VMX and an ethernet without such parameters
was spotted in the wild. On top of that even our documentation says the
whole VMWare Distrubuted Switch configuration is a best-effort.
Resolves: https://issues.redhat.com/browse/RHEL-46099
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This wires up the emulator 'debug' parameter to control the
/usr/bin/swtpm 'level' parameter for logging.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Pick up some more of the qemu_driver.c code so this function supports
both CONFIG and LIVE updates.
Note that qemuDomainUpdateDeviceFlags() passed vm->def to
virDomainDeviceDefParse() for the VIR_DOMAIN_AFFECT_CONFIG case, which
is technically incorrect; in the test driver code we'll fix this.
Signed-off-by: John Levon <john.levon@nutanix.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
mem_nodes[i].ndistances is written outside the loop causing an out-of-bounds
write leading to heap corruption.
While we are at it, the entire cleanup portion can be removed as it can be
handled in virDomainNumaFree. One instance of VIR_FREE is also removed and
replaced with g_autofree.
This patch also adds a testcase which would be picked up by ASAN, if this
portion regresses.
Fixes: 742494eed8
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Under the test environment, driver->domainEventState is uninitialized. If a
disk gets dropped, it will attempt to queue an event which will cause a
segmentation fault. This crash does not occur during normal use.
This patch moves driver->domainEventState initialization from qemuhotplugtest
to qemuTestDriverInit in testutilsqemu (Credit goes to Michal Privoznik as he
had already provided the diff).
An additional test case is added to test dropping of disks with startupPolicy
set as optional.
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Changing the postgroup attribute caused unexpected behavior.
Although it can be implemented, it has a non-trivial solution.
No requirement or use has yet been found for implementing this
feature, so it has been disabled for hot-plug.
Resolves: https://issues.redhat.com/browse/RHEL-7299
Signed-off-by: Adam Julis <ajulis@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The current hostdev parsing logic sets rawio or sgio even if the hostdev type
is not 'scsi'. The rawio field in virDomainHostdevSubsysSCSI overlaps with
wwpn field in virDomainHostdevSubsysSCSIVHost, consequently setting a bogus
pointer value such as 0x1 or 0x2 from virDomainHostdevSubsysSCSIVHost's
point of view. This leads to a segmentation fault when it attempts to free
wwpn.
While setting sgio does not appear to crash, it shares the same flawed logic
as setting rawio.
Instead, we ensure these are set only after the hostdev type check succeeds.
This patch also adds two test cases to exercise both scenarios.
Fixes: bdb95b520c
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add basic coverage of device update; for now, only support disk updates
until other types are needed or tested.
Signed-off-by: John Levon <john.levon@nutanix.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The 'hostFips' member of _virQEMUDriver struct is not used
really, due to previous cleanups. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The support for VXHS device was removed in QEMU commit
v5.1.0-rc1~16^2~10. Since we require QEMU-5.2.0 at least there's
no QEMU that has the device and thus the corresponding capability
can be retired.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that the minimal required version of QEMU is 5.2.0 the
conditional setting of QEMU_CAPS_ENABLE_FIPS and
QEMU_CAPS_NETDEV_USER is effectively a dead code. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
According to repology.org and/or distro repos these are the version of QEMU:
CentOS Stream 9: qemu-kvm-9.0.0
Debian 11: qemu-5.2.0
Fedora 39: qemu-8.3.1
openSUSE Leap 15.3: qemu-5.2.0
RHEL-8: qemu-6.2.0
Ubuntu 22.04: qemu-6.2.0
Since the minimal version is 5.2.0 we can bump from 4.2.0 to
5.2.0.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Soon, the minimal version of QEMU is going to be bumped to 5.2.0.
Drop capabilities for older versions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Soon, the minimal version of QEMU is going to be bumped to 5.2.0.
Drop test cases that require older version.
NB, iothreads-disk-virtio-ccw test is removed completely as we
already have plenty of other tests covering the same code paths.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The disk-network-tlsx509-vxhs.xml file will be removed soon. Drop
the test case in qemusecuritytest that relies on it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
As I don't have a sparc machine handy add emulated capabilities.
This patch is in preparation for bumping minimum qemu version beyond the
oldest 'sparc' caps we currently have.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It may happen that QEMU is compiled without SLIRP but with
support for passt. In such case it is acceptable to alter user
provided configuration and switch backend to passt as it offers
all the features as SLIRP.
Resolves: https://issues.redhat.com/browse/RHEL-45518
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that the logic for detecting supported net backend types has
been moved to domain capabilities generation, we can just use it
when validating net backend type. Just like we do for device
models and so on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous commits, domain capabilities XML reports basically
two possible values for backend type: 'default' and 'passt'.
Despite its misleading name, 'default' really means 'use
hypervisor's builtin SLIRP'. Since it's reported in domain
capabilities as a value accepted, make our parser and XML schema
accept it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
If mgmt apps on top of libvirt want to make a decision on the
backend type for <interface type='user'/> (e.g. whether past is
supported) we currently offer them no way to learn this fact.
Domain capabilities were invented exactly for this reason. Report
supported net backend types there.
Now, because of backwards compatibility, specifying no backend
type (which translates to VIR_DOMAIN_NET_BACKEND_DEFAULT) means
"use hyperviosr's builtin SLIRP". That behaviour can not be
changed. But it may happen that the hypervisor has no support for
SLIRP. So we have to report it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that we have a capability for each domain net backend we can
start validating user's selection against QEMU capabilities.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since -netdev user can be disabled during QEMU compilation, we
can't blindly expect it to just be there. We need a capability
that tracks its presence.
For qemu-4.2.0 we are not able to detect the capability so do the
next best thing - assume the capability is there. This is
consistent with our current behaviour where we blindly assume the
capability, anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There are some features/improvements/bug fixes I've either
contributed or reviewed/merged. Document them for upcoming
release.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When enabling switchover-ack on qemu from libvirt, the .party value
was set to both source and target; however, qemuMigrationParamsCheck()
only takes that into account to validate that the remote side of the
migration supports the flag if it is marked optional or auto/always on.
In the case of switchover-ack, when enabled on only the dst and not
the src, the migration will fail if the src qemu does not support
switchover-ack, as the dst qemu will issue a switchover-ack msg:
qemu/migration/savevm.c ->
loadvm_process_command ->
migrate_send_rp_switchover_ack(mis) ->
migrate_send_rp_message(mis, MIG_RP_MSG_SWITCHOVER_ACK, 0, NULL)
Since the src qemu doesn't understand messages with header_type ==
MIG_RP_MSG_SWITCHOVER_ACK, qemu will kill the migration with error:
qemu-kvm: RP: Received invalid message 0x0007 length 0x0000
qemu-kvm: Unable to write to socket: Bad file descriptor
Looking at the original commit [1] for optional migration capabilities,
it seems that the spirit of optional handling was to enhance a given
existing capability where possible. Given that switchover-ack
exclusively depends on return-path, adding it as optional to that cap
feels right.
[1] 61e34b0856 ("qemu: Add support for optional migration capabilities")
Fixes: 1cc7737f69 ("qemu: add support for qemu switchover-ack")
Signed-off-by: Jon Kohler <jon@nutanix.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Avihai Horon <avihaih@nvidia.com>
Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: YangHang Liu <yanghliu@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
In ideal world, where clients close connection gracefully their
SASL session is freed in virNetServerClientDispose() as it's
stored in client->sasl. Unfortunately, if client connection is
closed prematurely (e.g. the moment virsh asks for credentials),
the _virNetServerClient member is never set and corresponding
SASL session is never freed. The handler is still stored in
client private data, so free it in remoteClientCloseFunc().
20,862 (288 direct, 20,574 indirect) bytes in 3 blocks are definitely lost in loss record 1,763 of 1,772
at 0x50390C4: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501BDAF: g_object_new_internal.part.0 (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501D43D: g_object_new_with_properties (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501E318: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x49BAA63: virObjectNew (virobject.c:252)
by 0x49BABC6: virObjectLockableNew (virobject.c:274)
by 0x4B0526C: virNetSASLSessionNewServer (virnetsaslcontext.c:230)
by 0x18EEFC: remoteDispatchAuthSaslInit (remote_daemon_dispatch.c:3696)
by 0x15E128: remoteDispatchAuthSaslInitHelper (remote_daemon_dispatch_stubs.h:74)
by 0x4B0FA5E: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
by 0x4B0F591: virNetServerProgramDispatch (virnetserverprogram.c:299)
by 0x4B18AE3: virNetServerProcessMsg (virnetserver.c:135)
Resolves: https://issues.redhat.com/browse/RHEL-22574
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With a simple cpuid (Section "E.4.17 Function
8000_001Fh—Encrypted Memory Capabilities" in "AMD64 Architecture
Programmer’s Manual Vol. 3") we can detect whether CPU is capable
of running SEV-ES and/or SEV-SNP guests. Report these in
virt-host-validate tool.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The code that validates AMD SEV is going to be expanded soon.
Move it into its own function to avoid lengthening
virHostValidateSecureGuests() where the code lives now, even
more.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now that the logic for detecting supported launchSecurity types
has been moved to domain capabilities generation, we can just use
it when validating launchSecurity type. Just like we do for
device models and so on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In order to learn what types of <launchSecurity/> are supported
users can turn to domain capabilities and find <sev/> and
<s390-pv/> elements. While these may expose some additional info
on individual launchSecurity types, we are lacking clean
enumeration (like we do for say device models). And given that
SEV and SEV SNP share the same basis (info found under <sev/> is
applicable to SEV SNP too) we have no other way to report SEV SNP
support.
Therefore, report supported launchSecurity types in domain
capabilities.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
While it's very unlikely to have QEMU that supports SEV-SNP but
doesn't support plain SEV, for completeness sake we ought to
query SEV capabilities if QEMU supports either. And similarly to
QEMU_CAPS_SEV_GUEST we need to clear the capability if talking to
QEMU proves SEV is not really supported.
This in turn removes the 'sev-snp-guest' capability from one of
our test cases as Peter's machine he uses to refresh capabilities
is not SEV capable. But that's okay. It's consistent with
'sev-guest' capability.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Soon, QEMU_CAPS_SEV_SNP_GUEST is going to be dependant on more
than plain presence of "sev-snp-guest" object in QEMU. Explicitly
enable the capability for "launch-security-sev-snp" test so that
we can continue testing cmd line and xml2xml.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
An iSCSI device with zero hosts will result in a segmentation fault. This patch
adds a check for the number of hosts, which must be one in the case of iSCSI.
Minimal reproducing XML:
<domain type='qemu'>
<name>MyGuest</name>
<uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid>
<os>
<type arch='x86_64'>hvm</type>
</os>
<memory>4096</memory>
<devices>
<disk type='network'>
<source name='dummy' protocol='iscsi'/>
<target dev='vda'/>
</disk>
</devices>
</domain>
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Add plumbing for QEMU's switchover-ack migration capability, which
helps lower the downtime during VFIO migrations. This capability is
enabled by default as long as both the source and destination support
it.
Note: switchover-ack depends on the return path capability, so this may
not be used when VIR_MIGRATE_TUNNELLED flag is set.
Extensive details about the qemu switchover-ack implementation are
available in the qemu series v6 cover letter [1] where the highlight is
the extreme reduction in guest visible downtime. In addition to the
original test results below, I saw a roughly ~20% reduction in downtime
for VFIO VGPU devices at minimum.
=== Test results ===
The below table shows the downtime of two identical migrations. In the
first migration swithcover ack is disabled and in the second it is
enabled. The migrated VM is assigned with a mlx5 VFIO device which has
300MB of device data to be migrated.
+----------------------+-----------------------+----------+
| Switchover ack | VFIO device data size | Downtime |
+----------------------+-----------------------+----------+
| Disabled | 300MB | 1900ms |
| Enabled | 300MB | 420ms |
+----------------------+-----------------------+----------+
Switchover ack gives a roughly 4.5 times improvement in downtime.
The 1480ms difference is time that is used for resource allocation for
the VFIO device in the destination. Without switchover ack, this time is
spent when the source VM is stopped and thus the downtime is much
higher. With switchover ack, the time is spent when the source VM is
still running.
[1] https://patchwork.kernel.org/project/qemu-devel/cover/20230621111201.29729-1-avihaih@nvidia.com/
Signed-off-by: Jon Kohler <jon@nutanix.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Avihai Horon <avihaih@nvidia.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: YangHang Liu <yanghliu@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When starting a domain on a host which lacks a vmx-* CPU feature which
is expected to be enabled by the CPU model specified in the domain XML,
libvirt properly marks such feature as disabled in the active domain
XML. But migrating the domain to a similar host which lacks the same
vmx-* feature will fail with libvirt reporting the feature as missing.
This is because of a bug in the hack ensuring backward compatibility
libvirt running on the destination thinks the missing feature is
expected to be enabled.
https://issues.redhat.com/browse/RHEL-40899
Fixes: v10.1.0-85-g5fbfa5ab8a
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit 7c8e606b64 attempted to fix
the specification of the ramfb property for vfio-pci devices, but it
failed when ramfb is explicitly set to 'off'. This is because only the
'vfio-pci-nohotplug' device supports the 'ramfb' property. Since we use
the base 'vfio-pci' device unless ramfb is enabled, attempting to set
the 'ramfb' parameter to 'off' this will result in an error like the
following:
error: internal error: QEMU unexpectedly closed the monitor
(vm='rhel'): 2024-06-06T04:43:22.896795Z qemu-kvm: -device
{"driver":"vfio-pci","host":"0000:b1:00.4","id":"hostdev0","display":"on
","ramfb":false,"bus":"pci.7","addr":"0x0"}: Property 'vfio-pci.ramfb'
not found.
This also more closely matches what is done for mdev devices.
Resolves: https://issues.redhat.com/browse/RHEL-28808
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This patch adds some previously missing test cases that test for
proper firewall rule creation when the following are included in the
network definition:
* <forward dev='blah'>
* no forward element (an "isolated" network)
* nat port range when only ipv4 is nat-ed
* nat port range when both ipv4 & ipv6 are nated
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
When the chain names and table name used by the nftables firewall
backend were changed in commit
958aa7f274, I forgot to change the test
data file base.nftables, which has the extra "list" and "add
chain/table" commands that are generated for the first test case of
networkxml2firewalltest.c. When the full set of tests is run, the
first test will be an iptables test case, so those extra commands
won't be added to any of the nftables cases, and so the data in
base.nftables never matches, and the tests are all successful.
However, if the test are limited with, e.g. VIR_TEST_RANGE=2 (test #2
will be the nftables version of the 1st test case), then the commands
to add nftables table/chains *will* be generated in the test output,
and so the test will fail. Because I was only running the entire test
series after the initial commits of nftables tests, I didn't notice
this. Until now.
base.nftables has now been updated to reflect the current names for
chains/table, and running individual test cases is once again
successful.
Fixes: 958aa7f274
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
A user reported that if they set <forward mode='nat|route' dev='blah'>
starting the network would fail if the device 'blah' didn't already
exist.
This is caused by using "iif" and "oif" in nftables rules to check for
the forwarding device - these two commands work by saving the named
interface's ifindex (an unsigned integer) when the rule is added, and
comparing it to the ifindex associated with the packet's path at
runtime. This works great if the interface both 1) exists when the
rule is added, and 2) is never deleted and re-created after the rule
is added (since it would end up with a different ifindex).
When checking for the network's bridge device, it is okay for us to
use "iif" and "oif", because the bridge device is created before the
firewall rules are added, and will continue to exist until just after
the firewall rules are deleted when the network is shutdown.
But since the forward device might be deleted/re-added during the
lifetime of the network's firewall rules, we must instead us "oifname"
and "iifname" - these are much less efficient than "Xif" because they
do a string compare of the interface's name rather than just comparing
two integers (ifindex), but they don't require the interface to exist
when the rule is added, and they can properly cope with the named
interface being deleted and re-added later.
Fixes: a4f38f6ffe
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
A few commits ago (v10.4.0-101-gc65eba1f57) I've introduced
virDomainDefLaunchSecurityValidate() and a switch() statement in
it. Some cases are empty but are lacking 'break' statement which
is not valid. Provide missing 'break' statement.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The firmware descriptors have 'amd-sev-snp` feature which
describes whether firmware is suitable for SEV-SNP guests.
Provide necessary implementation to detect the feature and pick
the right firmware if guest is SEV-SNP enabled.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Pretty straightforward as qemu has 'sev-snp-guest' object which
attributes maps pretty much 1:1 to our XML model. Except for
@vcek where QEMU has 'vcek-disabled`, an inverted boolean, while
we model it as virTristateBool. But that's easy to map too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
SEV-SNP is an enhancement of SEV/SEV-ES and thus it shares some
fields with it. Nevertheless, on XML level, it's yet another type
of <launchSecurity/>.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This capability tracks sev-snp-guest object availability.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In QEMU commit v9.0.0-1155-g59d3740cb4 the return type of
'query-sev' monitor command changed to accommodate SEV-SNP. Even
though we currently support launching plain SNP guests, this will
soon change.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In a few instances there is a plain if() check for
_virDomainSecDef::sectype. While this works perfectly for now,
soon there'll be another type and we can utilize compiler to
identify all the places that need adaptation. Switch those if()
statements to switch().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The sectype member of _virDomainSecDef struct is already declared
as of virDomainLaunchSecurity type. There's no need to typecast
it to the very same type when passing it to switch().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
To avoid convolution of switch() inside of virDomainSecDefFormat() even
more (as new sectypes are added), move formatting into a separate
function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Some parts of SEV are to be shared with SEV SNP. In order to
reuse XML parsing / formatting code cleanly, let's move those
common bits into a new struct (virDomainSEVCommonDef) and adjust
rest of the code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
While working on qemuMonitorJSONGetSEVMeasurement() and
qemuMonitorJSONGetSEVInfo() I've noticed that if these functions
fail, they do so without appropriate error set. Fill in error
reporting.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When a VM terminates itself while it's being migrated in running state
libvirt would report wrong error:
error: cannot get locked memory limit of process 2502057: No such file or directory
rather than the proper error:
error: operation failed: domain is not running
Remember the error on error paths in qemuMigrationSrcConfirmPhase and
qemuMigrationSrcPerformPhase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
'qemuProcessStop()' clears the 'current' job data. While the code under
the 'error' label in 'qemuMigrationSrcRun()' does check that the VM is
active before accessing the job, it also invokes multiple helper
functions to clean up the migration including
'qemuMigrationSrcNBDCopyCancel()' which calls 'qemuDomainObjWait()'
invalidating the result of the liveness check as it unlocks the VM.
Duplicate the liveness check and explain why. The rest of the code e.g.
accessing the monitor is safe as 'qemuDomainEnterMonitorAsync()'
performs a liveness check. The cleanup path just ignores the return
values of those functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The function is a pointless wrapper on top of
qemuMigrationDstWaitForCompletion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Similarly to the one change in commit 4d1a1fdffd
we should be checking that the VM is not being yet destroyed if we've
invoked qemuDomainObjWait().
Use the new helper qemuDomainObjIsActive().
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The helper checks whether VM is active including the internal qemu
state. This helper will become useful in situations when an async job
is in use as VIR_JOB_DESTROY can run along async jobs thus both checks
are necessary.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Prevent the possibility that a VM could be considered as alive while
inside qemuProcessStop.
A recently fixed bug which unlocked the domain object while inside
qemuProcessStop showed that there's possibility to confuse the state of
the VM to be considered active while 'qemuProcessStop' is processing
shutdown of the VM. Ensure that this doesn't happen by clearing the
'beingDestroyed' flag only after the VM id is cleared.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There are few function calls done while cleaning up a stopped VM which
do require the old VM id, to e.g. clean up paths containing the 'short'
domain name in the path.
Anything else, which doesn't strictly require it can be moved after
clearing the 'id' in order to decrease likelyhood of potential bugs.
This patch moves all the code which does not require the 'id' (except
for the log entry and closing the monitor socket) after the statement
clearing the id and adds a comment explaining that anything in the
section must not unlock the VM object.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
'qemuDomainObjStopWorker()' which is meant to dispose of the event loop
thread for the monitor unlocks the VM object while disposing the thread
to prevent possible deadlocks with events waiting on the monitor thread.
Unfortunately 'qemuDomainObjStopWorker()' is called *before* the VM is
marked as inactive by clearing 'vm->def->id', but at the same time it's
no longer marked as 'beingDestroyed' when we're inside
'qemuProcessStop()'.
If 'vm' would be kept locked this wouldn't be a problem. Same way it's
not a problem for anything that uses non-ASYNC VM jobs, or when the
monitor is accessed in an async job, as the 'destroy' job interlocks
with those.
It is a problem for code inside an async job which uses
'qemuDomainObjWait()' though. The API contract of qemuDomainObjWait()
ensures the caller that the VM on successful return from it, but in this
specific reason it's not the case, as both 'beingDestroyed' is already
false, and 'vm->def->id' is not yet cleared.
To fix the issue move the 'qemuDomainObjStopWorker()' call *after*
clearing 'vm->def->id' and also add a note stating what the function is
doing.
Fixes: 860a999802
Closes: https://gitlab.com/libvirt/libvirt/-/issues/640
Reported-by: luzhipeng <luzhipeng@cestc.cn>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Document why this function exists and meaning of return values.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Clear the 'disk' member of 'blockjob' as we're freeing the disk object
at this point. While this should not normally happen it was observed
when other bug allowed the VM to be cleared while other threads didn't
yet finish.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Similarly to other blockjob handlers, if there's no disk associated with
the blockjob the handler needs to behave correctly. This is needed as
the disk might have been de-associated on unplug or other operations.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In newer DASD driver versions the ID_TYPE tag is supported. This tag is
missing after a system reboot but when the ccw device is set offline and
online the tag is included. To fix this version independently we need to
check if devices detected as type disk is actually a DASD to maintain
the node object consistency and not end up with multiple node objects
for DASDs.
Resolves: https://issues.redhat.com/browse/RHEL-39497
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The virNetworkObjSetFwRemoval() function is called at least two
times when there's a network running and network driver
initializes:
1) when loading state XMLs:
#0 virNetworkObjSetFwRemoval (obj=0x7fffd4028250, fwRemoval=0x7fffd4020ad0) at ../src/conf/virnetworkobj.c:258
#1 0x00007ffff7a69c68 in virNetworkLoadState (...) at ../src/conf/virnetworkobj.c:952
#2 0x00007ffff7a6a35d in virNetworkObjLoadAllState (...) at ../src/conf/virnetworkobj.c:1072
#3 0x00007ffff7f9625f in networkStateInitialize (...) at ../src/network/bridge_driver.c:624
2) when firewall rules are being reloaded:
#0 virNetworkObjSetFwRemoval (obj=0x7fffd4028250, fwRemoval=0x7fffd402e5b0) at ../src/conf/virnetworkobj.c:258
#1 0x00007ffff7f997b4 in networkReloadFirewallRulesHelper (obj=0x7fffd4028250, opaque=0x0) at ../src/network/bridge_driver.c:1703
#2 0x00007ffff7a6b09b in virNetworkObjListForEachHelper (payload=0x7fffd4028250, ...) at ../src/conf/virnetworkobj.c:1414
#3 0x00007ffff79287b6 in virHashForEachSafe (...) at ../src/util/virhash.c:387
#4 0x00007ffff7a6b119 in virNetworkObjListForEach (...) at ../src/conf/virnetworkobj.c:1441
#5 0x00007ffff7f99978 in networkReloadFirewallRules (...) at ../src/network/bridge_driver.c:1742
#6 0x00007ffff7f962f2 in networkStateInitialize (...) at ../src/network/bridge_driver.c:645
Since virNetworkObjSetFwRemoval() does not free the object stored
in the first call, the second call just overwrites the stored
pointer leading to a memory leak:
5,530 (48 direct, 5,482 indirect) bytes in 1 blocks are definitely lost in loss record 1,863 of 1,880
at 0x4848C43: calloc (vg_replace_malloc.c:1595)
by 0x4F1E979: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.7800.6)
by 0x4976E32: virFirewallNew (virfirewall.c:118)
by 0x4979BA9: virFirewallParseXML (virfirewall.c:1071)
by 0x4ABEB1E: virNetworkLoadState (virnetworkobj.c:938)
by 0x4ABF35C: virNetworkObjLoadAllState (virnetworkobj.c:1072)
by 0x4E9A25E: networkStateInitialize (bridge_driver.c:624)
by 0x4CB1FA6: virStateInitialize (libvirt.c:665)
by 0x15A6C6: daemonRunStateInit (remote_daemon.c:611)
by 0x49E69F0: virThreadHelper (virthread.c:256)
by 0x532B428: start_thread (in /lib64/libc.so.6)
by 0x5397373: clone (in /lib64/libc.so.6)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
As a part of parsing XML, virFirewallParseXML() calls
virXMLNodeContentString() and then passes the return value
further. But virXMLNodeContentString() is documented so that it's
the caller's responsibility to free the returned string, which
virFirewallParseXML() never does. This leads to a memory leak:
14,300 bytes in 220 blocks are definitely lost in loss record 1,879 of 1,891
at 0x4841858: malloc (vg_replace_malloc.c:442)
by 0x5491E3C: xmlBufCreateSize (in /usr/lib64/libxml2.so.2.12.6)
by 0x54C2401: xmlNodeGetContent (in /usr/lib64/libxml2.so.2.12.6)
by 0x49F7791: virXMLNodeContentString (virxml.c:354)
by 0x4979F25: virFirewallParseXML (virfirewall.c:1134)
by 0x4ABEB1E: virNetworkLoadState (virnetworkobj.c:938)
by 0x4ABF35C: virNetworkObjLoadAllState (virnetworkobj.c:1072)
by 0x4E9A25E: networkStateInitialize (bridge_driver.c:624)
by 0x4CB1FA6: virStateInitialize (libvirt.c:665)
by 0x15A6C6: daemonRunStateInit (remote_daemon.c:611)
by 0x49E69F0: virThreadHelper (virthread.c:256)
by 0x532B428: start_thread (in /lib64/libc.so.6)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit 23c4794488 added parsing of serial ports connected to vspc, but
the VM can also have a network serial port with an empty filename or no
filename at all. Parse these the same way, as a <serial type='null'>.
Resolves: https://issues.redhat.com/browse/RHEL-32182
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove unused declaration of the virDomainDiskFindByBusAndDst()
function. Removed in v5.9.0-rc1~91 and then mistakenly
re-introduced in v5.9.0-rc1~65.
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Sometimes in release hook it is useful to know if the VM shutdown was graceful
or not. This is especially useful to do cleanup based on the VM shutdown failure
reason in release hook. This patch proposes to use the last argument 'extra'
to pass VM shutoff reason in the call to release hook.
Making this change for Qemu and LXC.
Signed-off-by: Swapnil Ingle <swapnil.ingle@nutanix.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Instead of accessing the global `driver` object pass the `udevEventData` as
parameter to the thread handler and watch callback. This has the advantage that:
1. proper refcounting
2. easier to read and test
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Use a worker pool for processing the events (e.g. udev, mdevctl config changes)
and the initialization instead of a separate initThread and a mdevctl-thread.
This has the large advantage that we can leverage the job API and now this
thread pool is responsible to do all the "costly-work" and emitting the libvirt
nodedev events.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
It's better practice for all functions called by the threads to pass the driver
via parameter and not global variables. Easier to test and cleaner.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
The documentation of gobject signals reads:
"If you are connecting handlers to signals and using a GObject instance as your
signal handler user data, you should remember to pair calls to
g_signal_connect() with calls to g_signal_handler_disconnect() or
g_signal_handlers_disconnect_by_func(). While signal handlers are automatically
disconnected when the object emitting the signal is finalised..." [1]
This means that the signal handlers are automatically disconnected as soon as
the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`.
But this also means that it's possible that new work is tried to be scheduled
for the workerpool by the `mdevctlEventHandleCallback` (main thread context)
even if the workerpool has already been stopped by `nodeStateShutdownWait`. To
fully understand this, it's important to know that the main loop of the main
thread is still running for some time even after `nodeStateShutdownPrepare` has
been called. Let's avoid this situation by explicitly disconnect the signals
during `nodeStateShutdownPrepare`, which is called in the main thread, so that
no new work is attempted to be scheduled for the worker pool.
[1] https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handlers
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Introduce and use the driver functions for the node state shutdown preparation
and wait. As they're also called in the error/cleanup path of
`nodeStateInitialize`, they must be written in a way, that they can safely be
executed even if not everything is initialized.
In the next commit, these functions will be extended.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Even if `priv->udev_monitor` was never initialized, the mdevctlLock, udevThread
were. Therefore let's match the order of releasing the resources the order of
allocating the resources in `nodeStateInitialize`.
In addition, use `g_steal_pointer` in `g_list_free_full`.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Everything is released in `udevEventDataDispose` except for the threads, change
this as this makes the code easier to read as it can be simplified a little.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Commit a99d876a0f ("node_device: Use automatic mutex management") replaced the
locking mechanism and accidentally removed the comment with the reason why the
lock is taken. The reason was to "ensure only a single thread can query mdevctl
at a time", but this reason is no longer valid or maybe it never was. Therefore,
let's remove this lock and add a comment to `mdevCtl` what it protects.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
It is done a little differently everywhere in libvirt, but most common is to
test for != -1.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
When a mdev device is destroyed or stopped the udev remove event
handling needs to reset the active config data of the node object
representing a persisted mdev.
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
When an udev add, change or remove event occurs the mdev active config data
requires an update via mdevctl as the udev does not contain all config data.
This update needs to occur immediately and to be finished before the libvirt
nodedev event is issued to keep the API usage reliable.
After this change, scheduleMdevctlUpdate call is already called in
`udevAddOneDevice` and can therefore be removed in `udevHandleOneDevice`.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
@def is owned by @obj after adding it the node device object list. As soon as
the @obj lock has been released, another thread could free @obj and therefore
@def. If now someone accesses @def this would lead to a heap-use-after-free and
therefore most likely to a segmentation fault, therefore set @def to NULL after
the ownership has moved.
While at it, add comments to other code places why @def is set to NULL.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Two situations will trigger an udev add event:
1) the mdev is created when started (transient) or
2) the mdev was defined and is started
In case 1 there is no node object existing and no config data is copied.
In case 2 copying the active config data of an existing node object will
only copy invalid data. Instead copying the defined config data will
store valid data into the newly added node object.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
While the function is exported via header, the symbol itself was not.
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The ci/manifest.yml file references a package 'libclang-rt-dev' that
does not exist in libvirt-ci mappings.yml. The latest refresh in
commit 0759cf3fa6
Author: Michal Prívozník <mprivozn@redhat.com>
Date: Fri May 3 15:58:20 2024 +0200
ci: Introduce Ubuntu 24.04
was presumably done against a local change to libvirt-ci.git that
had not yet been merged, as the clang packages now appear on many
more build envs.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Although virDomainDeviceDefValidate() is called as a part of
parsing device XML routine, it validates only that single device.
The virDomainDefValidate() function performs a more comprehensive
check. It should detect errors resulting from dependencies
between devices, or a device and some other part of XML config.
Therefore, a call to virDomainDefValidate() is added at the end
of qemuDomainAttachDeviceConfig().
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There are two scenarios identified after the recent firewall backend
selection was introduced, which result in libvirtd failing to startup
due to an inability to find either iptables/nftables
- On Linux if running unprivileged with $PATH lacking the dir
containing iptables/nftables
- On non-Linux where iptables/nftables never existed
In the former case, it is preferrable to restore the behaviour whereby
the driver starts successfully. Users will get an error reported when
attempting to start any virtual network, due to the lack of permissions
needed to create bridge devices. This makes the missing firewall backend
irrelevant.
In the latter case, the network driver calls the 'nop' platform
implementation which does not attempt to implement any firewall logic,
just allowing the network to start without firewall rules.
To solve this are number of changes are required
* Introduce VIR_FIREWALL_BACKEND_NONE, which does nothing except
report a fatal error from virFirewallApply(). This code path
is unreachable, since we'll never create a virFirewall
object with with VIR_FIREWALL_BACKEND_NONE, so the error reporting
is just a sanity check.
* Ignore the compile time backend defaults and assume use of
the 'none' backend if running unprivileged.
This fixes the first regression, avoiding the failure to start
libvirtd on Linux in unprivileged context, instead allowing use
of the driver and expecting a permission denied when creating a
bridge.
* Reject the use of compile time backend defaults no non-Linux
and hardcode the 'none' backend. The non-Linux platforms have
no firewall implementation at all currently, so there's no
reason to permit the use of 'firewall_backend_priority'
meson option.
This fixes the second regression, avoiding the failure to start
libvirtd on non-Linux hosts due to non-existant Linux binaries.
* Change the Linux platform backend to raise an error if the
firewall backend is 'none'. Again this code path is unreachable
by default since we'll fail to create the bridge before getting
here, but if someone modified network.conf to request the 'none'
backend, this will stop further progress.
* Change the nop platform backend to raise an error if the
firewall backend is 'iptables' or 'nftables'. Again this code
path is unreachable, since we should already have failed to
find the iptables/nftables binaries on non-Linux hosts, so
this is just a sanity check.
* 'none' is not permited as a value in 'firewall_backend_priority'
meson option, since it is conceptually meaningless to ask for
that on Linux.
NB, 'firewall_backend_priority' allows repeated options temporarily,
which we don't want. Meson intends to turn this into a hard error
DEPRECATION: Duplicated values in array option is deprecated. This will become a hard error in the future.
and we can live with the reduced error checking until that happens.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
There's no need to guard virBufferEscapeString() with a call to
NULL as the very first thing the function does is check all three
arguments for NULL.
This patch was generated using the following spatch:
@@
expression X, Y, E;
@@
- if (E)
virBufferEscapeString(X, Y, E);
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
These projects are not strictly part of libvirt, but are closely related
with many of the same developers and we manage them with 'lcitool
manifest' so it is useful to have them on the dashboard.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Quite a few of the projects we have on the CI dashboard have been
archived at this point, thus don't show any pipeline status info.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The previous fix:
commit b069efe29c
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Fri Jun 14 19:57:06 2024 +0100
gitlab: fix codestyle CI job
was incomplete, as the job inheritance was also
broken.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The aim of virProcessSetAffinity() is to set affinity of given
process to given CPUs. While we currently print the PID into
logs, the CPU map is not printed. It may help when debugging
weird scenarios.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In one of my previous commits I've made us substract isolcpus
from all online CPUs when setting affinity on QEMU threads. See
commit below for more info on that. Nevertheless, this is
something that surely deserves an entry in log. I've chosen INFO
priority for now. We can promote that to a regular WARN if users
complain.
Fixes: da95bcb6b2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jobs whose names start with a '.' as treated as templates, so
not actually run in a pipeline.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We currently hardcode the systemd sysusersdir, but it is desirable to be
able to choose a different location in some cases. For example, Fedora
flatpak builds change the RPM %_sysusersdir macro, but we can't currently
honour that.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reported-by: Yaakov Selkowitz <yselkowi@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This conversion was missed in the previous commit:
commit a7eb7de531
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Thu Jun 6 12:57:08 2024 +0100
meson: allow systemd unitdir to be changed
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Yaakov Selkowitz <yselkowi@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
A couple of paths passed in the error messages, didnt match the paths
that were actually being tested.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Update to v9.0.0-1388-g80e8f06021 plus a patch from upstream fixing a
crash when probing, which has no impact on the data.
Notable changes:
- 'MEM_UNPLUG_ERROR' event removed
- 'discard-source' argument for 'blockdev-backup' added
- 'sev-snp-guest' QOM object added
- 'query-sev' now returns variants of the return object based on sev
type
- removed deprecated 'vcpu' field from trace-event infrastructure
- 'scsi' option of 'virtio-blk-pci' removed
(a variant of 'virtio-lun' qemuxmlconftest case was pinned to the
previous version to continue testing the positive use case)
- new cpu features:
'fred', 'succor', 'vmx-nested-exception', 'lkgs', 'overflow-recov',
'wrmsrns'
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The support will be dropped soon by qemu, and libvirt is not rejecting
such configurations. Add validation of this explicitly requested config.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virt-pki-validate command can validate the system certificate
directories. The remote driver, however, also supports a standard
per-user certs location, as well as a runtime custom path. This
extends the validation tool to be able to cope with these alternate
locations too.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virt-pki-validate tool is currently a shell script. We have a
general goal of eliminating use of shell in the project. By doing a
new implementation in C, we can also make use of our more thorough
sanity checking code to validate the certificate setup.
This new implementation the same output format as the host validation
tool for a more consistent user experiance.
It also eliminates the requirement to have certtool installed on
libvirt hosts, which has been an issue for Fedora flatpak packages
since certtool isn't in the default platform runtime.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The /etc/sysconfig/libvirtd file is a Fedora/RHEL specific concept.
Since those distros switched to systemd socket activation, the
existance of --listen parameter in /etc/sysconfig/libvirtd is no
longer a reliable check. This was further degraded with the switch
to modular daemons where virtproxyd takes over the role.
The /etc/sysconfig/iptables file is a Fedora/RHEL specific concept.
Since those distros switched to firewalld, this file is no longer
a reliable check.
Rather than complicating these checks, just remove them, so that
the virt-pki-validate tool focuses exclusively on TLS configuration
validation.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
These tools never supported passing an argument to --version, this is
a copy+paste mistake from virsh, which did support an argument.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The common messaging helpers will be reused in the new impl of the
virt-pki-validate tool.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When first writing the manpage in
commit 3decd4f9f1
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Wed Sep 16 14:42:57 2009 +0100
Make pki_check.sh into an installed & supported tool
I incorrectly credited Richard, instead of Daniel, who was the
author per
commit 62442d578d
Author: Daniel Veillard <veillard@redhat.com>
Date: Thu Jul 12 15:47:19 2007 +0000
* docs/libvir.html docs/remote.html: update the remote page,
add an index
* docs/pki_check.sh: shell script to check the PKI and client/server
environment.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The TLS cert validation logic will be reused for the new impl of the
virt-pki-validate tool.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This will facilitate moving much of the code into a new file in the
subsequent commit.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We'll want to access these paths from outside the TLS context code,
so split them into a standalone file.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Change the 'include' in the AppArmor policy to use 'include if exists'
when including <uuid>.files. Note that 'if exists' is only available
after AppArmor 3.0, therefore a #ifdef check must be added.
When the <uuid>.files is not present, there are some failures in the
AppArmor tools like the following, since they expect the file to exist
when using 'include':
ERROR: Include file /etc/apparmor.d/libvirt/libvirt-8534a409-a460-4fab-a2dd-0e1dce4ff273.files not found
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We currently hardcode the systemd unitdir, but it is desirable to be
able to choose a different location in some cases. For examples, Fedora
flatpak builds change the RPM %_unitdir macro, but we can't currently
honour that.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We link to libsasl2.so, so get a dep on cyrus-sasl-libs automatically.
The dep on cyrus-sasl-gssapi gets us the mechanism that matches our
default config.
The 'cyrus-sasl' package merely contains some man pages and the
saslauthd daemon, which is not required by libvirt. This dep appears
to have been redundant since we first added in
commit 1b1d647439
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Wed Dec 5 15:24:15 2007 +0000
Initial integration of SASL authentication, working for Kerberos only
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Everywhere we use TPM 2.0 as our default, the chances of TPM
1.2 being supported by the guest OS are very slim. Just reject
such configurations outright.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
TPM 1.2 is a pretty bad default these days, especially for
architectures which were introduced when TPM 2.0 already existed.
We're already carving out exceptions for several scenarios, but
that's basically backwards: at this point, using TPM 1.2 is the
exception.
Restructure the code so that it reflects reality and we don't
have to remember to update it every time a new architecture is
introduced.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The default-models tests provide coverage for these scenarios
now.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We have a non-trivial amount of architecture-specific logic
dealing with TPM, so it's good to have coverage for it.
Note that two architectures currently don't have support for
TPM devices enabled by default in QEMU: loongarch64 and s390x.
The situation might change for the former, but that's unlikely
to happen for the latter.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This copies the behaviour of the native builds that disable -Werror
on Fedora, since frequently updating toolchains and deps often
introduce new warnings.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The qemuMonitorTestAddErrorResponse() function is a printf-like
function. But the annotation was mistakenly done in .c file
instead of corresponding .h file rendering the annotation
ineffective. Move the annotation to the header file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While __attribute((sentinel)) (exposed by glib under
G_GNUC_NULL_TERMINATED macro) is a gcc extension, it's supported
by clang too. It's already being used throughout our code but
some functions that take variadic arguments and expect NULL at
the end were lacking such annotation. Fill them in.
After this, there are still some functions left untouched because
they expect a different sentinel than NULL. Unfortunately, glib
does not provide macro for different sentinels. We may come up
with our own, but let's save that for future work.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The %meson_test macro expands to have a newline at the start, so
rather than expanding to
VIR_TEST_DEBUG=1 meson test ....
we get
VIR_TEST_DEBUG=1
meson test ....
which has no effect, since VIR_TEST_DEBUG isn't exported.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The udevInterfaceGetXMLDesc method takes a reference on the udev
driver as its first action. If the virCheckFlags() condition
fails, however, this reference is never released.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We never release the reference on the GSource created for
interrupting the main loop, nor do we remove it from the
main context if our thread is woken up prior to the wakeup
callback firing.
This can result in a leak of GSource objects, along with an
ever growing list of GSources attached to the main context,
which will gradually slow down execution of the loop, as
several operations are O(N) for the number of attached GSource
objects.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When attempting to run:
libvirt.git/_build # ./run --selinux ./src/libvirtd
the following error is thrown:
Refusing to change selinux context of file './src/libvirtd' outside build directory
which is obviously wrong. The problem is 'being inside of build
directory' is detected by simple progpath.startswith(builddir).
While builddir is an absolute path, progpath isn't necessarily.
And while looking into the code, I've noticed chcon() function
accessing variable outside its scope when printing out the path
it's working on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When the 'pages' job is configured to run 'on_success' it's skipped if
any other pipeline fails. This is bad in cases such as if an external
service runs out of CI minutes as the web stops being updated.
Since the 'artifacts' of the 'website_job' are generated only if that
phase succeeds this will update the web when the web part is buildable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When hot-plugging a FS device with un-assigned address with a bootindex
the recently-added validation check would fail as validation on hotplug
is done prior to address assignment.
To fix this problem we can simply relax the check to also pass on _NONE
addresses. Unsupported configurations will still be caught as previous
commit re-checks the definition after address assignment prior to
hotplug.
Resolves: https://issues.redhat.com/browse/RHEL-39271
Fixes: 4690058b6d
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Some of the checks make sense only after the address is allocated and
thus we need to re-do the validation after the address is assigned.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
While the function is exported via header, the symbol itself was not.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When removing rotated log files, their name is matched against a
regex (@log_regex) and if they contain '.N' suffix the 'N' is
then parsed into an integer. Well, due to a bug in
virLogCleanerParseFilename() this is not how the code works. If
the suffix isn't found then g_match_info_fetch() returns an empty
string instead of NULL which then makes str2int parsing fail.
Just check for this case before parsing the string.
Based on the original patch sent by David.
Reported-by: David Negreira <david.negreira@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
These are either features/bugfixes I've worked on or
participated in.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
In one of my recent commits, I've introduced
virDomainInterfaceClearQoS() which is a helper that either calls
virNetDevBandwidthClear() ('tc' implementation) or
virNetDevOpenvswitchInterfaceClearQos() (for ovs ifaces). But I
made a micro optimization which leads to a bug: the function
checks whether passed iface has any QoS set and returns early if
it has none. In majority of cases this is right thing to do, but
when removing QoS on virDomainUpdateDeviceFlags() this is
problematic. The new definition (passed as argument to
virDomainInterfaceClearQoS()) contains no QoS (because user
requested its removal) and thus instead of removing the old QoS
setting nothing is done.
Fortunately, the fix is simple - pass olddev which contains the
old QoS setting.
Fixes: 812a146dfe
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The current implementation requires users to configure the
preference as such:
-Dfirewall_backend_default_1=iptables
-Dfirewall_backend_default_2=nftables
In addition to being more verbose than one would hope, there
are several things that could go wrong.
First of all, meson performs no validation on the provided
values, so mistakes will only be caught by the compiler.
Additionally, it's entirely possible to provide nonsensical
combinations, such as repeating the same value twice.
Change things so that the preference can now be configured
as such:
-Dfirewall_backend_priority=iptables,nftables
Checks have been added to prevent invalid values from being
accepted.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Add test data based on qemu commit v9.0.0-995-g60b54b67c6 on x86_64
Comparison to previous release:
Feature additions:
- 9.1 machine type added
- 'SierraForest' cpu type added
- 'SapphireRapids-v3-x86_64-cpu' added
- 'VFIO_MIGRATION' event added (and corresponding 'migration-events'
bool for the device
- 'exit-on-error' argument for 'migrate-incoming' added
- 'sev-guest' gained 'legacy-vm-type' boolean
- cpu topology added 'module' fields
- 'compat-props' argument 'query-machines' added
- 'deprecated-props' argument for 'query-cpu-model-expansion' added
Deprecated removals:
- legacy non-shared-storage migration fully removed (config/stats)
- legacy migration compression fully removed
- RDMA support removed
- dropped 'nios2' field type from 'query-cpus-fast' return data
Note that this dump was done on a newer kernel version which resulted in
the 'pcommit' feature being removed from the few test cases which depend
on the real CPU flag dump.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This can happen only for cmdComplete() in interactive mode (which
I'm still not convinced is any useful for users and whether we
should support it). Anyway, running plain 'complete' command with
no additional arguments boils down to @text being NULL in
vshReadlineParse() which handles the case just right but is then
subsequently passed to vshCompleterFilter() which isn't prepared
for this case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Problem with readline is its API. It's basically a bunch of
global variables with no clear dependencies between them. In this
specific case that I'm seeing: in interactive mode the
cmdComplete() causes instant crash of virsh/virt-admin:
==27999== Invalid write of size 1
==27999== at 0x516EF71: _rl_init_line_state (readline.c:742)
==27999== by 0x5170054: rl_initialize (readline.c:1192)
==27999== by 0x516E5E4: readline (readline.c:379)
==27999== by 0x1B7024: vshReadline (vsh.c:3048)
==27999== by 0x140DCF: main (virsh.c:905)
==27999== Address 0x0 is not stack'd, malloc'd or (recently) free'd
This is because readline keeps a copy of pointer to
rl_line_buffer and the moment cmdComplete() returns and readline
takes over, it accesses the copy which is now a dangling pointer.
To fix this, just keep the original state of rl_line_buffer and
restore it.
Fixes: 41400ac1dd
Fixes: a0e1ada63c
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Our completer callbacks must refrain from printing anything onto
stderr, but unfortunately that's not how service code around
behaves. It may call vshError() and what not. Rather trying to
fix all possible paths (just consider opening a connection), just
close the stderr. We're already closing stdin.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In some cases (e.g. when virt-admin connects to the default URI)
some info message is printed onto stdout (using vshPrintExtra()).
This hurts user experience, just consider:
virt-admin<TAB><TAB>
NOTE\:\ Connecting\ to\ default\ daemon.\ Specify\ daemon\ using\ -c\ \(e.g.\ virtqemud\:///system\)
when no daemon is running. Suppress extra prints by passing '-q'
in the bash-completion script.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We currently require full argument specification:
virt-admin daemon-timeout --timeout X
Well, the '--timeout' feels a bit redundant. Turn the argument
into a positional so that the following works too:
virt-admin daemon-timeout X
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In a few examples we recommend disabling daemon timeout when
fetching debug logs. While it makes sense the actual syntax used
results in an error:
# virt-admin daemon-timeout 0
error: unexpected data '0'
This is because --timeout is required. Update examples to include
it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
iifname/oifname need to lookup the string that contains the name of
the interface each time a packet is checked, while iif/oif compare the
ifindex of the interface, which is included directly in the
packet. Conveniently, the rule is created using the *name* of the
interface (which gets converted to ifindex as the rule is added), so
no extra work is required other than changing the commandline option.
If it was the case that the interface could be deleted and re-added
during the life of the rule, we would have to use Xifname (since
deleting and re-adding the interface would result in ifindex
changing), but for our uses this never happens, so Xif works for us,
and undoubtedly improves performance by at least 0.0000001%.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Commit 91f4ebbac8 (v10.0.0-185-g91f4ebbac8)
changed the return value of virSocketSendFD() from 0 to 1 on success.
Unfortunately in 'virFileOpenForked' the return value was used to report
the error back to the main process from the fork'd child. As process
return codes are positive only, the code negates the value of 'ret' and
reports it. This resulted in the parent thinking the process exited with
failure:
# virsh save avocado-vt-vm1 /mnt/save
error: Failed to save domain 'avocado-vt-vm1' to /mnt/save
error: Error from child process creating '/mnt/save': Unknown error 255
This error reproduces on NFS mounts with 'root_squash' enabled. I've
also observed it in one specific migration case when root_squash NFS is
used with following error:
Failed to open file '/var/lib/libvirt/images/alpine.qcow2': Unknown error 255'
To fix the issue the code is refactored so that it doesn't actually
touch the 'ret' variable needlessly and assigns to it only on failure
cases, which prevents the '1' to be propagated to the parent process as
'255' after negating and storing in the process return code.
Fixes: 91f4ebbac8
Resolves: https://issues.redhat.com/browse/RHEL-36721
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Use contemporary style for declarations and automatic memory clearing
for a helper string.
Since the function can't fail any more, remove any mention of returning
errno and remove error checks from all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Declare one argument per line and one variable per line and use boolean
operators at the end of the line rather than at the beginning.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The iptables backend (which was used as the model for the nftables
backend) used the same "filter" and "nat" tables used by other
services on the system (e.g. firewalld or any other host firewall
management application), so it was possible that one of those other
services would be blocking DNS, DHCP, or TFTP from guests to the host;
we added our own rules at the beginning of the chain to allow this
traffic no matter if someone else rejected it later.
But with nftables, each service uses their own table, and all traffic
must be acepted by all tables no matter what - it's not possible for
us to just insert a higher priority/earlier rule that will override
some reject rule put in by, e.g., firewalld. Instead the firewalld (or
other) table must be setup by that service to allow the traffic. That,
along with the fact that our table is already "accept by default",
makes it possible to eliminate the individual accept rules for DHCP,
DNS, and TFTP. And once those rules are eliminated, there is no longer
any need for the guest_to_host or host_to_guest tables.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Because the chains added by the network driver nftables backend will
go into a table used only by libvirt, we don't need to have "libvirt"
in the chain names. Instead, we can make them more descriptive and
less abrasive (by using lower case, and using full words rather than
abbreviations).
Also (again because nobody else is using the private "libvirt_network"
table) we can directly put our rules into the input ("guest_to_host"),
output ("host_to_guest"), and postrouting ("guest_nat") chains rather
than creating a subordinate chain as done in the iptables backend.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This way when we implement nftables for the nwfilter driver, we can
create a separate table called "libvirt_nwfilter" and everything will
look all symmetrical and stuff.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
It will still be possible to install iptables and use the iptables
backend, but we'll be showing a greater preference for nftables, which
is the proper thing to be doing.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The patch that added the nftables backend for virtual networks left
iptables as the default backend when both nftables and iptables are
installed.
The only functional difference between the two backends is that the
nftables backend doesn't add any rules to fix up the checksum of DHCP
packets, which will cause failures on guests with very old OSes
(e.g. RHEL5) that have a virtio-net network interface using vhost
packet processing (the default), connected to a libvirt virtual
network, and configured to acquire the interface IP using DHCP. Since
RHEL5 has been out of support for several years already, we might as
well start off nftables support right by making it the default.
Distros that aren't quite ready to default to nftables (e.g. maybe
they're rebasing libvirt within a release and don't want to surprise
anyone with an automatic switch from iptables to nftables) can simply
run meson with "-Dfirewall_backend=iptables" during their official
package build.
In the extremely unlikely case that this causes a problem for a user,
they can work around the failure by adding "<driver name='qemu'/> to
the guest <interface> element.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Run all the networkxml2firewall tests twice - once with iptables
backend, and once with the nftables backend.
The results files for the existing iptables tests were previously
named *.args. That has been changed to *.iptables, and the results
files for the new nftables tests are named *.nftables.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Support using nftables to setup the firewall for each virtual network,
rather than iptables. The initial implementation of the nftables
backend creates (almost) exactly the same ruleset as the iptables
backend, determined by running the following commands on a host that
has an active virtual network:
iptables-save >iptables.txt
iptables-restore-translate -f iptables.txt
(and the similar ip6tables-save/ip6tables-restore-translate for an
IPv6 network). Correctness of the new backend was checked by comparing
the output of:
nft list ruleset
when the backend is set to iptables and when it is set to nftables.
This page was used as a guide:
https://wiki.nftables.org/wiki-nftables/index.php/Moving_from_iptables_to_nftables
The only differences between the rules created by the nftables backed
vs. the iptables backend (aside from a few inconsequential changes in
display order of some chains/options) are:
1) When we add nftables rules, rather than adding them in the
system-created "filter" and "nat" tables, we add them in a private
table (ie only we should be using it) created by us called "libvirt"
(the system-created "filter" and "nat" tables can't be used because
adding any rules to those tables directly with nft will cause failure
of any legacy application attempting to use iptables when it tries to
list the iptables rules (e.g. "iptables -S").
(NB: in nftables only a single table is required for both nat and
filter rules - the chains for each are differentiated by specifying
different "hook" locations for the toplevel chain of each)
2) Since the rules that were added to allow tftp/dns/dhcp traffic from
the guests to the host are unnecessary in the context of nftables,
those rules aren't added.
(Longer explanation: In the case of iptables, all rules were in a
single table, and it was always assumed that there would be some
"catch-all" REJECT rule added by "someone else" in the case that a
packet didn't match any specific rules, so libvirt added these
specific rules to ensure that, no matter what other rules were added
by any other subsystem, the guests would still have functional
tftp/dns/dhcp. For nftables though, the rules added by each subsystem
are in a separate table, and in order for traffic to be accepted, it
must be accepted by *all* tables, so just adding the specific rules to
libvirt's table doesn't help anything (as the default for the libvirt
table is ACCEPT anyway) and it just isn't practical/possible for
libvirt to find *all* other tables and add rules in all of them to
make sure the traffic is accepted. libvirt does this for firewalld (it
creates a "libvirt" zone that allows tftp/dns/dhcp, and adds all
virtual network bridges to that zone), however, so in that case no
extra work is required of the sysadmin.)
3) nftables doesn't support the "checksum mangle" rule (or any
equivalent functionality) that we have historically added to our
iptables rules, so the nftables rules we add have nothing related to
checksum mangling.
(NB: The result of (3) is that if you a) have a very old guest (RHEL5
era or earlier) and b) that guest is using a virtio-net network
device, and c) the virtio-net device is using vhost packet processing
(the default) then DHCP on the guest will fail. You can work around
this by adding <driver name='qemu'/> to the <interface> XML for the
guest).
There are certainly much better nftables rulesets that could be used
instead of those implemented here, and everything is in place to make
future changes to the rules that are used simple and free of surprises
(e.g. the rules that are added have coresponding "removal" commands
added to the network status so that we will always remove exactly the
rules that were previously added rather than trying to remove the
rules that "the current build of libvirt would have added" (which will
be incorrect the first time we run a libvirt with a newly modified
ruleset). For this initial implementation though, I wanted the
nftables rules to be as identical to the iptables rules as possible,
just to make it easier to verify that everything is working.
The backend can be manually chosen using the firewall_backend setting
in /etc/libvirt/network.conf. libvirtd/virtnetworkd will read this
setting when it starts; if there is no explicit setting, it will check
for availability of FIREWALL_BACKEND_DEFAULT_1 and then
FIREWALL_BACKEND_DEFAULT_2 (which are set at build time in
meson_options.txt or by adding -Dfirewall_backend_default_n=blah to
the meson commandline), and use the first backend that is available
(ie, that has the necessary programs installed). The standard
meson_options.txt is set to check for nftables first, and then
iptables.
Although it should be very safe to change the default backend from
iptables to nftables, that change is left for a later patch, to show
how the change in default can be undone if someone really needs to do
that.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Daniel P. Berrangé <berrange@redhat.com>
This was the only reason we required the iptables and ebtables
packages at build time, and many other external commands already have
their binaries found at runtime by looking through $PATH (virCommand
automatically does this), so we may as well do it for these commands
as well.
Since we no longer need iptables or iptables at build time, we can
also drop the BuildRequires for them from the rpm specfile.
Inspired-by: 6aa2fa38b0
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In the case that a new version of libvirt is started that uses
different rules to build the network firewall, we need to re-save the
status so that when the network is destroyed (or the *next* time
libvirt is restarted and wants to remove/re-add the firewall), it will
have the proper information to perform the firewall removal.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When destroying a network, the network driver has always assumed that
it knew what firewall rules had been added as the network was
started. This was usually correct - I only recall one time in the past
that the firewall rules added by libvirt were changed. But if the
exact rules used for a network *were* ever changed from one
build/version of libvirt to another, then we would end up attempting
to remove rules that hadn't been added, and could possibly *not*
remove rules that had been added.
The solution to this to not make such brash assumptions about the
past, but instead to save (in the network status object at network
start time) a list of all the rules needed to remove the rules that
were added for the network, and then use that saved list during
network destroy to remove exactly what was previous added.
Beyond making net-destroy more precise, there are other benefits:
1) We can change the details of the rules we add for networks from one
build/release of libvirt to another and painlessly upgrade.
2) The user can switch from one firewall backend to another by simply
changing the setting in network.conf and restarting
libvirtd/virtnetworkd.
In both cases, the restarted libvirtd/virtnetworkd will remove all the
rules that had been previously added (based on the network status),
and then add new rules (saving the new removal commands back into the
network status)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This virFirewall object will store the list of actions required to
remove the firewall that was added for the currently active instance
of the network, so it has been named "fwRemoval" (and when parsed into
XML, the <firewall> element will have the name "fwRemoval").
There are no uses of the fwRemoval object in the virNetworkObj yet,
but everything is in place to add it to the XML when formatted, parse
it from the XML when reading network status, and free the virFirewall
object when the virNetworkObj is freed.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
These functions convert a virFirewall object to/from XML so that it
can be serialized to disk (in a virNetworkObj's status file) and
restored later (e.g. after libvirtd/virtnetworkd is restarted).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virFirewallNewFromRollback() creates a new virFirewall object that
contains a copy of the "rollback" commands from an existing
virFirewall object, but in reverse order. The intent is that this
virFirewall be saved and used later to remove the firewall rules that
were added for a network.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This will be used to label (via "name='blah'") a firewall when it is
formatted to XML and written to the network status.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
So far this will only affect what happens if there is some failure
while applying the firewall rules; the rollback rules aren't yet
persistent beyond that time. More work is needed to remember the
rollback rules while the network is active, and use those rules to
remove the firewall for the network when it is destroyed.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
If the VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK flag is set, each time
an iptables command is executed that is adding a rule or chain, a
corresponding command that will *delete* the same rule/chain is
constructed and added to the list of rollback commands. If we later
want to undo the entire firewall, we can just run those commands.
This isn't yet used anywhere, since
VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK isn't being set.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In the past virFirewall required all rollback commands for a group
(those commands necessary to "undo" any rules that had been added in
that group in case of a later failure) to be manually added by
switching into the virFirewall object into "rollback mode" and then
re-calling the inverse of the exact virFirewallAddCmd*() APIs that had
been called to add the original rules (ie. for each
"iptables --insert" command, for rollback we would need to add a
command with all arguments identical except that "--insert" would be
replaced by "--delete").
Because nftables can't search for rules to remove by comparing all the
arguments (it instead expects *only* a handle that is provided via
stdout when the rule was originally added), we won't be able to follow
the iptables method and manually construct the command to undo any
given nft command by just duplicating all the args of the command
(except the action). Instead we will need to be able to automatically
create a rollback command at the time the rule-adding command is
executed (e.g. an "nft delete rule" command that would include the
rule handle returned in stdout by an "nft add rule" command).
In order to make this happen, we need to be able to 1) learn whether
the user of the virFirewall API desires this behavior (handled by a new
transaction flag called VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK that
can be retrieved with the new virFirewallTransactionGetFlags() API),
and 2) add a new command to the current group's rollback command list (with
the new virFirewallAddRollbackCmd()).
We will actually use this capability in an upcoming patch.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Modify networkSetupPrivateChains() in the network driver to accept a
firewallBackend argument so it will know which backend to call. (right
now it always calls the iptables version of the lower level function,
but in the future it could instead call the nftables version based on
configuration).
But networkSetupPrivateChains() was being called with virOnce(), and
virOnce() doesn't support calling functions that require an argument
(it's based on pthread_once(), which accepts no arguments, so it's not
something we can easily fix in our implementation of virOnce()). To
solve this dilemma, this patch eliminates use of virOnce() by adding a
static lock, and putting all of networkSetupPrivateChains() (including
the setting of "chainInitDone") inside a lock guard - now the places
that used to call it via virOnce() can safely call it directly
instead (adding in the necessary argument to specify backend).
(If it turns out to be significant, we could optimize this by checking
for chainInitDone outside the lock guard, returning immediately if
it's already set, and then moving the setting of chainInitDone up to
the top of the guarded section.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
It still can have only one useful value ("iptables"), but once a 2nd
value is supported, it will be selectable by setting
"firewall_backend=nftables" in /etc/libvirt/network.conf.
If firewall_backend isn't set in network.conf, then libvirt will check
to see if FIREWALL_BACKEND_DEFAULT_1 is available and, if so, set
that. (Since FIREWALL_BACKEND_DEFAULT_1 is currently "iptables", this
means checking to see it the iptables binary is present on the
system). If the default backend isn't available, that is considered a
fatal error (since no networks can be started anyway), so an error is
logged and startup of the network driver fails.
NB: network.conf is itself created from network.conf.in at build time,
and the advertised default setting of firewall_backend (in a commented
out line) is set from the meson_options.txt setting
"firewall_backend_default_1". This way the conf file will have correct
information no matter what ordering is chosen for default backend at
build time (as more backends are added, settings will be added for
"firewall_backend_default_n", and those will be settable in
meson_options.txt and on the meson commandline to change the ordering
of the auto-detection when no backend is set in network.conf).
virNetworkLoadDriverConfig() may look more complicated than necessary,
but as additional backends are added, it will be easier to add checks
for those backends (and to re-order the checks based on builders'
preferences).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This file is generated from network.conf.in because it will soon have
an item that must be modified according to meson buildtime config.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(This paragraph is for historical reference only, described only to
avoid confusion of past use of the name with its new use) In a past
life, virFirewallBackend had been a private static in virfirewall.c
that was set at daemon init time, and used to globally (i.e. for all
drivers in the daemon) determine whether to directly execute iptables
commands, or to run them indirectly via the firewalld passthrough
API. This was removed in commit d566cc55, since we decided that using
the firewalld passthrough API is never appropriate.
Now the same enum, virFirewallBackend, is being reintroduced, with a
different meaning and usage pattern. It will be used to pick between
using nftables commands or iptables commands (in either case directly
handled by libvirt, *not* via firewalld). Additionally, rather than
being a static known only within virfirewall.c and applying to all
firewall commands for all drivers, each virFirewall object will have
its own backend setting, which will be set during virFirewallNew() by
the driver who wants to add a firewall rule.
This will allow the nwfilter and network drivers to each have their
own backend setting, even when they coexist in a single unified
daemon. At least as important as that, it will also allow an instance
of the network driver to remove iptables rules that had been added by
a previous instance, and then add nftables rules for the new instance
(in the case that an admin, or possibly an update, switches the driver
backend from iptables to nftable)
Initially, the enum will only have one usable value -
VIR_FIREWALL_BACKEND_IPTABLES, and that will be hardcoded into all
calls to virFirewallNew(). The other enum value (along with a method
of setting it for each driver) will be added later, when it can be
used (when the nftables backend is in the code).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We know at the time a virFirewallCmd is created (with
virFirewallAddCmd*()) whether or not we will later want to ignore
errors encountered when attempting to apply that command - if
ignoreErrors is set in the AddCmd or if the group has already had
VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS set, then we ignore the errors.
Rather than setting the fwCmd->ignoreErrors only according to the arg
sent to virFirewallAddCmdFull(), and then later (at ApplyCmd-time)
combining that with the group transactionFlags setting (and passing it
all the way down the call chain), just combine the two flags right
away and store this final value in fwCmd->ignoreErrors when the
virFirewallCmd is created (thus avoiding the need to look at anything
other than fwCmd->ignoreErrors at the time the command is applied). Once
that is done, we can simply grab ignoreErrors from the object down in
virFirewallApply() rather than cluttering up the argument list on the
entire call chain.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We will already need a separate function for virFirewallApplyCmd for
iptables vs. nftables, but the only reason for needing a separate
function for virFirewallAddCmd* is that iptables/ebtables need to have
an extra arg added for locking (to prevent multiple iptables commands
from running at the same time). We can just as well add in the
-w/--concurrent during virFirewallApplyCmd, so move the arg-add to
ApplyCmd to keep AddCmd simple.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In normal practice a virFirewallCmd should never have 0 args by the
time it gets to the Apply stage, but at some time while debugging one
of the other patches in this series, exactly that happened (due to a
bug that was since squashed), and having a check for it helped
debugging, so let's permanently check for it.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
I had originally named these as VIR_NETFILTER_* because I assumed the
same enum would eventually be used by our nftables backend as well as
iptables. But it turns out that in most cases it's not possible to
delete an nftables rule, so we just never used the enum anyway, so
this patch is renaming the values to IPTABLES_ACTION_*, and taking
advantage of the newly defined (via VIR_ENUM_DECL/IMPL)
iptablesActionTypeToString() to replace all the ternary operators used
to translate the enum into a string for the iptables commandline with
iptablesActionTypeToString().
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
These objects aren't rules, they are commands that are executed that
may create a firewall rule, delete a firewall rule, or simply list the
existing firewall rules. It's confusing for the objects to be called
"Rule" (especially in the case of the function
virFirewallRemoveRule(), which doesn't remove a rule from the
firewall, it takes one of the objects out of the list of commands to
execute! In order to remove a rule from the host's firewall, you have
to Add a "rule" (now "cmd" aka command) to the list that will, when
applied/run, remove a rule from the host firewall.)
Changing the name to virFirewallCmd makes it all much less confusing.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now that the toplevel iptables functions have been moved out of the
linux bridge driver into network_iptables.c, all of the utility
functions are used only within that same file, so simplify it.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Although initially we will add exactly the same rules for the nftables
backend, the two may (hopefully) soon diverge as we take advantage of
nftables features that weren't available in iptables. When we do that,
there will need to be a different version of these functions (currently in
bridge_driver_linux.c) for each backend:
networkAddFirewallRules()
networkRemoveFirewallRules()
networkSetupPrivateChains()
Although it will mean duplicating some amount of code (with just the
function names changed) for the nftables backend, this patch moves all
of the rule-related code in the above three functions into iptables*()
functions in network_iptables.c, and changes the functions in
bridge_driver_linux.c to call the iptables*() functions. When we make
a different backend, it will only need to make equivalents of those 3
functions publicly available to the upper layer.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
These functions are only ever used by the network driver, and are so
specific to the network driver's usage of iptables that they likely
won't ever be used elsewhere. The files are renamed to
network_iptables.[ch] to be more in line with driver-specific file
naming conventions.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The virStateDriver struct has .stateInitialize callback which is
declared to return virDrvStateInitResult enum. But some drivers
return a plain int in their implementation which is UB.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
For the links of drvinterface, drvnetwork, drvnwfilter, and Nagios-virt,
there are no alternative docs. Just remove them directly.
Signed-off-by: Han Han <hhan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There was no test for this and we mistakenly used 'B' rather than 'T'
when constructing the json value for this parameter. Thus, a value of
'off' was VIR_TRISTATE_SWITCH_OFF=2, which was translated to a boolean
value of 'true'.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Unlike other input types, evdev is not a true device since it's backed by
'-object'. We must use object-add/object-del monitor commands instead of
device-add/device-del in this particular case.
This patch adds support for handling live attachment and
detachment of evdev type devices.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/529
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As a general rule, we use defines for features that can only be
enabled on a subset of the platforms that we target, and we
don't offer fine-grained control over every single possible
meson configuration knob at the RPM level.
In the case of ssh-proxy, we are enabling it everywhere already,
so having a define for it is unnecessary.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The ssh-proxy feature works independently of the clients,
just like the NSS plugin does.
Moreover, ssh-proxy only works for local VMs, while clients
are routinely used to manage remote hypervisors.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The refactor of the libvirt tools command parser introduced a bug where
the '--help' option would cause an error:
$ virsh list --help
error: command 'list' doesn't support option --help
rather than printing the help for the command as the help option is
supposed to be handled separately from the real options.
Re-introduce the separate handling to the new parser code.
Fixes: 5540c3d241
Resolves: https://issues.redhat.com/browse/RHEL-36565
Reported-by: Lili Zhu <lizhu@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
I've noticed some tests fail to run under valgrind with the
following error:
$ valgrind --leak-check=full --trace-children=yes ./qemuxmlconftest
valgrind: symbol lookup error: libvirt.git/_build/tests/libdomaincapsmock.so: undefined symbol: virQEMUCapsGet
But without valgrind the test passes just fine. While we usually
don't want to change our code just to adhere to random tools, in
this case we ought to make an exception because valgrind helps us
to detect memory leaks.
NB, the --trace-children=yes is needed whenever a test
re-executes itself, i.e. when it uses mocks. Otherwise we'd just
get (boring) result for the first invocation of main() which does
nothing more than sets up the environment and calls exec().
When running the test binary without valgrind I can see the
libtest_qemu_driver.so being loaded even after exec:
$ LD_DEBUG=libs ./qemuxmlconftest 2>&1 | grep -e libtest_qemu_driver.so -e virQEMUCapsGet
6439: find library=libtest_qemu_driver.so [0]; searching
6439: trying file=libvirt.git/_build/tests/../src/libtest_qemu_driver.so
6439: trying file=libvirt.git/_build/tests/glibc-hwcaps/x86-64-v3/libtest_qemu_driver.so
6439: trying file=libvirt.git/_build/tests/glibc-hwcaps/x86-64-v2/libtest_qemu_driver.so
6439: trying file=libvirt.git/_build/tests/libtest_qemu_driver.so
6439: calling init: libvirt.git/_build/tests/libtest_qemu_driver.so
6439: find library=libtest_qemu_driver.so [0]; searching
6439: trying file=libvirt.git/_build/tests/libtest_qemu_driver.so
6439: calling init: libvirt.git/_build/tests/libtest_qemu_driver.so
6439: calling fini: libvirt.git/_build/tests/libtest_qemu_driver.so [0]
But running the same under valgrind:
$ LD_DEBUG=libs valgrind --leak-check=full --trace-children=yes ./qemuxmlconftest 2>&1 | grep -e libtest_qemu_driver.so -e virQEMUCapsGet
6515: find library=libtest_qemu_driver.so [0]; searching
6515: trying file=libvirt.git/_build/tests/../src/libtest_qemu_driver.so
6515: trying file=libvirt.git/_build/tests/glibc-hwcaps/x86-64-v3/libtest_qemu_driver.so
6515: trying file=libvirt.git/_build/tests/glibc-hwcaps/x86-64-v2/libtest_qemu_driver.so
6515: trying file=libvirt.git/_build/tests/libtest_qemu_driver.so
6515: calling init: libvirt.git/_build/tests/libtest_qemu_driver.so
6515: libvirt.git/_build/tests/libdomaincapsmock.so: error: symbol lookup error: undefined symbol: virQEMUCapsGet (fatal)
valgrind: symbol lookup error: libvirt.git/_build/tests/libdomaincapsmock.so: undefined symbol: virQEMUCapsGet
To me, it looks like valgrind forced linker to lookup symbols
"sooner", as individual libraries are loaded. But I must admit I
have no idea how valgrind does that (or if that's even valgrind's
'fault').
But fix is pretty simple: link mocks that rely on symbols from
the QEMU driver with the QEMU driver, well, its test suite
suitable version (libtest_qemu_driver.so).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The message that's thrown at users when they try to open a pull
request on github suggests opening the MR on gitlab instead.
While this works for other libvirt subprojects, for the main
libvirt.git we still use e-mail workflow. Update the message to
reflect this fact.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Add a missing option for the test to prove that we parse/format this
option.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Adds documentation for the <snapshotDeleteInProgress/> element to
the libvirt snapshot format XML reference. The <snapshotDeleteInProgress/>
element, introduced at commit 565bcb5d79, ensures the consistency of qcow2
images during snapshot deletion operations by marking disks in snapshot
metadata as invalid until deletion is successfully completed.
The commit was merged but the related documentation was missing.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/609
Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
It's currently running against AlmaLinux 8 which went out of
support.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's currently running against AlmaLinux 8 which went out of
support.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Ubuntu 24.04 was released recently. Add it to our CI. Also, to be
able to run ASAN/UBSAN builds on Ubuntu 24.04 libclang-rt-dev
needs to be installed (because clang's runtime was moved into a
separate package). Hence so many seemingly unrelated changes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that we don't have any distro stuck with glib-2.56.0, we can
bump the glib version. In fact, this is needed, because of
g_clear_pointer. Since v7.4.0-rc1~301 we declare at compile time
what version of glib APIs we want to use (by setting
GLIB_VERSION_MIN_REQUIRED = GLIB_VERSION_MAX_ALLOWED = 2.56.0),
regardless of actual glib version in the host.
And since we currently require glib-2.56.0 and force glib to use
APIs of that version, some newer bits are slipping from us. For
instance: regular function version of g_clear_pointer() is used
instead of a fancy macro. So what? Well, g_clear_pointer()
function typecasts passed free function to void (*)(void *) and
then calls it. Well, this triggers UBSAN, understandably. But
with glib-2.58.0 the g_clear_pointer() becomes a macro which
calls the free function directly, with no typecasting and thus no
undefined behavior.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's now more than two years since Ubuntu 22.04 was released and
per our support policy, Ubuntu 20.04 (the previous major release)
is now not supported. Remove it from our CI testing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since Fedora 40 was released recently, Fedora 38 is now
unsupported. Drop Fedora 38 and introduce Fedora 40 to our CI.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
By the time of release, it's going to be more than two years
since AlmaLinux 9 was released and per our support policy,
AlmaLinux 8 (the previous major release) will be not supported.
Switch from AlmaLinux 8 to AlmaLinux 9.
This also means the website_job which depends on AlmaLinux 8
needs to be moved to newer AlmaLinux.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Strictly speaking, xdrproc_t is declared as following:
typedef bool_t (*xdrproc_t)(XDR *, ...);
But our rpcgen generates properly typed functions, e.g.:
bool_t xdr_virNetMessageError(XDR *xdrs, virNetMessageError *objp)
Now, these functions of ours are passed around as callbacks (via
an argument of xdrproc_t type), for instance in
virNetMessageEncodePayload(). But these two types are strictly
different. We silence the compiler by typecasting the callbacks
when passing them, but strictly speaking - calling such callback
later, when a function of xdrproc_t is expected is an undefined
behavior.
Ideally, we would fix our rpcgen to generate proper function
headers, but: a) my brain is too small to do that, and b) we
would lose compiler protection if an xdr_*() function is called
directly but argument of a wrong type is passed.
Silence UBSAN for now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The .probe member of virSecurityDriver struct is declared to
return virSecurityDriverStatus enum. But there are two instances
(AppArmorSecurityManagerProbe() and
virSecuritySELinuxDriverProbe()) where callbacks are defined to
return an integer. This is an undefined behavior because integer
has strictly bigger space of possible values than the enum.
Defined those aforementioned callbacks so that they return the
correct enum instead of int.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Allocated in testQemuInfoSetArgs(), the vdpafds member of
testQemuArgs is never freed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Previously, the network device hotplug logic would try to ensure only CCW or
PCI addresses. With recent support for the usb-net model, this patch will
ensure USB addresses for usb-net network devices.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/14
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It can be safely removed from the VMX, VMWare will still boot the
machine and once another ethernet is added it is updated in the VMX to
zero. So do not require it and default to zero too since this part of
the XML is done as best effort and it is mentioned even in our
documentation.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'virQEMUCapsSearchData' has been unused since
commit bc33b8c639 ("qemu: capabilities: Drop the
virQEMUCapsCacheLookupByArch function")
Remove it.
Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
An incorrect check for domainRegister caused the DNS server for a
virtual domain to be registered with systemd-resolved even if
register='no' attribute was present. Only omitting the attribute
completely would disable the registration.
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The 'check-html-references' test will process the built HTML files,
so they must exist before it is run, along with any images that
they point to.
If using the older 'configure_file' command, no changes are needed
since that always gets executed at 'meson setup' time, rather than
at 'meson compile' time.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The nodedev code unhelpfully reports
couldn't convert node device def to mdevctl JSON
which hides the actual error message
No JSON parser implementation is available
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The mdev code requires YAJL in order to convert from node dev XML to
mdev's config format.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The libxlmock.c conditionalizes on WITH_YAJL, but this mock is
used from other tests which only conditionalize on WITH_LIBXL.
The libxl code does not have any dependancy on YAJL, so the
bogus condition can be removed from the mock and also from
libxlxml2domconfigtest.c
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The securityselinuxhelper build is conditionalized on the SELinux
security driver feature. It is also needed, however, by viridentitytest
whenever libselinux is present.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The 'virsh-auth' test is mistakenly conditionalized on the libvirtd
daemon build, however, it just uses the 'test:///default' driver
URI, so does not require a daemon.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virdrivermoduletest will attempt to dlopen() each driver module,
so they must be build before the test can run.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The 'virsh-auth' test needs to be able to invoke the 'virsh' binary
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We fail to express an ordering between the custom target that
generates the combined augeas test input file, and the meson
test command.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Several meson options cannot be enabled, without first enabling another
option. This adds a small comment prior to an option to record its
mandatory dependencies.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Various tests try to open a connection to 'test:///default' and
must be skipped when the test driver is disabled.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This skips building tests which rely on tirpc when it is not
present.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When testPipeFeeder copies the XML document into the padded buffer, it
tells virStrcpy that 'xmlsize' bytes are available. This is under
reporting size by 1 byte, and as a result it fails to copy the trailing
'\n' replacing it with '\0'. The return value of virStrcpy wasn't
checked, but was reporting this truncation.
When testPipeFeeder then sends the padded buffer down the pipe, it asks
to send 'emptyspace + xmlsize + 1' bytes, which means it sends the data,
as well as the trailing '\0' terminator.
Both bugs combined mean it is sending '\0\0' as the last bytes, instead
of '\n' which was intended. When virFileReadAll reads data from the
pipe, it ends up adding another '\0' resulting in a very NUL terminated
string ('\0\0\0'). This is all harmless, but should be fixed regardless.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virshtest program testPipeFeeder method is doing this:
mkfifo("test.fifo", 0600) ;
int fd = open("test.fifo", O_RDWR);
char buf[...];
memset(buf, 'a', sizeof(buf));
write(fd, buf, sizeof(buf)) == sizeof(buf));
close(fd);
while the the 'virsh' child process then ends up doing:
fd = open("test.fifo", O_RDONLY);
read(fd, buf, sizeof(buf)) == sizeof(buf));
close(fd);
The 'virsh' code hangs on open() on at least ppc64 and some other
arches. It can be provoked to hang even on x86 by reducing the size of
the buffer. It can be prevented from hanging on ppc64 by increasing the
size of the buffer.
What is happening is a result of differing page sizes, altering the
overall pipe capacity size, since pipes on linux default to 16 pages
in size and thus have architecture specific capacity when measured
in bytes.
* On x86, testPipeFeeder opens R+W, tries to write 140kb and
write() blocks because the pipe is full. This gives time for
virsh to start up, and it can open the pipe for O_RDONLY
since testPipeFeeder still has it open for write. Everything
works as intended.
* On ppc64, testPipeFeeder opens R+W, tries to write 140kb
and write() succeeds because the larger 64kb page size
resulted in greater buffer capacity for the pipe. It thus
quickly closes the pipe, removing the writer, and triggering
discard of all the unread data. Now virsh starts up, tries
to open the pipe for O_RDONLY and blocks waiting for a new
writer to open it, which will never happen. Meson kills
the test after 30 seconds.
NB, every now & then, it will not block because virsh starts
up quickly enough that testPipeFeeder has not yet closed the
write end of the pipe, giving the illusion of correctness.
The key flaw here is that it should not have been using O_RDWR
in testPipeFeeder. Synchronization is required such that both
virsh and testPipeFeeder have their respective ends of the pipe
open before any data is sent. This is trivially arranged by
using O_WRONLY in testPipeFeeder.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In case when the interface is being detached/reattached it may happen
that udev will return NULL from 'udev_device_get_sysname()'.
As the RPC code requires nonnull strings in the return array it fails to
serialize such reply:
libvirt: XML-RPC error : Unable to encode message payload
Fix this by simply ignoring such interfaces as there's nothing we can
report in such case.
A similar fix was done to 'udevConnectListAllInterfaces' in commit
2ca94317ac.
Resolves: https://issues.redhat.com/browse/RHEL-34615
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Make the array-filling operation of udevListInterfacesByStatus optional
and replace the completely redundant udevNumOfInterfacesByStatus by it.
Further patches fixing the listing will not need to be duplicated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Some public objects (like virDomain, virInterface, and so on) are
missing g_autoptr() cleanup functions. Provide missing
declarations. Note, this is only for our internal use - hence
datatypes.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When starting a domain and there's no vCPU/emulator pinning set,
we query the list of all online physical CPUs and set affinity of
the child process (which eventually becomes QEMU) to that list.
We can't assume libvirtd itself had affinity to all online CPUs
and since affinity of the child process is inherited, we should
fix it afterwards. But that's not necessarily correct. Users
might isolate some physical CPUs and we should avoid touching
them unless explicitly told so (i.e. vCPU/emulator pinning told
us so).
Therefore, when attempting to set affinity to all online CPUs
subtract the isolated ones.
Before this commit:
root@localhost:~# cat /sys/devices/system/cpu/isolated
19,21,23
root@virtlab414:~# taskset -cp $(pgrep qemu)
pid 14835's current affinity list: 0-23
After:
root@virtlab414:~# taskset -cp $(pgrep qemu)
pid 17153's current affinity list: 0-18,20,22
Resolves: https://issues.redhat.com/browse/RHEL-33082
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This is a helper that parses /sys/devices/system/cpu/isolated
into a virBitmap. It's going to be needed soon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Some sysfs files contain either string representation of a bitmap
or just a newline character. An example of such file is:
/sys/devices/system/cpu/isolated. Our current implementation of
virFileReadValueBitmap() fails in the latter case, unfortunately.
Introduce a slightly modified version that accepts empty files.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Some sysfs files contain either string representation of a bitmap
or just a newline character. An example of such file is:
/sys/devices/system/cpu/isolated. Our current implementation of
virBitmapParseUnlimited() fails in the latter case,
unfortunately. Introduce a slightly modified version that accepts
empty files.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Recent rework of virshtest uncovered a subtle bug that was
dormant in now vsh but before that even in monolithic virsh.
In vsh.c there's this vshReadlineInit() function that's supposed
to initialize readline library, i.e. set those global rl_*
pointers. But it also initializes history library. Then, when
virsh/virt-admin quits, vshReadlineDeinit() is called which
writes history into a file (ensuring the parent directory
exists). So far no problem.
Problem arises when cmdComplete() is called (from a bash
completer, for instance). It does not guard call to
vshReadlineInit() with check for interactive shell (and it should
not), but it sets ctl->historyfile which signals to
vshReadlineDeinit() the history should be written.
Now, no real history is written, because nothing was entered on
the stdin, but the parent directory is created nevertheless. With
recent movement in virshtest.c this means some test cases might
create virsh history file which breaks our promise of not
touching user's data in test suite.
Resolves: https://bugs.gentoo.org/931109
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Return value of a function 'virDomainChrDefNew' is dereferenced
at hyperv_driver.c without checking for NULL, which can lead to
NULL dereference immediately after.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
Reviewed-by: Kristína Hanicová <khanicov@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Similar to commit 57d084febe, another case of the libxl driver not
adapting to modular daemons. When converting configuration that
contains a type='network' interface, the converter calls
virNetworkLookupByName, passing the hypervisor connection object
instead of a connection to virtnetworkd. E.g.
> cat dom.xml
...
<interface type='network'>
<source network='default'/>
</interface>
...
> virsh net-info default
Name: default
UUID: 25a5b089-1e71-4956-99aa-df2213bbb407
Active: yes
Persistent: no
Autostart: no
Bridge: virbr0
> virsh domxml-to-native xen-xl dom.xml
error: Network not found: default
Acquire a connection to virtnetworkd and use it when calling
virNetwork* APIs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The mpx feature was removed from the corresponding qemu cpu models.
With mpx in the libvirt cpu models, libvirt believes the feature
to be implicitly enabled when creating qemu VMs, while in fact it is
disabled.
This became an issue when commit 94eacd5a5f introduced new vmx-*
features, of which some are dependent on mpx (see "feature_dependencies"
table in qemu target/i386/cpu.c), e.g. vmx-exit-clear-bndcfgs and
vmx-entry-load-bndcfgs. These features cannot be enabled by qemu
without also mpx being enabled, leading to the error message
error: Failed to create domain from testdomain.xml
error: operation failed: guest CPU doesn't match
specification: missing features: mpx,vmx-exit-clear-bndcfgs,
vmx-entry-load-bndcfgs
when trying to create a VM with a "host-model" cpu on a host that
does support mpx and the mentioned vmx-* features:
<domain>
...
<cpu mode='host-model' check='full' />
...
</domain>
Resolve the issue by removing mpx from libvirt's cpu models as well.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Features removed from a CPU model are marked with "removed='yes'"
attribute in the CPU map. Such features will always be present in a CPU
definition produced by libvirt regardless on their state. In other words
a running domain (even saved in a file) will always explicitly contain
states of all features removed from the specified CPU model. This
enables migration to older libvirt which would otherwise think the
affected features should be enabled as they are still included in the
CPU model in the older version of CPU map. Migration from an old libvirt
to a new one would be broken as the new libvirt would think the removed
features should be disabled (because they are not included in the CPU
model anymore), which might not be the case on the source host. Thus we
were refusing to remove CPU features unless they were never working and
no domain could even be running with those features enabled.
This patch removes the limitation. When handling CPU definitions with
missing features marked as removed in the specified CPU model, we know
whether it comes from a running domain, in which case it must have been
created by older libvirt where the missing CPU features were not removed
yet. This means the features must have been enabled on the source and we
can automatically fix the definition by adding the missing features with
correct states.
We can safely remove any CPU feature from our CPU models now, but it
should only be used for features removed from all versions of a given
CPU model in QEMU because unversioned models correspond to v1.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virCPUUpdate check the CPU definition for features that were marked as
removed in the specified CPU model and explicitly adds those that were
not mentioned in the definition. So far such features were added with
VIR_CPU_FEATURE_DISABLE policy, but the caller may want to use a
different policy in some situations, which is now possible via the
removedPolicy parameter.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virCPUDefAddFeatureInternal helper function only fails if it is
called with VIR_CPU_ADD_FEATURE_MODE_EXCLUSIVE, which is only used in
virCPUDefAddFeature. The other callers (virCPUDefUpdateFeature and
virCPUDefAddFeatureIfMissing) will never get anything but 0 from
virCPUDefAddFeatureInternal and their return type can be changed to
void.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When using vSPC (Virtual Serial Port Concentrator) in vSphere the actual
address for it is saved in serialX.vspc in which case the
serialX.fileName is most probably something we can't get any useful
information from and we also fail during the parsing rendering any
dumpxml and similar tries unsuccessful.
Instead of parsing the vspc URL with something along the lines of
`virURIParse(vspc ? vspc : fileName)`, which could lead to us reporting
information that is very prune to misuse (the vSPC seemingly has a
protocol on top of the telnet connection; redefining the domain would
change the behaviour; the URL might have a fragment we are not saving;
etc.) or adding more XML knobs to indicate vSPC usage (which we would
not be able to configure; we'd have to properly error out everywhere;
etc.) let's just report dummy serial port that leads to nowhere (i.e.
type="null").
Resolves: https://issues.redhat.com/browse/RHEL-32182
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Users are seeing periodic segfaults from libvirt client apps,
especially thread heavy ones like virt-manager. A typical
stack trace would end up in the virNetClientIOEventFD method,
with illegal access to stale stack data. eg
==238721==ERROR: AddressSanitizer: stack-use-after-return on address 0x75cd18709788 at pc 0x75cd3111f907 bp 0x75cd181ff550 sp 0x75cd181ff548
WRITE of size 4 at 0x75cd18709788 thread T11
#0 0x75cd3111f906 in virNetClientIOEventFD /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:1634:15
#1 0x75cd3210d198 (/usr/lib/libglib-2.0.so.0+0x5a198) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2)
#2 0x75cd3216c3be (/usr/lib/libglib-2.0.so.0+0xb93be) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2)
#3 0x75cd3210ddc6 in g_main_loop_run (/usr/lib/libglib-2.0.so.0+0x5adc6) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2)
#4 0x75cd3111a47c in virNetClientIOEventLoop /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:1722:9
#5 0x75cd3111a47c in virNetClientIO /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2002:10
#6 0x75cd3111a47c in virNetClientSendInternal /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2170:11
#7 0x75cd311198a8 in virNetClientSendWithReply /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2198:11
#8 0x75cd31111653 in virNetClientProgramCall /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclientprogram.c:318:9
#9 0x75cd31241c8f in callFull /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/remote/remote_driver.c:6054:10
#10 0x75cd31241c8f in call /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/remote/remote_driver.c:6076:12
#11 0x75cd31241c8f in remoteNetworkGetXMLDesc /usr/src/debug/libvirt/libvirt-10.2.0/build/src/remote/remote_client_bodies.h:5959:9
#12 0x75cd31410ff7 in virNetworkGetXMLDesc /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/libvirt-network.c:952:15
The root cause is a bad assumption in the virNetClientIOEventLoop
method. This method is run by whichever thread currently owns the
buck, and is responsible for handling I/O. Inside a for(;;) loop,
this method creates a temporary GSource, adds it to the event loop
and runs g_main_loop_run(). When I/O is ready, the GSource callback
(virNetClientIOEventFD) will fire and call g_main_loop_quit(), and
return G_SOURCE_REMOVE which results in the temporary GSource being
destroyed. A g_autoptr() will then remove the last reference.
What was overlooked, is that a second thread can come along and
while it can't enter virNetClientIOEventLoop, it will register an
idle source that uses virNetClientIOWakeup to interrupt the
original thread's 'g_main_loop_run' call. When this happens the
virNetClientIOEventFD callback never runs, and so the temporary
GSource is not destroyed. The g_autoptr() will remove a reference,
but by virtue of still being attached to the event context, there
is an extra reference held causing GSource to be leaked. The
next time 'g_main_loop_run' is called, the original GSource will
trigger its callback, and access data that was allocated on the
stack by the previous thread, and likely SEGV.
To solve this, the thread calling 'g_main_loop_run' must call
g_source_destroy, immediately upon return, to guarantee that
the temporary GSource is removed.
CVE-2024-4418
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Martin Shirokov <shirokovmartin@gmail.com>
Tested-by: Martin Shirokov <shirokovmartin@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Allow generation of command line for virtio-sound-pci and virtio-sound-device
devices along with additional virtio options.
A new testcase is added to test virtio-sound-pci. The
arm-vexpressa9-virtio testcase is also extended to test virtio-sound-device.
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This patch adds parsing of the virtio sound model, along with parsing
of virtio options and PCI/virtio-mmio address assignment.
A new 'streams' attribute is added for configuring number of PCM streams
(default is 2) in virtio sound devices. QEMU additionally has jacks and chmaps
parameters but these are currently stubbed, hence they are excluded in this
patch series.
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This drops the CentOS 8 Stream distro target, since that is going EOL
at the end of May, at which point it will cease to be installable
due to package repos being archived.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This brings in a fix to the job rules which solves a problem with
jobs getting skipped in merge requests in some scenarios. It also
changes the way Cirrus CI vars are set, which involves a weak to
the way $PATH is set in build.yml.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The capability can be used to detect if the qemu binary already
supports 'ras' feature for 'virt' machine type.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Refactor the existing logic using two nested loops with a jump into the
middle of both with 3 separate places fetching next token to a single
loop using a state machine with one centralized place to fetch next
tokens and add explanation comments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As we now have a centralized point to assign values to options move the
debugging logic there.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This check was needed due to the use "unsigned long long" as bitmap
which was refactored recently.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Refactor the very old opaque logic (using multiple bitmaps) by
fully-allocating vshCmdOpt for each possible argument and then filling
them as they go rather than allocating them each time after it's parsed.
This simplifies the checkers and removes the need to cross-reference
multiple arrays.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Neither of them is used outside of vsh.c. 'vshCmddefSearch' needed to be
rearranged as it was called earlier in vsh.c than it was defined.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Remove the old helpers which were used previously to pick which field to
complete.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In preparation for internal parser refactor introduce new accessors for
the VSH_OT_ARGV type which will return a NULL-terminated string list or
even a concatenated string for the given argument.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Currently the code decides which option to complete by looking into the
input string and trying to infer it based on whether we are at the
end position as we truncate the string to complete to the current cursor
position.
That basically means that only the last-parsed option will be up for
completion.
Replace the logic by remembering which is the last option rather than
using two different position checks and base the completion decision on
that and the actual value of the last argument (see comment).
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add both single invocations as well as a script containing the same
commands.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The argument will be used for testing the command/option completer
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
While the 'complete' command is meant to be hidden and used only for
the completion script, there's nothing preventing it being used in all
virsh modes.
This poses a problem as the command tries to close 'stdin' to avoid the
possibility that an auth callback would want to read the password.
In interactive mode this immediately terminates virsh and in
non-interactive mode it attempts to close it multiple times if you use
virsh in batch mode.
Fix the issues by using virOnce() to close it exactly once and do so
only in non-interactive mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Shorten the function name as there isn't any vshCommandOptString.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
'vshReadlineInit' is called when interactive virsh is started but also
on each call to 'cmdComplete'. Calling it repeatedly (using the
'complete' command interactively, or multiple times in batch mode) leaks
the buffers for history file configuration.
Avoid multiple setups of this function by returning success in case the
history file config is already present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The buffer which we assign to the 'rl_line_buffer' variable of readline
would be overwritten and thus leaked on multiple invocations of
cmdComplete in one session.
Free/clear it after it's used.
Hitting this leak was until recenly possible only in non-interactive
batch mode and recently also in interactive mode as 'complete' can be
used multiple times now interactively.
Fixes: a0e1ada63c
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The "quantum" attribute of HTB is documented as:
Number of bytes to serve from this class before the scheduler
moves to the next class.
Since v1.3.2-rc1~225 we compute what we think is the appropriate
value and pass it on the TC command line. But kernel and
subsequently TC use uint32_t to store this value. If we compute
value outside of this type then TC fails and prints usage which
we then interpret as an error message. Needlessly long error
message. While there's not much we can do about the latter, we
can put a cap on the value and stop tickling this behavior of TC.
Fixes: 065054daa7
Resolves: https://issues.redhat.com/browse/RHEL-34112
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit v10.0.0-265-ge67bca23e4 added a `active_config` and
`defined_config` to nodedev mdev internal XML handling.
`defined_config` can be filled at XML parse time, but `active_config`
must be filled in by nodedev driver. This wasn't implemented for the
test driver however, which caused virt-manager test suite regressions.
Working example:
```
$ virsh --connect test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
<device>
<name>mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110</name>
<path>/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110</path>
<parent>css_0_0_0023</parent>
<capability type='mdev'>
<type id='vfio_ccw-io'/>
<iommuGroup number='0'/>
</capability>
</device>
```
Broken example:
```
$ virsh --connect test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
<device>
<name>mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110</name>
<path>/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110</path>
<parent>css_0_0_0023</parent>
<capability type='mdev'>
<iommuGroup number='0'/>
</capability>
</device>
```
There's already code that does what we want in the test suite.
Move it to a shared function, and call it in test driver when
creating a nodedev from driver startup XML.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
- Error if INACTIVE requested for transient object
- Force dumping INACTIVE XML when object is inactive
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This was the implied default before nodedevs gained a notion of
being inactive and transient. It also matches the implied default
when parsing other object types
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Rework 'virDomainUSBDeviceDefForeach' to use virDomainDeviceInfoIterate
instead of open-coding all iterators. To achieve this
'virDomainDeviceIsUSB' needs to be fixed as it didn't properly handle
'sound', 'fs', 'chr', 'ccid', and 'net' usb devices.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Since libvirt now tries to interpret network device models (unless an
unknow model is used) the documentation didn't make a good job
specifying what is supported.
Rewrite the docs to explicitly list the models which we do parse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
qemu-9.0 was released so update the capability dump to the final
version.
Notable changes:
- the 'vdpa' simulator support was reverted for now
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Prior to commit eac646ea49 VIR_TEST_MOCK included the path to the
build directory, but the code was not fixed after VIR_TEST_MOCK was
changed resulting in the following failure when attempting to probe
capaibilities:
$ ./tests/qemucapsprobe /path/to/qemu/qemu-system-x86_64 > out
libqemucapsprobemock.so: No such file or directory
Fix the construction of the path to the mock library by concatenating it
back with the absolute path to the build directory.
Fixes: eac646ea49
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
While QEMU accepts and interprets an empty string in the tls-hostname
field in migration parametes as if it's unset, the same does not apply
for the 'tls-hostname' field when 'blockdev-add'-ing a NBD backend for
non-shared storage migration.
When libvirt sets up migation with TLS in 'qemuMigrationParamsEnableTLS'
the QEMU_MIGRATION_PARAM_TLS_HOSTNAME migration parameter will be set to
empty string in case when the 'hostname' argument is passed as NULL.
Later on when setting up the NBD connections for non-shared storage
migration 'qemuMigrationParamsGetTLSHostname', which fetches the value
of the aforementioned TLS parameter.
This bug was mostly latent until recently as libvirt used
MIGRATION_DEST_CONNECT_HOST mode in most cases which required the
hostname to be passed, thus the parameter was set properly.
This changed with 8d693d79c4 for post-copy migration, where libvirt now
instructs qemu to connect and thus passes NULL hostname to
qemuMigrationParamsEnableTLS, which in turn causes libvirt to try to
add NBD connection with empty string as tls-hostname resulting in:
error: internal error: unable to execute QEMU command 'blockdev-add': Certificate does not match the hostname
To address this modify 'qemuMigrationParamsGetTLSHostname' to undo the
weird semantics the migration code uses to handle TLS hostname and make
it return NULL if the hostname is an empty string.
Fixes: e8fa09d66b
Resolves: https://issues.redhat.com/browse/RHEL-32880
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Since OVS keeps desired state in a DB, upon sudden crash of the
host we may leave a port behind. There's no problem on VM
shutdown or NIC hotunplug as we call corresponding del-port
function (virNetDevOpenvswitchRemovePort()). But if the host
suddenly crashes we won't ever do that. What happens next, is
when OVS starts it finds desired state in its DB and creates a
stale port.
OVS added support for transient ports in v2.5.0 (Feb 2016) and
since its v2.9.0 it even installs a systemd service
(ovs-delete-transient-ports) that automatically deletes transient
ports on system startup. If we mark a port as transient then OVS
won't restore its state on restart after crash.
This change may render "--may-exist" argument redundant, but I'm
not sure about all the implications if it was removed. Let's keep
it for now.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/615
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
The CCW variant of the 'vhost-user-fs' device in qemu doesn't
deliberately support the 'bootindex' attribute as the machine is unable
to boot from such device.
Reject '<boot order' on non-PCI virtiofs, add tests validating that it's
rejected as well as that virtiofs on PCI-based hosts but without address
specified will be accepted.
Resolves: https://issues.redhat.com/browse/RHEL-22728
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Replace symlink by a real output file so that we can also test updates
to input file.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Only two controller models allow setting mem-reserve:
pcie-root-port and pci-bridge. Reflect this fact during
validation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
There are PCI devices with pretty large non-prefetchable memory,
for instance:
Memory at 9d800000 (64-bit, non-prefetchable) [size=8M]
Memory at a6800000 (64-bit, non-prefetchable) [size=16K]
For cold plugged devices this is not a problem, because firmware
sets PCI controllers in a way that make devices behind them just
work. Problem arises if such PCI device is to be hot plugged.
Since the PCI device wasn't present at cold boot, firmware could
not take it into calculations and the amount of reserved memory
is not sufficient.
Introduce a know that allows users overriding value computed by
FW and thus allow hot plug of such PCI devices.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
The purpose of ERROR() macro in our NSS module is to print error
message provided as arguments followed by error string
corresponding to errno. Historically, we've used strerror_r() for
that (please note, we want our NSS module to be free of libvirt
internal functions, or glib even - hence, g_strerror() is off the
table).
Now strerror_r() is documented as:
Returns ... a pointer to a string that the function stores in
buf, or a pointer to some (immutable) static string (in which
case buf is unused).
Therefore, we can't rely the string being stored in the buf and
really need to store the retval and print that instead.
While touching this area, decrease the ebuf size, since its
current size (1KiB) is triggering our stack limit (2KiB) in some
cases.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Ages ago origCPU in domain private data was introduced to provide
backward compatibility when migrating to an old libvirt, which did not
support fetching updated CPU definition from QEMU. Thus origCPU will
contain the original CPU definition before such update. But only if the
update actually changed anything. Let's always fill origCPU with the
original definition when starting a domain so that we can rely on it
being always set, even if it matches the updated definition.
This fixes migration or save operations with custom domain XML after
commit v10.1.0-88-g14d3517410, which expected origCPU to be always set
to the CPU definition from inactive XML to check features explicitly
requested by a user.
https://issues.redhat.com/browse/RHEL-30622
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
$ virsh --connect test:///default nodedev-list
error: Failed to list node devices
error: unsupported flags (0x80000000) in function testConnectListAllNodeDevices
The test driver handles the nodedev state flags, we just need to
allow them
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The typed parameter array length must be non-NULL and either 0, or a
positive number.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The only thing we need to free in the cleanup code is virCPUDef and for
that we already have g_autoptr handler.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virt-aa-helper bash script constructs a path to itself when
it runs. But it isn't prepared for the case when there is a space
in the path leading to the script (something, something, double
quotes, something).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If path to the build directory contains spaces (e.g. meson setup
'a b') then our mocks don't work. The problem is in glibc where
not just a colon but also a space character is a delimiter for
LD_PRELOAD [1]. Hence, a test using mock tries to preload
something like libvirt.git/a b/libsomethingmock.so which is
interpreted by glibc as two separate strings: "libvirt.git/a",
"b/libsomethingmock.so".
One trick to get around this is to set LD_PRELOAD to just the
shared object file (without path) and let glibc find the mock in
paths specified in LD_LIBRARY_PATH (where only a colon or a
semicolon are valid separators [1]). This can be seen in action
by running say:
LD_DEBUG=libs ./virpcitest
1: https://man7.org/linux/man-pages/man8/ld.so.8.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In one of my recent commits I've chopped just too much and moved
a variable declaration into a function not realizing it's still
used on FreeBSD. Bring it back but only for the FreeBSD case.
Fixes: f8b5bd855f
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
clang on Fedora started to complain about some calls to g_new0()
we're making in vbox_snapshot_conf.c. Specifically, we're passing
zero as number of elements to allocate. And while usually SA
tools are not clever, in this specific case clang is right.
There are three cases where such call is made, but all of them
later use VIR_EXPAND_N() to allocate more memory (if needed). But
VIR_EXPAND_N() accepts a variable set to NULL happily.
Therefore, just drop those three calls to g_new0(..., 0) and let
VIR_EXPAND_N() allocate memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In QEMU and LXC drivers in a few places only
virNetDevBandwidthClear() is called. This means that if an
interface is of openvswitch vport profile, its QoS is not
removed. And to make matters worse - OVS is designed to remember
state even when corresponding interface is gone. This leads to
stale QoS settings piling up in OVS database.
To resolve this, introduce virDomainInterfaceClearQoS() which
looks at given interface and calls corresponding QoS clear
function. Then, basically replace virNetDevBandwidthClear() calls
in those hypervisor drivers with this new function.
Resolves: https://issues.redhat.com/browse/RHEL-30373
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The reason virDomainClearNetBandwidth() exists in src/conf/ is
that at the time its introduction we did not have a better place.
But now we do. Firstly, virDomainClearNetBandwidth() is
hypervisor agnostic code, but really has nothing to do with
domain configuration (it doesn't parse/format XML). Secondly, in
near future it'll call another function from src/hypervisor/ and
that's not really allowed from src/conf/.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The @brname argument of virNetDevOpenvswitchRemovePort() is and
was unused ever since its introduction in v0.9.11-rc1~257. Just
remove it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Both LXC and QEMU drivers have the same code to remove vport when
removing a domain's interface. Instead of repeating the same
pattern in both drivers, move the code into hypervisor agnostic
location (src/hypervisor/) and switch to calling this new
function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The comment to virNetDevOpenvswitchInterfaceGetMaster() contains
wrong function name. Fix this.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The vshFindTypedParamByName() function no longer exists (as of
v1.0.2-rc1~82), but its header file declaration was still kept
around. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When I suggested to Jim to call real virFileExists() I forgot to
also suggest calling init_syms(). Without it, real_virFileExists
pointer might be left unset. And indeed, that's what we were
seeing on FreeBSD.
This effectively reverts commit 4b5cc57ed3.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Calling the real virFileExists in qemusecuritymock.c can cause a
segfault in qemusecuritytest. No segfaults are noticed when calling
access(2) instead of virFileExists.
Fixes: 4ed5ade753
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
When performing an install, it's common for tooling such as virt-install
to remove the install kernel/initrd once they are successfully booted and
the domain has been redefined to boot without them. After the installation
is complete and the domain is rebooted/shutdown, the DAC and selinux
security drivers attempt to restore labels on the now deleted files. It's
harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir>
Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Add a check for file existence to the virSecurity*RestoreFileLabel functions,
and avoid relabeling if the file is no longer available. Skipping the restore
caused failures in qemusecuritytest, which mocks stat, chown, etc as part of
ensuring the security drivers properly restore labels. virFileExists is now
mocked in qemusecuritymock.c to return true when passed a file previously
seen by the mocked stat, chown, etc functions.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Since header file structure is a bit different on MacOS, it
doesn't get uint64_t type declaration and thus test_demo.c must
include it explicitly. This is proper solution anyway, because on
Linux we're apparently relying on the header file sneaking
through some other include.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/619
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
At the moment, there is no configuration option for the libvirt-guests
service that allows users to define that only persistent virtual machines
should be shutdown on host shutdown.
Currently, the service config allows to choose between two ON_SHUTDOWN
actions that are executed on running virtual machines when the host goes
down: shutdown, suspend.
The ON_SHUTDOWN action should be orthogonal to the type of the virtual
machine. However, the existing implementation, does not suspend
transient virtual machines.
This is the matrix of actions that is executed on virtual machines based
on the configured ON_SHUTDOWN action and the type of a virtual machine.
| persistent | transient
shutdown | shutdown | shutdown (what we want to change)
suspend | suspend | nothing
Add config option PERSISTENT_ONLY to libvirt-guests config that allows
users to define if the ON_SHUTDOWN action should be applied only on
persistent virtual machines. PERSISTENT_ONLY can be set to true, false,
default. The default option will implement the already existing logic.
Case 1: PERSISTENT_ONLY=default
| persistent | transient
shutdown | shutdown | shutdown
suspend | suspend | nothing
Case 2: PERSISTENT_ONLY=true
| persistent | transient
shutdown | shutdown | nothing
suspend | suspend | nothing
Case 3: PERSISTENT_ONLY=false
| persistent | transient
shutdown | shutdown | shutdown
suspend | suspend | suspend
Signed-off-by: Benjamin Taubmann <benjamin.taubmann@nutanix.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
While at it, simplify the code.
==9104== 38 bytes in 2 blocks are definitely lost in loss record 1,943 of 3,250
==9104== at 0x483B8C0: malloc (vg_replace_malloc.c:442)
==9104== by 0x4DFB69B: g_malloc (gmem.c:130)
==9104== by 0x4E1921D: g_strdup (gstrfuncs.c:363)
==9104== by 0x495D60B: g_strdup_inline (gstrfuncs.h:321)
==9104== by 0x495D60B: virFCReadRportValue (virfcp.c:62)
==9104== by 0x4A5F5CB: virNodeDeviceGetSCSITargetCaps (node_device_conf.c:2914)
==9104== by 0xBF62529: udevProcessSCSITarget (node_device_udev.c:657)
==9104== by 0xBF62529: udevGetDeviceDetails (node_device_udev.c:1406)
==9104== by 0xBF62529: udevAddOneDevice (node_device_udev.c:1563)
==9104== by 0xBF639B5: udevProcessDeviceListEntry (node_device_udev.c:1637)
==9104== by 0xBF639B5: udevEnumerateDevices (node_device_udev.c:1691)
==9104== by 0xBF639B5: nodeStateInitializeEnumerate (node_device_udev.c:2009)
==9104== by 0x49BDBFD: virThreadHelper (virthread.c:256)
==9104== by 0x5242069: start_thread (in /usr/lib64/libc.so.6)
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add small test case to demonstrate use of usb-net with user networking
backend.
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This patch will allow usb-net devices to be automatically assigned a USB
address (and skip any attempt to assign a PCI one).
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit 2ecdf25929 was intended to
implement two things: reduce stack usage inside ACL helpers and
minimally initialize virDomainDef object to avoid passing garbage
inside validation framework. Though original commit has not
touched other ACL helpers.
This patch adds proper clauses to
remoteRelayNetworkEventCheckACL
remoteRelayStoragePoolEventCheckACL
remoteRelayNodeDeviceEventCheckACL
remoteRelaySecretEventCheckACL
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We already allow the user to specify display="on" and ramfb="on" for
mdev host devices. But newer GPU models will no longer use the mdev
framework, so we should enable this same functionality for other
non-mdev passthrough PCI devices.
Resolves: https://issues.redhat.com/browse/RHEL-28808
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Right now, we display the message before actually attempting
to connect to the VM console. That operation, however, can
fail for a number of reasons: for example, is the VM doesn't
have a serial device, the output ends up looking like
$ virsh console cirros
Connected to domain 'cirros'
Escape character is ^] (Ctrl + ])
error: internal error: cannot find character device <null>
The initial message is misleading. Change things so that it's
only printed if we actually successfully connected to the VM
console.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reimplement the virsh-uriprecedence test case in virshtest. To do this
we need to add infrastructure to pass extra environment variables to the
tested virsh.
The user config files are shipped in repo rather than created in the
script.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The test simply invokes libvirtd and expects it to fail. We can do that
directly in meson without the need for a wrapper script.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The self-test command for both virsh and virt-admin is self contained
and directly reports success, thus we don't actually need to run a shell
wrapper around it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The test case is a fairly simple invocation of pool-create-as which can
be done easily from 'virshtest'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Test both situations (reading from non-regular file and reading a file
larger than (arbitrary) buffer size) via 'virshtest'.
To feed the pipe we need to create a thread that does it, but otherwise
it's fairly straightforward.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Invoke the majority of the command via DO_TEST_SCRIPT in 'virshtest'.
Some adaptation was needed to avoid printing of tables with volatile
data such as checkpoint creation time, which were converted to list
names-only.
To proprely test redefinition we store XMLs rather than taking them from
the defined checkpoints and use them separately to test redefinition of
checkpoint XMLs. This makes use of the 'cd' command in non-interactive
mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Invoke the majority of the command via DO_TEST_SCRIPT in 'virshtest'.
Some adaptation was needed to avoid printing of tables with volatile
data such as snapshot creation time, which were converted to list
names-only.
To proprely test redefinition we store XMLs rather than taking them from
the defined snapshots and use them separately to test redefinition of
snapshot XMLs. This makes use of the 'cd' command in non-interactive
mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
For testing purposes it will come handy to change the directory from a
batch-mode script. Remove the check forbidding use of the 'cd' command
in batch mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Until now when '--name' was used the parent was not printed and the
option was ignored. One option would be to declare the options mutually
exclusive, but for testing it may come handy to print both the snapshot
name and parent. Adjust the code to print them tab-separated and adjust
the docs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The 'virsh-start' case simply tried to start an already running VM. This
can be easily tested together with the tests for undefining a VM.
For this test the test driver config with multiple VMs comes handy as we
need to test 3 situations when we undefine and stop the VM.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's a simple virsh invocation which can be done in 'virshtest'
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's a simple test case invoking one virsh command thus it can be moved
to 'virshtest'
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
As all cases are negative we can test them all in one virsh run.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than using 'virsh define' for the tests use the XML (or idea what
the XML is testing) and use them as 'qemuxmlconftest' cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Adapt the 'tests/virsh-output-commands' file from 'virsh-output' test as
a source. Apart from expanding the bash function to each command, I've
also had to drop the negative tests for argument population, as a
command parsing error aborts the execution of the script right away
rather than just reporting the error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move the argument parsing tests excercising 'virsh event' options
from 'virsh-optparse' to 'virshtest'.
As the test invokes 'virsh event' with a timeout and thus waits for one
second pointlessly the patch also adds infrastructure to mark individual
cases as expensive and is skipped normally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move the argument parsing tests excercising various numeric options
(except 'virsh event') from 'virsh-optparse' to 'virshtest'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move the argument parsing tests excercising 'virsh snapshot-create-as'
from 'virsh-optparse' to 'virshtest'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move the argument parsing tests excercising 'virsh setvcpus' from
'virsh-optparse' to 'virshtest'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that all tests were converted, this is no longer needed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Adapt the tests to be invoked in one run. Note that multiple fake VMs
were used for the distinct tests so that they don't influence each
other.
This is the final coversion of tests to run in batch mode which halved
the runtime of 'virshtest' on my machine (1.11s vs 2.33s).
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The query and update can be tested in one run and validated against
files rather than hardcoded strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All of the commands can be tested in one 'virsh' run in batch mode and
tested against a file rather than hardcoded strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add the basic list and info commands into a script and run it via
VIR_TEST_RUN_SCRIPT to simplify the code and save up on 'virsh'
instances exec'd for the test.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that the output is tested against files these are not needed any
more. The brief existence of both proved that the output is identical.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Managing output files is much simpler especially with
VIR_TEST_REGENERATE_OUTPUT compared to putting the expected string blobs
into the C source file.
For now the output is tested both against the hardcoded strings as well
as the output files.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Embedding the expected output in a C source code makes it very hard to
extend tests. In order to be able to test the outputs against data in
files on disk we need better naming of the tests themselves.
Use virTestCounterNext/Reset with appropriate tags to give reasonable
names to the 'virsh echo' tests' and prepare the 'DO_TEST' macro for
wider use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Express what's possible via a "virsh script" rather than invoking
separate virsh for each one.
We need to keep a few for parity as the argument parser behaves
differently when processing argv-like input compared to a string.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Both argument passing and multiple command handling is already tested in
the 'multiple commands' cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Optimize invocation of the tests to share one 'virsh' binary as they
don't influence each other.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add support for reading a file and passing it to virsh in 'batch' mode
so that multiple commands can be easily tested with one invocation of
virsh.
To show how it's used adapt the alias handling tests to be invoked all
at once.
As in batch mode the arguments are read from a string and separated
inside virsh, one test is kept separate to be parsed in argv mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Upcoming patches will require that possibly multiple occurences of the
string to drop are present in the output string thus we need to adapt
testFilterLine to handle them.
Additionally we drop the unused return value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modify the test code so that if virsh fails both 'stdout' and 'stderr'
are captured and compared against the output and also the return value
is checked by appending it to the output.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modify testCompareOutputLit to take a filename argument and compare it
against and populate the arguments.
For tests which don't use the 'data' from virTestRun, we'll expect to
pass the output filename, thus we also propagate it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Drop the last enum member VSH_OFLAG_NONE and remove the 'flags' variable
from vshCmdOptDef.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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>
Enable domain host name translation to IP addresses
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
