The parent array takes ownership of the inserted value once all checks
pass. Don't make the callers second-guess when that happens and modify
the function to take a double pointer so that it can be cleared once the
ownership is taken.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The 'conflict' key in a virt_daemon_unit dictionary is not used when
generating systemd service and socket files. The comment associated
with the key claims the default is 'true', and a few build files
needlessly set it to 'true' when defining their virt_daemon_unit.
Remove the 'conflict' key and its use in the affect build files.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In the past, the MTU of libvirt virtual network bridge devices was
implicitly set by setting the MTU of the "dummy tap device" (which was
being added in order to force a particular MAC address from the
bridge). But the dummy tap device was removed in commit ee6c936fbb
(libvirt-6.8.0), and so the mtu setting in the network is ignored.
The solution is, of course, to explicitly set the bridge device MTU
when it is created.
Note that any guest interface with a larger MTU that is attached will
cause the bridge to (temporarily) assume the larger MTU, but it will
revert to the bridge's own MTU when that device is deleted (this is
not due to anything libvirt does; it's just how Linux host bridges
work).
Fixes: ee6c936fbb
Resolves: https://bugzilla.redhat.com/1913561
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When defining/creating a network the bridge name may be filled in
automatically by libvirt (if none provided in the input XML or
the one provided is a pattern, e.g. "virbr%d"). During the
bridge name generation process a candidate name is generated
which is then checked with the rest of already defined/running
networks for collisions.
Problem is, that there is no mutex guarding this critical section
and thus if two threads line up so that they both generate the
same candidate they won't find any collision and the same name is
then stored.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/78
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
When rewriting the function, I've mistakenly declared a variable
and assigned it to itself. Let's initialize the variable properly.
Fixes: 5fb6d98c88
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
After v6.3.0-rc1~64 a lease can have infinite expiry time. This
means that the expiration time will appear as a value of zero.
Do the expiration check only if the expiration time is not zero.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1908053
Fixes: 97a0aa2467
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Firstly, bring variables that are used only within loops into
their respective loops. Secondly, drop 'error' label which is
redundant since we have @rv which holds the return value.
Thirdly, fix indendation in one case, the rest is indented
properly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This function is misusing VIR_INSERT_ELEMENT() to behave like
VIR_APPEND_ELEMENT(). Use the latter to make it explicit what we
are trying to achieve.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We don't need to track the lease file size. Instead, we can
simply check if the file was empty by comparing the buffer the
file was read into with an empty string.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
If leasehelper fails all that we are left with is a simple error
message produced by dnsmasq:
lease-init script returned exit code 1
This is because the leasehelper did not write any message to
stderr. According to dnsmasq's manpage, whenever it's invoking
leasehelper the stderr is kept open:
All file descriptors are closed except stdin, which is open to
/dev/null, and stdout and stderr which capture output for
logging by dnsmasq.
As debugging leasehelper is not trivial (because dnsmasq invokes
it with plenty of env vars set - that's how data is passed onto
helper), let's print an error into stderr if exiting with an
error. And since we are not calling public APIs, we have to call
virDispatchError() explicitly and since we don't have any
connection open, we have to pass NULL.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We are generating a fresh UUID and storing it in the XML for the
default network, but this is unnecessary because the network
driver will automatically generate one if it's missing from the
XML; the fact that we only do this if the uuidgen command happens
to be available on the build machine is further proof that we can
safely skip this step.
This patch is best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
0f7436ca54 added code during virtual network startup to wait for DAD
(Duplicate Address Detection) to complete if there were any IPv6
addresses on the network. This wait was needed because (according to
the commit log) "created problems when [the "dummy" tap device] is set
to IFF_DOWN prior to DAD completing".
That commit in turn referenced commit db488c7917, which had added the
code to set the dummy tap device IFF_DOWN, commenting "DAD has
happened (dnsmasq waits for it)", and in its commit message pointed
out that if we just got rid of the dummy tap device this wouldn't be
needed.
Now that the dummy tap device has indeed been removed (commit
ee6c936fbb), there is no longer any need to set it IFF_DOWN, and thus
nothing requiring us to wait for DAD to complete. At any rate, with
the dummy tap device removed, leaving nothing else on the bridge when
it is first started, DAD never completes, leading to failure to start
any IPv6 network.
So, yes, this patch removes the wait for DAD completion, and IPv6
networks can once again start, and their associated dnsmasq process
starts successfully (this is the problem that the DAD wait was
originally intended to fix)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
A long time ago we introduced a dummy tap device (e.g. virbr0-nic) that
we attached to the bridge device created for virtual networks:
commit 5754dbd56d
Author: Laine Stump <laine@redhat.com>
Date: Wed Feb 9 03:28:12 2011 -0500
Give each virtual network bridge its own fixed MAC address
This was a hack to workaround a Linux kernel bug where it would not
honour any attempt to set a MAC address on a bridge. Instead the
bridge would adopt the numerically lowest MAC address of all NICs
attached to the bridge. This lead to the MAC addrss of the bridge
changing over time as NICs were attached/detached.
The Linux bug was actually fixed 3 years before the libvirt
workaround was added in:
commit 92c0574f11598c8036f81e27d2e8bdd6eed7d76d
Author: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue Jun 17 16:10:06 2008 -0700
bridge: make bridge address settings sticky
Normally, the bridge just chooses the smallest mac address as the
bridge id and mac address of bridge device. But if the administrator
has explictly set the interface address then don't change it.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
but libvirt needed to support RHEL-5 kernels at that time, so
none the less added the workaround.
We have long since dropped support for RHEL-5 vintage distros,
so there's no reason to keep the dummy tap device for the purpose
of setting the bridge MAC address.
Later the dummy TAP device was used for a second purpose related
to IPv6 DAD (Duplicate Address Detection) in:
commit db488c7917
Author: Benjamin Cama <benoar@dolka.fr>
Date: Wed Sep 26 21:02:20 2012 +0200
network: fix dnsmasq/radvd binding to IPv6 on recent kernels
This was again dealing with a regression in the Linux kernel, where
if there were no devices attached to the bridge in the UP state,
IPv6 DAD would not be performed. The virbr0-nic was attached but
in the DOWN state, so the above libvirt fix tenporarily brought
the NIC online. The Linux commit causing the problem was in v2.6.38
commit 1faa4356a3bd89ea11fb92752d897cff3a20ec0e
Author: stephen hemminger <shemminger@vyatta.com>
Date: Mon Mar 7 08:34:06 2011 +0000
bridge: control carrier based on ports online
A short while later Linux was tweaked so that DAD would still occur
if the bridge had no attached devices at all in 3.1:
commit b64b73d7d0c480f75684519c6134e79d50c1b341
Author: stephen hemminger <shemminger@vyatta.com>
Date: Mon Oct 3 18:14:45 2011 +0000
bridge: leave carrier on for empty bridge
IOW, the only reason we need the DAD hack of bringing virbr0-nic
online is because virbr0-nic exists. Once it doesn't exist, then
we hit the "empty bridge" case which works in Linux.
We can rely on distros having Linux kernel >= 3.1, so both things
that the virbr0-nic are doing are redundant.
Fixes https://gitlab.com/libvirt/libvirt/-/issues/53
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently, we are mixing: #if HAVE_BLAH with #if WITH_BLAH.
Things got way better with Pavel's work on meson, but apparently,
mixing these two lead to confusing and easy to miss bugs (see
31fb929eca for instance). While we were forced to use HAVE_
prefix with autotools, we are free to chose our own prefix with
meson and since WITH_ prefix appears to be more popular let's use
it everywhere.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This patch takes care of just the obvious cases: there are
many more situations where the data we pass to configure_file()
could likely be obtained in a more effective way, but we can
address the low-hanging fruits as a first approximation.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Whenever libvirt is upgraded on a Debian system, the user will be
prompted along the lines of
Configuration file '/etc/libvirt/qemu/networks/default.xml'
==> Modified (by you or by a script) since installation.
==> Package distributor has shipped an updated version.
What would you like to do about it ? Your options are:
Y or I : install the package maintainer's version
N or O : keep your currently-installed version
D : show the differences between the versions
Z : start a shell to examine the situation
The default action is to keep your current version.
*** default.xml (Y/I/N/O/D/Z) [default=N] ? d
--- /etc/libvirt/qemu/networks/default.xml 2020-08-04 12:57:25.450911143 +0200
+++ /etc/libvirt/qemu/networks/default.xml.dpkg-new 2020-08-03 22:47:15.000000000 +0200
@@ -1,19 +1,11 @@
-<!--
-WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
-OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:
- virsh net-edit default
-or other application using the libvirt API.
--->
-
<network>
<name>default</name>
- <uuid>612a2cab-72fb-416d-92bc-4d9e597bfb63</uuid>
- <forward mode='nat'/>
- <bridge name='virbr0' stp='on' delay='0'/>
- <mac address='52:54:00:1f:03:79'/>
- <ip address='192.168.122.1' netmask='255.255.255.0'>
+ <uuid>d020b839-4379-492c-aa74-eab7365076e6</uuid>
+ <bridge name="virbr0"/>
+ <forward/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
- <range start='192.168.122.2' end='192.168.122.254'/>
+ <range start="192.168.122.2" end="192.168.122.254"/>
</dhcp>
</ip>
</network>
The UUID situation should probably be handled the same way it is
in the spec file by stripping it, and in general we could behave
much better towards users, but one part of the diff that
immediately stands out is that some lines are highlighted not
because they are semantically different, but simply because they
use different types of quotes around attributes.
Since the canonical version of all libvirt XML documents (as
returned by the various vir*GetXMLDesc() APIs) as well as the
on-disk representations use single quotes, let's use the same
for configuration files we install as well.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
EXTRA_DIST is not relevant because meson makes a git copy when creating
dist archive so everything tracked by git is part of dist tarball.
The remaining ones are not converted to meson files as they are
automatically tracked by meson.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
Refactor networkSetIPv6Sysctls to remove repetition and reuse
of the 'field' variable.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.
This patch coincidentally fixes a bug in
networkFindUnusedBridgeName(), where we would log an error yet still
return success if we failed to find a single unused "virbrNNN" name
after checking all values of "N" from 0 - 256. Said bug was introduced
when that function was originally written, in commit a28d3e485f
(libvirt 1.2.15, 2015)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This includes standard g_autofree() as well as other objects that have
a cleanup function defined to use via g_autoptr (virCommand,
virJSONValue)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
hostsfilestr was not being freed. This will be turned into g_autofree
in an upcoming patch converting a lot more of the same file to using
g_auto*, but I wanted to make a separate patch for this first so the
other patch is simpler to review (and to make backporting easier).
The leak was introduced in commit 97a0aa2467
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Some functions or code paths that may fail don't report error
(e.g. when acquiring PID file fails) leading to a silent quit
of the leaseshelper. This makes it super hard for us and users
to debug what is happening. Fortunately, dnsmasq captures both
stdout and stderr so we can write an error message there.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
On a DHCP transaction, dnsmasq runs our leases helper which
updates corresponding JSON files. While one dnsmasq won't run the
leaseshelper in parallel, two dnsmasqs (from two distinct
networks) might. To avoid corrupting JSON file, the leaseshelper
acquires PID file first. Well, the way it's acquiring it is not
ideal - it calls virPidFileAcquirePath(wait = false); which
means, that either it acquires the PID file instantly or returns
an error and does not touch the JSON at all. This in turn means
that there might be a leases record missing. With wait = true,
this won't happen.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1840307
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now that we have support for IPv6 in the iptables helpers, and a new
option in the XML schema, we can wire up support for it in the network
driver.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Juan Quintela noticed that when he restarted libvirt he was getting
extra iptables rules added by libvirt even though he didn't have any
libvirt networks that used iptables rules. It turns out this also
happens if the firewalld service is restarted. The extra rules are
just the private chains, and they're sometimes being added
unnecessarily because they are added separately in a global
networkPreReloadFirewallRules() that does the init if there are any
active networks, regardless of whether or not any of those networks
will actually add rules to the host firewall.
The fix is to change the check for "any active networks" to instead
check for "any active networks that add firewall rules".
(NB: although the timing seems suspicious, this isn't a new regression
caused by the recently pushed f5418b427 (which forces recreation of
private chains when firewalld is restarted); it was an existing bug
since iptables rules were first put into private chains, even after
commit c6cbe18771 delayed creation of the private chains. The
implication is that any downstream based on v5.1.0 or later that cares
about these extraneous (but harmless) private chains would want to
backport this patch (along with the other two if they aren't already
there))
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
When firewalld is stopped, it removes *all* iptables rules and chains,
including those added by libvirt. Since restarting firewalld means
stopping and then starting it, any time it is restarted, libvirt needs
to recreate all the private iptables chains it uses, along with all
the rules it adds.
We already have code in place to call networkReloadFirewallRules() any
time we're notified of a firewalld start, and
networkReloadFirewallRules() will call
networkPreReloadFirewallRules(), which calls
networkSetupPrivateChains(); unfortunately that last call is called
using virOnce(), meaning that it will only be called the first time
through networkPreReloadFirewallRules() after libvirtd starts - so of
course when firewalld is later restarted, the call to
networkSetupPrivateChains() is skipped.
The neat and tidy way to fix this would be if there was a standard way
to reset a pthread_once_t object so that the next time virOnce was
called, it would think the function hadn't been called, and call it
again. Unfortunately, there isn't any official way of doing that (we
*could* just fill it with 0 and hope for the best, but that doesn't
seem very safe.
So instead, this patch just adds a static variable called
chainInitDone, which is set to true after networkSetupPrivateChains()
is called for the first time, and then during calls to
networkPreReloadFirewallRules(), if chainInitDone is set, we call
networkSetupPrivateChains() directly instead of via virOnce().
It may seem unsafe to directly call a function that is meant to be
called only once, but I think in this case we're safe - there's
nothing in the function that is inherently "once only" - it doesn't
initialize anything that can't safely be re-initialized (as long as
two threads don't try to do it at the same time), and it only happens
when responding to a dbus message that firewalld has been started (and
I don't think it's possible for us to be processing two of those at
once), and even then only if the initial call to the function has
already been completed (so we're safe if we receive a firewalld
restart call at a time when we haven't yet called it, or even if
another thread is already in the process of executing it. The only
problematic bit I can think of is if another thread is in the process
of adding an iptable rule at the time we're executing this function,
but 1) none of those threads will be trying to add chains, and 2) if
there was a concurrency problem with other threads adding iptables
rules while firewalld was being restarted, it would still be a problem
even without this change.
This is yet another patch that fixes an occurrence of this error:
COMMAND_FAILED: '/usr/sbin/iptables -w10 -w --table filter --insert LIBVIRT_INP --in-interface virbr0 --protocol tcp --destination-port 67 --jump ACCEPT' failed: iptables: No chain/target/match by that name.
In particular, this resolves: https://bugzilla.redhat.com/1813830
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
