Assume there's a dnsmasq running (because there's an active
virtual network that spawned it). Now, shut down the daemon,
remove the dnsmasq binary and start the daemon again. At this
point, networkUpdateState() is called, but dnsmasq_caps is NULL
(because networkStateInitialize() called earlier failed to set
them, rightfully though).
Now, the networkUpdateState() tries to read the dnsmasq's PID
file using virPidFileReadIfAlive() which takes a path to the
corresponding binary as one of its arguments. To provide that
path, dnsmasqCapsGetBinaryPath() is called, but since
dnsmasq_caps is NULL, it dereferences it and thus causes a crash.
It's true that virPidFileReadIfAlive() can deal with a removed
binary (well virPidFileReadPathIfAlive() which it calls can), but
iff the binary path is provided in its absolute form. Otherwise,
virFileResolveAllLinks() fails to canonicalize the path
(expected, the path doesn't exist anyway).
Therefore, reading dnsmasq's PID file didn't work before
v8.1.0-rc1~401 which introduced this crash. It was always set to
-1. But passing NULL as binary path instead, makes
virPidFileReadIfAlive() return early, right after the PID file is
read and it's confirmed the PID exists.
Yes, this may yield wrong results, as the PID might be of a
completely different binary. But this problem is preexistent and
until we start locking PID files, there's nothing we can do about
it. IOW, it would require rework of dnsmasq PID file handling.
Fixes: 4b68c982e2
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/456
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The only caller that actually wants to wait for the lock.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The parameter was added for consistency with virPidFileAcquirePath.
However, all callers of virPidFileAcquire pass false.
Remove the argument.
Partially-reverts: 2250a2b5d2
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Prior to firewalld version 1.0.0, the default action of ACCEPT in the
"libvirt" zone (subsequently overridden with a lower priority "REJECT"
action) would result in an implicit rule that allowed incoming sessions
through the zone; libvirt relied on this implicit rule to permit
incoming connections to guests that were connected via a libvirt
"routed" network.
Starting in firewalld 1.0.0, the rules generated for this same
zonefile changed such that incoming sessions through the libvirt zone
were no longer allowed, breaking the longstanding convention that they
should be allowed (only for routed networks).
However, beginning with firewalld 0.9.0, a zone can explicitly
allow/block forwarded traffic (by adding a "policy" to the zone that
specifies what happens to packets that are going in one zone and out
another zone).
This patch changes the zone for routed networks from "libvirt" to the
newly-added "libvirt-routed" zone that uses the new policy
functionality to once again allow incoming sessions to guests on
routed networks.
(If firewalld is < 0.9.0, then the policy file won't be read at all,
so firewalld won't log any error, and libvirt will just use the old
setup that takes advantage of the implicit forwarding rules).
Resolves: https://bugzilla.redhat.com/2055706
Signed-off-by: Eric Garver <eric@garver.life>
Reviewed-by: Laine Stump <laine@redhat.com>
This zone will be used for the routed network by default.
Note that this zone definition omits "forward" aka intra-zone
forwarding, because it requires firewalld >= 0.9.0.
Signed-off-by: Eric Garver <eric@garver.life>
Reviewed-by: Laine Stump <laine@redhat.com>
The path() method is deprecated in 0.55.0 and we're recommended
to use full_path() instead. Interestingly, we were already doing
do in couple of places, but not all of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace virNetworkDefParseString/File by direct calls to
virNetworkDefParse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace the two helpers virNetworkPortDefParseString/File with the
common helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Upcoming patch which is fixing the opening of drivers in monolithic mode
needs to know whether we are inside 'libvirtd' but the code where the
decision needs to happen is not re-compiled per daemon. Thus we need to
pass this information to the stateful driver init function so that it
can be remebered.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similar to the other drivers, virNetworkDriverState now has a
virObject-derived object called virNetworkDriverConfig which is used
for config items.
As a starting point, the directory paths used by the network driver
are moved there (again, parallelling what is done for other drivers).
Using items in virNetworkDriverConfig is (yes, again) similar to using
items in the other drivers' config - anything in the config object is
immutable (once initialized), so the state object only needs to be
locked while getting a reference to the config object, and then the
members of the config object can be safely used until the config
object is unrefed.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is more similar to lxc and qemu drivers, where the driver state
struct is defined along with a config struct in ${driver}_conf.h
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virMacMap module is used only for libvirt_guests NSS module
as it records list of MAC addresses used by certain guest. But
the module itself is usable if and only if the network assigns IP
addresses (i.e. has dnsmasq running). If it's some other
authority that assigns IP addresses then we do not need the
virMacMap module at all.
For instance, a network with no <forward/> type and no DHCP set
won't create /var/lib/libvirt/dnsmasq/ dir which is what the
module expects to exist. But there's no need for the module to
even care about such network.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/348
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Currently, whenever virNetworkObjSetMacMap() is called the same
pattern is used:
1) call virMacMapFileName() to generate a filename,
2) pass this filename to virMacMapNew(), and finally
3) pass retval from previous step to virNetworkObjSetMacMap().
Move this code into a helper (networkSetMacMap()) and replace
both pattern occurrences with its call.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virNetworkObjSetMacMap() API effectively steals passed
@macmap argument. However, the argument is a plain, first order
pointer. This requires every caller to set the argument to NULL
after the function was called. Let's make the function take
double pointer instead to make it obvious that the argument is
consumed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's annotate virMacMap variables in bridge_driver.c with
g_autoptr() so that they are automatically freed upon error. This
may look like a needless commit, since there's no memory leak
currently, but it simplifies the next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The networkStartNetworkVirtual() function handles starting of
networks of different forward types (none, nat, route, open).
And as a part of startup process dnsmasq might be spawned but
doesn't have to be (depending on the network configuration). The
@dnsmasqStarted variable is supposed to track whether dnsmasq was
started or not (so that it can be killed when starting network
fails after it was started). But the variable is set even when
the code decided not to start it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We already allow users to provide TFTP root path in network XML
and not specify any DHCP. This makes sense, because dnsmasq is
not only DHCP server but also TFTP server and users might have
a DHCP server configured on their own, outside of libvirt's
control and want just the TFTP part.
By moving TFTP config generator out of DHCP generator and calling
it for every IPv4 range, users can finally enable just TFTP.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2026765
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Generating configuration file for dnsmasq is done in
networkDnsmasqConfContents() which is this big, self-contained
function. Separate at least DHCP part into its own function for
better readability.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In networkDnsmasqConfContents() there's a for() loop which
initializes some variables in its initialization block. This
makes both the loop() statement and variable declaration block
look needlessly ugly. Speaking of variable declaration, also move
some variables which are used only within blocks into their
respective blocks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All these features are supposed to be handled by the call to
virDriverFeatureIsGlobal() placed right above the switch
statement, so if any of them is actually encountered inside
the switch statement it means there's a bug in the driver and
we should report an error.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virNetworkForwardDef.managed is a bool but
virNetworkPortDef.hostdevpci.managed is a virTristateBool, which
means that the current code performs the following incorrect
conversion:
false -> BOOL_ABSENT
true -> BOOL_YES
Using the virTristateBoolFromBool() helper solves the issue.
Fixes: 6cb0ec48bd
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
That is the proper POSIX way.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The 'virDrvFeature' has a combination of features which are asserted by
the specific driver and features which are actually global.
In many cases the implementation was cargo-culted into newer drivers
without re-assesing whether it makes sense.
This patch introduces a global function which will specifically handle
these global flags and defer the rest to the driver.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
This change was generated using the following spatch:
@ rule1 @
expression a;
identifier f;
@@
<...
- f(*a);
... when != a;
- *a = NULL;
+ g_clear_pointer(a, f);
...>
@ rule2 @
expression a;
identifier f;
@@
<...
- f(a);
... when != a;
- a = NULL;
+ g_clear_pointer(&a, f);
...>
Then, I left some of the changes out, like tools/nss/ (which
doesn't link with glib) and put back a comment in
qemuBlockJobProcessEventCompletedActiveCommit() which coccinelle
decided to remove (I have no idea why).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We recently started listing these in the spec file and, since we
were not creating them during the installation phase, that broke
RPM builds.
Fixes: 4b43da0bff
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The service files were copied out of the service file for libvirtd and
the name of the corresponding manpage was not fixed.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2045959
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
sysconfig files are owned by the admin of the host. They have the
liberty to put anything they want into these files. This makes it
difficult to provide different built-in defaults.
Remove the sysconfig file and place the current desired default into
the service file.
Local customizations can now go either into /etc/sysconfig/name
or /etc/systemd/system/name.service.d/my-knobs.conf
Attempt to handle upgrades in libvirt.spec.
Dirty files which are marked as %config will be renamed to file.rpmsave.
To restore them automatically, move stale .rpmsave files away, and
catch any new rpmsave files in %posttrans.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Since dnsmasq supports --ra-param for a long time, this code is now
unused.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
The network driver has put all its rules into private chains (created
by libvirt) since commit 7431b3eb9a, which was included in
libvirt-5.1.0. When the conversion was made, code was included that
would attempt to delete existing rules in the default chains, to make
it possible to upgrade libvirt without restarting the host OS.
Almost 3 years has passed, and it is doubtful that anyone will be
attempting to upgrade directly from a pre-5.1.0 libvirt to something
as new as 8.0.0 (possibly with the exception of upgrading the entire
OS to a new release, which would include also rebooting), so it is now
safe to remove this code.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The function can't fail at this point. Remove the return value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Some forward declarations in bridge_driver.c are not needed
really. They only create a noise when trying to jump onto the
correct tag. Drop them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
In a few places we declare a variable (which is optionally
followed by a code not touching it) then set the variable to a
value and return the variable immediately. It's obvious that the
variable is needless and the value can be returned directly
instead.
This patch was generated using this semantic patch:
@@
type T;
identifier ret;
expression E;
@@
- T ret;
... when != ret
when strict
- ret = E;
- return ret;
+ return E;
After that I fixed couple of formatting issues because coccinelle
formatted some lines differently than our coding style.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
I added new driver functions to handle creating network with
given flags. I also replaced definitions of the functions without
flags with function calls to the new ones.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We don't need to propagate all public flags, only the information
about the presence of the validation one, which can differ from
function to function. This patch makes it easier and more
readable in case of a future additions of validation flags.
This change was suggested by Daniel.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
