Update the device state file every time the device is connected,
disconnected, or becomes unmanaged. In this way, NM becomes more
robust against crashes or forced terminations because it can resume
the previous device state seamlessly.
Rename nm_manager_write_device_state() to
nm_manager_write_device_state_all(), and split out the code to write a
single device state to a new function.
When the IPv4 method is 'auto' and there are static addresses
configured in the connection, start a DAD probe for the static
addresses and apply them immediately on success, without waiting for
DHCP to complete.
Note that if the static address is in the same subnet of the DHCP one,
when we add the DHCP address we want it to be primary and so we will
remove the static address temporarily to achieve the right order of
addresses.
https://bugzilla.redhat.com/show_bug.cgi?id=1369905
Some properties in NetworkManager's D-Bus API are IPv4 addresses
in network byte order (big endian). That is problematic.
It is no problem, when the NetworkManager client runs on the same
host. That is the case with libnm, which does not support to be used
remotely for the time being.
It is a problem for an application that wants to access the D-Bus
interface of NetworkManager remotely. Possibly, such an application
would be implemented in two layers:
- one layer merely remotes D-Bus, without specific knowledge of
NetworkManager's API.
- a higher layer which accesses the remote D-Bus interface of NetworkManager.
Preferably it does so in an agnostic way, regardless of whether it runs
locally or remotely.
When using a D-Bus library, all accesses to 32 bit integers are in
native endianness (regardless of how the integer is actually encoded
on the lower layers). Likewise, D-Bus does not support annotating integer
types in non-native endianness. There is no way to annotate an integer
type "u" to be anything but native order.
That means, when remoting D-Bus at some point the endianness must be
corrected.
But by looking at the D-Bus introspection alone, it is not possible
to know which property need correction and which don't. One would need
to understand the meaning of the properties.
That makes it problematic, because the higher layer of the application,
which knows that the "Nameservers" property is supposed to be in network
order, might not easily know, whether it must correct for endianness.
Deprecate IP4Config properties that are only accessible with a particular
endianness, and add new properties that expose the same data in an
agnostic way.
Note that I added "WinsServerData" to be a plain "as", while
"NameserverData" is of type "aa{sv}". There is no particularly strong
reason for these choices, except that I could imagine that it could be
useful to expose additional information in the future about nameservers
(e.g. are they received via DHCP or manual configuration?). On the other
hand, WINS information likely won't get extended in the future.
Also note, libnm was not modified to use the new D-Bus fields. The
endianness issue is no problem for libnm, so there is little reason to
change it (at this point).
https://bugzilla.redhat.com/show_bug.cgi?id=1153559https://bugzilla.redhat.com/show_bug.cgi?id=1584584
nm_gobject_notify_together() freezes the notifications to emit both
notification signals together. That matters for NMDBusObject base
class, which hooks into dispatch_properties_changed() to emit a combined
"PropertiesChanged" signal.
Note, that during calls like nm_ip4_config_replace(), we already
froze/thawed the notifications. So, this change adds unnecessary
freeze/thaw calls, because signal emition is already frozen.
That is a bit ugly, because g_object_freeze_notify() is more heavy
than I'd wish it would be.
Anyway, for other places, like nm_ip4_config_reset_routes() that is
not the case. And correctness trumps performance.
Ultimately, the issue there is that we use NMIP4Config / NMIP6Config
both to track internal configuration, and to expose it on D-Bus.
The majority of created NMIP4Config / NMIP6Config instances won't get
exported, and but still pay an unnecessary overhead. The proper solution
to minimize the overhead would be, to separate these uses.
It seems, curl_multi_socket_action() can fail with
connectivity check failed: 4
where "4" means CURLM_INTERNAL_ERROR.
When that happens, it also seems that the file descriptor may still have data
to read, so the glib IO callback _con_curl_socketevent_cb() will be called in
an endless loop. Thereby, keeping the CPU busy with doing nothing (useful).
Workaround by disabling polling on the file descriptor when something
goes wrong.
Note that optimally we would cancel the affected connectivity-check
right away. However, due to the design of libcurl's API, from within
_con_curl_socketevent_cb() we don't know which connectivity-checks
are affected by a failure on this file descriptor. So, all we can do
is avoid polling on the (possibly) broken file descriptor. Note that
we anyway always schedule a timeout of last resort for each check. Even
if something goes very wrong, we will fail the check within 15 seconds.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=903996
On non-Windows, libcurl's "curl_socket_t" type is just a typedef for
int. We rely on that, because we use it as file descriptor.
Add a compile time check to ensure that.
Since this is "C" there are not namespaces and libraries commonly choose
a particular name prefix for their symbols.
In case of libcurl, that is "curl_".
We should avoid using the same name prefix, and choose something distinct.
Before:
$ nmcli connection up my-wired
Error: Connection activation failed: No suitable device found for this connection.
After:
$ nmcli connection up my-wired
Error: Connection activation failed: No suitable device found for this connection (device eth0 not available because device has no carrier).
This relies on nm_manager_get_best_device_for_connection() giving a
suitable error. That is however a bit complicated, because if no
suitable device is found, it's not immediately clear what is the
exact reason. E.g. if you try to activate a Wi-Fi profile, the
failure reason
"SSID is not visible"
is better than
"Wi-Fi profile cannot activate on ethernet device".
This is controlled by carefully setting the failure codes
NM_UTILS_ERROR_CONNECTION_AVAILABLE_* to indicate an absolute
relevance of the failure. And subsequently, by selecting the failure
with the highest relevance. This might still need some improvements,
for example by reordering checks (so that more relevant failures
are handled first) and tweaking the error relevance.
Before:
$ nmcli connection up my-wired ifname eth0
Error: Connection activation failed: Connection 'my-wired' is not available on the device eth0 at this time.
After:
$ nmcli connection up my-wired ifname eth0
Error: Connection activation failed: Connection 'my-wired' is not available on device eth0 because device has no carrier
Still unused, but will be used to give a better failure reason when
no device is found.
The difficulty here is to select the failure message from the most appropriate
device. This might still need some tweaking by setting the error codes accordingly
and re-ordering checks so that failure cares that are more accurate are handled
first.
Note the special error codes NM_UTILS_ERROR_CONNECTION_AVAILABLE_*.
This will be used to determine, whether the profile is fundamentally
incompatible with the device, or whether just some other properties
mismatch. That information will be importand during a plain `nmcli
connection up`, where NetworkManager searches all devices for a device
to activate. If no device is found (and multiple errors happened),
we want to show the error that is most likely relevant for the user.
Also note, how NMDevice's check_connection_compatible() uses the new
class field "device_class->connection_type_check_compatible" to simplify
checks for compatible profiles.
The error reason is still unused.
It seems to me the NM_DEVICE_CLASS_DECLARE_TYPES() macro confuses more
than helping. Let's explicitly initialize the two fields, albeit with
another helper macro NM_DEVICE_DEFINE_LINK_TYPES() to get the list of
link-types right.
For consistency, also leave nop-lines like
device_class->connection_type_supported = NULL;
device_class->link_types = NM_DEVICE_DEFINE_LINK_TYPES ();
because all NMDevice class init methods should have this same
boiler plate code and to make it explicit that this is intended.
And there are only 3 occurences where this actually comes into play.
NMDeviceOvsPort and NMDeviceOvsInterface don't have an underlying link-type from platform.
Still use NM_DEVICE_CLASS_DECLARE_TYPES() macro, for consistancy reasons.
This requires to extend NM_DEVICE_CLASS_DECLARE_TYPES() macro, to support
a variadic argument list with zero link-types.
The majority of device implementations name their parent-class variable
"device_class". That also makes more sense as it is more consistant.
E.g. "parent" sounds like it's the direct parent, but that is not
the crucial point here. The crucial point at this place, is that we
access the NMDeviceClass typed pointer. Rename.
We also have NMDeviceClass.check_connection_compatible(). It is preferable
to use unique names, especially for the virtual function table. A reasonable
thing to do is grep for the function name to find all places that implement
this function. But if different classes use the same name, grep just
turns up annoying false positives.
Extend the lame-ass dhclient.conf parser to ignore the blocks we can't
do anything useful about: alias{}, pseudo{} and even lease{}.
Note that there's still a lot of cases we can't handle without a
full-fledged dhclient.conf parser -- notably the files that don't use
line breaks to separate the statements.
That is probably okay -- the whole thing is probably mostly useless and
we shall ever bother only about cases that actually cause trouble.
https://github.com/NetworkManager/NetworkManager/pull/153
We used to build with -std=gnu89 so commit 1391bdfa61
added a local patch to systemd code to avoid compilation error due to
missing __STDC_VERSION__ define.
In the meantime, since commit ba2b2de3ad
and commit b9bc20f4da, we also use -std=gnu99
and thus __STDC_VERSION__ is defined.
Revert our local modification.
1) the command line gets shorter. I frequently run `make V=1` to see
the command line arguments for the compiler, and there is a lot
of noise.
2) define each of these variables at one place. This makes it easy
to verify that for all compilation units, a particular
define has the same value. Previously that was not obvious or
even not the case (see commit e5d1a71396
and commit d63cf1ef2f).
The point is to avoid redundancy.
3) not all compilation units need all defines. In fact, most modules
would only need a few of these defines. We aimed to pass the necessary
minium of defines to each compilation unit, but that was non-obvious
to get right and often we set a define that wasn't used. See for example
"src_settings_plugins_ibft_cppflags" which needlessly had "-DSYSCONFDIR".
This question is now entirely avoided by just defining all variables in
a header. We don't care to find the minimum, because every component
gets anyway all defines from the header.
4) this also avoids the situation, where a module that previously did
not use a particular define gets modified to require it. Previously,
that would have required to identify the missing define, and add
it to the CFLAGS of the complation unit. Since every compilation
now includes "config-extra.h", all defines are available everywhere.
5) the fact that each define is now available in all compilation units
could be perceived as a downside. But it isn't, because these defines
should have a unique name and one specific value. Defining the same
name with different values, or refer to the same value by different
names is a bug, not a desirable feature. Since these defines should
be unique accross the entire tree, there is no problem in providing
them to every compilation unit.
6) the reason why we generate "config-extra.h" this way, instead of using
AC_DEFINE() in configure.ac, is due to the particular handling of
autoconf for directory variables. See [1].
With meson, it would be trivial to put them into "config.h.meson".
While that is not easy with autoconf, the "config-extra.h" workaround
seems still preferable to me.
[1] https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Installation-Directory-Variables.html
Also, "src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-common.h"
already has a define IFCFG_DIR, but with a different value.
We shouldn't name different things the same.
We previously kept any acd-manager running if the device was
disconnected. It was possible to trigger a crash by setting a long
dad-timeout and interrupting the activation request:
nmcli con add type ethernet ifname eth0 con-name eth0+ ip4 1.2.3.4/32
nmcli con mod eth0+ ipv4.dad-timeout 10000
nmcli -w 2 con up eth0+
nmcli con down eth0+
After this, the n-acd timer would fire after 10 seconds and try to
disconnect an already disconnected device, throwing the assertion:
NetworkManager:ERROR:src/devices/nm-device.c:9845:
activate_stage5_ip4_config_result: assertion failed: (req)
Fixes: 28f6e8b4d2
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
If a VPN with default route is activated, the Manager's
PrimaryConnection property is not updated to indicate the VPN as
primary connection.
This happens because the PrimaryConnection property gets updated when
the default_ipX_device property of NMPolicy changes, and the primary
connection is set to the activation request currently pending on the
default device. We select the base (for example, ethernet) device as
best device and therefore the NMActRequest active on it is selected as
primary connection.
This patch fixes the problem by properly selecting the VPN as
primary. It seems a better choice to track best active connections
directly from NMPolicy instead of going through two steps.
Commit 10753c3616 ("manager: merge VPN handling into
_new_active_connection()") added a check to fail the activation of
VPNs when a device is passed to ActivateConnection(), since the device
argument is ignored for VPNs.
This broke activating VPNs from nm-applet as nm-applet sets both the
specific_object (parent-connection) and device arguments in the
activation request.
Note that we already check in _new_active_connection() that when a
device is supplied, it matches the device of the parent
connection. Therefore, the check can be dropped.
Reported-by: Michael Biebl <biebl@debian.org>
Fixes: 10753c3616https://github.com/NetworkManager/NetworkManager/pull/159
