Laine Stump [Sat, 20 Apr 2024 02:19:43 +0000 (22:19 -0400)]
meson: stop looking for iptables/ip6tables/ebtables at build time
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: 6aa2fa38b04b802f137e51ebbeb4ca9b67487575 Signed-off-by: Laine Stump <laine@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: save network status when firewall rules are reloaded
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: use previously saved list of firewall removal commands
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
conf: add a virFirewall object to virNetworkObj
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: new functions virFirewallParseXML() and virFirewallFormat()
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: new function virFirewallNewFromRollback()
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: turn on auto-rollback for the rules added for virtual networks
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: implement rollback rule autocreation for iptables commands
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: new functions to support adding individual firewall rollback commands
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: framework to call backend-specific function to init private filter chains
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: support setting firewallBackend from network.conf
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util/network: new virFirewallBackend enum
(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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: determine ignoreErrors value when creating virFirewallCmd, not when applying
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: add -w/--concurrent when applying a FirewallCmd rather than when building it
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: check for 0 args when applying iptables rule
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: rename virNetFilterAction to iptablesAction, and add VIR_ENUM_DECL/IMPL
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util: change name of virFirewallRule to virFirewallCmd
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: make all iptables functions used only in network_iptables.c static
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
network: move all functions manipulating iptables rules into network_iptables.c
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:
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>
Laine Stump [Sat, 20 Apr 2024 02:19:42 +0000 (22:19 -0400)]
util/network: move viriptables.[ch] from util to network directory
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>
Michal Privoznik [Wed, 22 May 2024 07:31:50 +0000 (09:31 +0200)]
src: Fix return types of .stateInitialize callbacks
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>
Jonathon Jongsma [Thu, 16 May 2024 21:46:04 +0000 (16:46 -0500)]
qemu: fix qemu command for pci hostdevs and ramfb='off'
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>
Rayhan Faizel [Wed, 15 May 2024 10:27:53 +0000 (15:57 +0530)]
qemu: Implement support for hotplugging evdev input devices
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>
Andrea Bolognani [Wed, 15 May 2024 15:55:49 +0000 (17:55 +0200)]
rpm: Drop with_ssh_proxy define
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>
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:
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>
Michal Privoznik [Fri, 10 May 2024 07:54:59 +0000 (09:54 +0200)]
github: Update lockdown message when opening a PR
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>
Abhiram Tilak [Mon, 13 May 2024 15:07:53 +0000 (20:37 +0530)]
docs: formatsnapshot: add docs for snapshotDeleteInProgress
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>
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>
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>
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>
Rayhan Faizel [Mon, 13 May 2024 13:55:03 +0000 (19:25 +0530)]
qemu_hotplug: Properly assign USB address to hotplugged usb-net device
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>
Michal Privoznik [Tue, 16 Apr 2024 14:32:26 +0000 (16:32 +0200)]
tools: Introduce SSH proxy
This allows users to SSH into a domain with a VSOCK device:
ssh user@qemu/machineName
So far, only QEMU domains are supported AND qemu:///system is
looked for the first for 'machineName' followed by
qemu:///session. I took an inspiration from Systemd's ssh proxy
[1] [2].
To just work out of the box, it requires (yet unreleased) systemd
to be running inside the guest to set up a socket activated SSHD
on the VSOCK. Alternatively, users can set up the socket
activation themselves, or just run a socat that'll forward vsock
<-> TCP communication.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/579 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jiri Denemark [Thu, 9 May 2024 14:16:57 +0000 (16:16 +0200)]
network: Register dnsmasq with resolved only when really requested
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>
docs: ensure HTML/images are built before running reference tests
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>
test: drop bogus check for YAJL from libxl test/mock
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>
tests: always build securityselinuxhelper if libselinux is present
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>
meson: record which other options are a pre-requisite
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>
tests: fix two off-by-1 errors in read-big-pipe test
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 '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>
Peter Krempa [Mon, 6 May 2024 15:44:42 +0000 (17:44 +0200)]
udevListInterfacesByStatus: Don't try to return NULL names
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 2ca94317ac6.
Resolves: https://issues.redhat.com/browse/RHEL-34615 Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal Privoznik [Thu, 18 Apr 2024 07:19:52 +0000 (09:19 +0200)]
datatypes: Declare g_autoptr cleanup functions for more public objects
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>
Michal Privoznik [Mon, 22 Apr 2024 09:55:46 +0000 (11:55 +0200)]
qemu: Substract isolcpus from all online affinity
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.
Resolves: https://issues.redhat.com/browse/RHEL-33082 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>
Adam Julis [Fri, 26 Apr 2024 18:28:47 +0000 (20:28 +0200)]
qemu_saveimage: add zstd to supported compression formats
Extend the list of supported formats, update and clarify comment
in qemu.conf.in (removed misleading sentence about the order of
compression format types).
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/589 Signed-off-by: Adam Julis <ajulis@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@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>
Oleg Sviridov [Fri, 3 May 2024 09:26:51 +0000 (12:26 +0300)]
hyperv: prevent potential NULL dereference
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.
Jim Fehlig [Mon, 29 Apr 2024 20:50:07 +0000 (14:50 -0600)]
libxl: Fix domxml-to-native conversion
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.
Tim Wiederhake [Fri, 9 Feb 2024 10:52:56 +0000 (11:52 +0100)]
cpu_map: Drop 'mpx' from x86 cpu models
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:
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>
conf: Change return value of some CPU feature APIs
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>
rpc: ensure temporary GSource is removed from client event loop
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>
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>
Michal Privoznik [Mon, 29 Apr 2024 07:49:17 +0000 (09:49 +0200)]
scripts/meson-dist.py: Get builddir from env too
When meson runs a dist script it sets both MESON_BUILD_ROOT and
MESON_DIST_ROOT envvars [1]. But for some reason, we took the
former as an argument and obtained the latter via env. Well,
obtain both via env.
1: https://mesonbuild.com/Reference-manual_builtin_meson.html#mesonadd_dist_script Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>