The kernel of Ubuntu 16.04 doesn't support IFLA_BR_VLAN_STATS_ENABLED.
If we want to run on such old kernels (which we probably do), we need to
detect that, and act accordingly.
Older versions of iproute2 (Ubuntu 16.04) don't support all the requested
bridge options. We need to gracefully ignore a failure and try with our
own implementation.
While doing that, only set the command line arguments if they are
necessary (that is, if they requested value is not the default already).
Luckily, Ubuntu 16.04's kernel supports these properties just fine, so
we can avoid complicated compatibility code to cope with missing kernel
support. It's really just an iproute2 limitation and affects only the
tests.
This makes the macro more function like. Also, taking a pointer
makes it a bit clearer that this possibly changes the value.
Of course, it's not a big difference to before, but this
form seems slightly preferable to me.
They ensure to consistently return -1, 0, 1. Also, I think they are
easier to understand.
What is in general hard to understand, whether a comparison sorts
ascending or descending. The macros maybe make that easier too, but it's
still confusing. That's why we have a test.
Userspace cannot add IPv6 routes with metric 0. Trying to do that, will
be coerced by kernel to route metric 1024. For IPv4 this is different,
and metric zero is commonly allowed.
However, kernel itself can add IPv6 routes with metric zero:
# ip -6 route show table local
local fe80::2029:c7ff:fec9:698a dev v proto kernel metric 0 pref medium
That means, we must not treat route metric zero special for most cases.
Only, when we want to add routes (based on user configuration), we must
coerce a route metric of zero to 1024.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/563
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
and _nm_utils_inet6_ntop() instead of nm_utils_inet6_ntop().
nm_utils_inet4_ntop()/nm_utils_inet6_ntop() are public API of libnm.
For one, that means they are only available in code that links with
libnm/libnm-core. But such basic helpers should be available everywhere.
Also, they accept NULL as destination buffers. We keep that behavior
for potential libnm users, but internally we never want to use the
static buffers. This patch needs to take care that there are no callers
of _nm_utils_inet[46]_ntop() that pass NULL buffers.
Also, _nm_utils_inet[46]_ntop() are inline functions and the compiler
can get rid of them.
We should consistently use the same variant of the helper. The only
downside is that the "good" name is already taken. The leading
underscore is rather ugly and inconsistent.
Also, with our internal variants we can use "static array indices in
function parameter declarations" next. Thereby the compiler helps
to ensure that the provided buffers are of the right size.
- track the broadcast address in NMPlatformIP4Address. For addresses
that we receive from kernel and that we cache in NMPlatform, this
allows us to show the additional information. For example, we
can see it in debug logging.
- when setting the address, we still mostly generate our default
broadcast address. This is done in the only relevant caller
nm_platform_ip4_address_sync(). Basically, we merely moved setting
the broadcast address to the caller.
That is, because no callers explicitly set the "use_ip4_broadcast_address"
flag (yet). However, in the future some caller might want to set an explicit
broadcast address.
In practice, we currently don't support configuring special broadcast
addresses in NetworkManager. Instead, we always add the default one with
"address|~netmask" (for plen < 31).
Note that a main point of IFA_BROADCAST is to add a broadcast route to
the local table. Also note that kernel anyway will add such a
"address|~netmask" route, that is regardless whether IFA_BROADCAST is
set or not. Hence, setting it or not makes very little difference for
normal broadcast addresses -- because kernel tends to add this route either
way. It would make a difference if NetworkManager configured an unusual
IFA_BROADCAST address or an address for prefixes >= 31 (in which cases
kernel wouldn't add them automatically). But we don't do that at the
moment.
So, while what NM does has little effect in practice, it still seems
more correct to add the broadcast address, only so that you see it in
`ip addr show`.
In several cases, the layer 2 and layer 3 type are very similar, also from
kernel's point of view. For example, "gre"/"gretap" and "ip6tnl"/"ip6gre"/"ip6gretap"
and "macvlan"/"macvtap".
While it makes sense that these have different NMLinkType types
(NM_LINK_TYPE_MACV{LAN,TAP}) and different NMPObject types
(NMPObjectLnkMacv{lan,tap}), it makes less sense that they have
different NMPlatformLnk* structs.
Remove the NMPlatformLnkMacvtap typedef. A typedef does not make things simpler,
but is rather confusing. Because several API that we would usually have, does
not exist for the typedef (e.g. there is no nm_platform_lnk_macvtap_to_string()).
Note that we also don't have such a typedef for NMPlatformLnkIp6Tnl
and NMPlatformLnkGre, which has the same ambiguity between the link type
and the struct with the data.
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
This is necessary on Travis/Ubuntu 16.04, otherwise the test
fails with
# NetworkManager-MESSAGE: <warn> [1575301791.7600] platform-linux: do-add-link[nm-test-device/team]: failure 95 (Operation not supported)
Aborted (core dumped)
# test:ERROR:../src/platform/tests/test-link.c:353:test_software: assertion failed: (software_add (link_type, DEVICE_NAME))
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
The targets that involve the use of the `NetworkManager` library,
built in the `src` build file have been improved by applying a set
of changes:
- Indentation has been fixed.
- Set of objects used in targets have been grouped together.
- Aritificial dependencies used to group dependencies and custom
compiler flags have been removed and their use replaced with
proper dependencies and compiler flags to avoid any confussion.
... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
Don't mix negative errno return value with a GError output. Instead,
return a boolean result indicating success or failure.
Also, optionally
- output GError
- set out_errsv to the positive errno (or 0 on success)
Obviously, the return value and the output arguments (contents, length,
out_errsv, error) must all agree in their success/failure result.
That means, you may check any of the return value, out_errsv, error, and
contents to reliably detect failure or success.
Also note that out_errsv gives the positive(!) errno. But you probably
shouldn't care about the distinction and use nm_errno_native() either
way to normalize the value.
Seen on gitlab-ci.
NMPlatformSignalAssert: ../src/platform/tests/test-link.c:260, test_slave(): failure to accept signal [0,2] times: link-changed-changed ifindex 15 (3 times received)
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
ERROR: src/platform/tests/test-link-linux - exited with status 133 (terminated by signal 5?)
(cherry picked from commit 483de2bb93)
IP addresses, routes, TC and QDiscs are all tied to a certain interface.
So when NetworkManager manages an interface, it can be confident that
all related entires should be managed, deleted and modified by NetworkManager.
Routing policy rules are global. For that we have NMPRulesManager which
keeps track of whether NetworkManager owns a rule. This allows multiple
connection profiles to specify the same rule, and NMPRulesManager can
consolidate this information to know whether to add or remove the rule.
NMPRulesManager would also support to explicitly block a rule by
tracking it with negative priority. However that is still unused at
the moment. All that devices do is to add rules (track with positive
priority) and remove them (untrack) once the profile gets deactivated.
As rules are not exclusively owned by NetworkManager, NetworkManager
tries not to interfere with rules that it knows nothing about. That
means in particular, when NetworkManager starts it will "weakly track"
all rules that are present. "weakly track" is mostly interesting for two
cases:
- when NMPRulesManager had the same rule explicitly tracked (added) by a
device, then deactivating the device will leave the rule in place.
- when NMPRulesManager had the same rule explicitly blocked (tracked
with negative priority), then it would restore the rule when that
block gets removed (as said, currently nobody actually does this).
Note that when restarting NetworkManager, then the device may stay and
the rules kept. However after restart, NetworkManager no longer knows
that it previously added this route, so it would weakly track it and
never remove them again.
That is a problem. Avoid that, by whenever explicitly tracking a rule we
also make sure to no longer weakly track it. Most likely this rule was
indeed previously managed by NetworkManager. If this was really a rule
added by externally, then the user really should choose distinct
rule priorities to avoid such conflicts altogether.
nmtst: initialize nmtst_get_rand() with NMTST_SEED_RAND=0
/link/bogus: OK
/link/loopback: OK
/link/internal: OK
/link/external: OK
/link/software/bridge: OK
/link/software/bond: OK
/link/software/team: NMPlatformSignalAssert: ../src/platform/tests/test-link.c:331, test_slave(): failure to accept signal [0,2] times: 'link-changed-changed' ifindex 15 (3 times received)
--- stderr ---
/builds/NetworkManager/NetworkManager/tools/run-nm-test.sh: line 264: 106682 Trace/breakpoint trap --quiet --error-exitcode= --leak-check=full --gen-suppressions=all --num-callers=100 --log-file=
The test failed. Also check the valgrind log at '/builds/NetworkManager/NetworkManager/build/src/platform/tests/test-link-linux.valgrind-log'
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
