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.
While at it, rename the "addr" field to "l_address". The term "addr" is
used over and over. Instead we should use distinct names that make it
easier to navigate the code.
nmtst_get_rand_int() was originally named that way, because it
calls g_rand_int(). But I think if a function returns an uint32, it
should also be named that way.
Rename.
For the most part, we only have one main .gitignore file.
There were a few nested files, merge them into the main file.
I find it better to have only one gitignore file, otherwise the
list of ignored files is spread out through the working directory.
Older versions of meson don't like building multiple artifacts
with the same name (even if they are in different directories). We
have multiple tests called "test-general.c", and it would be natural
to compile a test binary of the same name.
Meson encountered an error in file src/tests/meson.build, line 14, column 2:
Tried to create target "test-general", but a target of that name already exists.
It's generally a bad idea to have in our source tree multiple files with the
same name. Rename the test.
Fixes: 16cd84d346 ('build/meson: rename platform tests to use same name as autotools'):
First of all, all file names in our source-tree should be unique. We should
not have stuff like "libnm-core/tests/test-general.c" and "src/tests/test-general.c".
The problem here are the C source files, and consequently also the test
binaries have duplicate names. We should avoid that in general. However,
our binaries should have a matching name with the C source. If
"test-general.c" is not good enough, that needs renaming. Not building
"platform-test-general" out of it.
On the other hand, all our tests should have a filename "*/tests/test-*", like
they do for autotools.
Rename the meson platform tests.
It's also important because "tools/run-nm-test.sh" relies on the test
name to workaround valgrind warnings.
On Ubuntu 14.04 kernel (4.4.0-146-generic, x86_64) this easily causes
test failures:
make -j 8 src/platform/tests/test-route-linux \
&& while true; do \
NMTST_SEED_RANDOM= ./tools/run-nm-test.sh src/platform/tests/test-route-linux -p /route/rule \
|| break; \
done
outputs:
...
/route/rule/1:
nmtst: initialize nmtst_get_rand() with NMTST_SEED_RAND=22892021
OK
/route/rule/2: >>> failing...
>>> no fuzzy match between: [routing-rule,0x205ab30,1,+alive,+visible; [6] 0: from all suppress_prefixlen 8 none]
>>> and: [routing-rule,0x205c0c0,1,+alive,+visible; [6] 0: from all suppress_prefixlen -1579099242 none]
**
test:ERROR:src/platform/tests/test-route.c:1695:test_rule: code should not be reached
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
We built (among others) two libraries from the sources in "shared/nm-utils":
"libnm-utils-base.la" and "libnm-utils-udev.la".
It's confusing. Instead use directories so there is a direct
correspondence between these internal libraries and the source files.
(cherry picked from commit 2973d68253)
Next we will need to detect more kernel features. First refactor the
handling of these to require less code changes and be more efficient.
A plain nm_platform_kernel_support_get() only reqiures to access an
array in the common case.
The other important change is that the function no longer requires a
NMPlatform instance. This allows us to check kernel support from
anywhere. The only thing is that we require kernel support to be
initialized before calling this function. That means, an NMPlatform
instance must have detected support before.
(cherry picked from commit ee269b318e)
All that setting track-default does, is calling nmp_rules_manager_track_default()
when the rules are first accessed.
That is not right API. Since nmp_rules_manager_track_default() is already public
API (good), every caller that wishes this behavior should track these routes explicitly.
Seems on a busy system, we can hit this timeout. Increase it.
ERROR:../src/platform/tests/test-common.c:939:_ip_address_add: code should not be reached
# NetworkManager-MESSAGE: <warn> [1553100541.6609] platform-linux: do-add-rule: failure 17 (File exists)
>>> failing... errno=-17, rule=[routing-rule,0xe9c540,1,+alive,+visible; [6] 4294967295: from all suppress_prefixlen 3 none goto-target 2955537847]
0: from all to 73.165.79.8/2 iif nm-test-device 178
0: from all 109
0: from all tos 0x13 lookup 10004 suppress_prefixlength 0 none
0: from all none
4294967295: not from all none
test:ERROR:../src/platform/tests/test-route.c:1607:test_rule: assertion failed (r == 0): (-17 == 0)
Possibly fixed by https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c8f4e6dc30996bff806285730a0bb4e714d3d52
The routing-rule tests generate a number of routing rules and tries to
add and delete them.
For that, _rule_create_random() sets random fields of the rule.
Note that especially interesting are rules that leave most fields
unset (at zero), because they trigger kernel issues rh#1686075 and
rh#1685816.
But a rule has many fields, so in order to generate rules that have most
fields unset, we need to use low probabilities when rolling the dice for
setting a field. Otherwise, most rules end up with several fields set
and don't reproduce the kernel issue (especially the test failed to hit
rh#1686075).
Routing rules are unlike addresses or routes not tied to an interface.
NetworkManager thinks in terms of connection profiles. That works well
for addresses and routes, as one profile configures addresses and routes
for one device. For example, when activating a profile on a device, the
configuration does not interfere with the addresses/routes of other
devices. That is not the case for routing rules, which are global, netns-wide
entities.
When one connection profile specifies rules, then this per-device configuration
must be merged with the global configuration. And when a device disconnects later,
the rules must be removed.
Add a new NMPRulesManager API to track/untrack routing rules. Devices can
register/add there the routing rules they require. And the sync method will
apply the configuration. This is be implemented on top of NMPlatform's
caching API.
Until now, all implemented NMPObject types have an ifindex field (from
links, addresses, routes, qdisc to tfilter).
The NMPObject structure contains a union of all available types, that
makes it easier to down-case from an NMPObject pointer to the actual
content.
The "object" field of NMPObject of type NMPlatformObject is the lowest
common denominator.
We will add NMPlatformRoutingRules (for policy routing rules). That type
won't have an ifindex field.
Hence, drop the "ifindex" field from NMPlatformObject type. But also add
a new type NMPlatformObjWithIfindex, that can represent all types that
have an ifindex.
Older versions of meson don't support building the same names
multiple times.
Meson encountered an error in file src/tests/meson.build, line 14, column 2:
Tried to create target "test-general", but a target of that name already exists.
We really need to use unique filenames everywhere. Revert the name
change for now.
This breaks again the valgrind workaround in "tools/run-nm-test.sh".
This reverts commit 5466edc63e.
Meson and autotools should name the tests the same way.
Also, all tests binaries built by autotools start on purpose
with "test-". Do that for meson too.
Also, otherwise "tools/run-nm-test.sh" fails to workaround
valgrind failures for platform tests as it does not expect
the tests to be named that way:
if [ $HAS_ERRORS -eq 0 ]; then
# valgrind doesn't support setns syscall and spams the logfile.
# hack around it...
if [ "$TEST_NAME" = 'test-link-linux' -o \
"$TEST_NAME" = 'test-acd' ]; then
if [ -z "$(sed -e '/^--[0-9]\+-- WARNING: unhandled .* syscall: /,/^--[0-9]\+-- it at http.*\.$/d' "$LOGFILE")" ]; then
HAS_ERRORS=1
fi
fi
fi
The defaults for test timeouts in meson is 30 seconds. That is not long
enough when running
$ NMTST_USE_VALGRIND=1 ninja -C build test
Note that meson supports --timeout-multiplier, and automatically
increases the timeout when running under valgrind. However, meson
does not understand that we are running tests under valgrind via
NMTST_USE_VALGRIND=1 environment variable.
Timeouts are really not expected to be reached and are a mean of last
resort. Hence, increasing the timeout to a large value is likely to
have no effect or to fix test failures where the timeout was too rigid.
It's unlikely that the test indeed hangs and the increase of timeout
causes a unnecessary increase of waittime before aborting.
We will need more flags.
WireGuard internal tools solve this by embedding the change flags inside
the structure that corresponds to NMPlatformLnkWireGuard. We don't do
that, NMPlatformLnkWireGuard is only for containing the information about
the link.
NMPNetns instances are immutable, hence they can be easily shared
between threads. All we need, is that the stack of namespaces is
thread-local.
Also note that NMPNetns uses almost no other API, except some bits from
"shared/nm-utils/" and nm-logging. These parts are already supposed to
be thread-safe.
The only complications is that when the thread exits, we need to
destroy the NMPNetns instances. That is especially important because
they hold file descriptors. This is accomplished using pthread's
thread-specific data. An alternative would be C11 threads' tss_create(),
but not all systems that we run against support that yet. This means,
we need to link with pthreads, but we already do that anyway.
Note that glib also requires pthreads. So, we don't get an additional
dependency here.
The caller may not wish to replace existing peers, but only update/add
the peers explicitly passed to nm_platform_link_wireguard_change().
I think that is in particular interesting, because for the most part
NetworkManager will configure the same set of peers over and over again
(whenever we resolve the DNS name of an IP endpoint of the WireGuard
peer).
At that point, it seems disruptive to drop all peers and re-add them
again. Setting @replace_peers to %FALSE allows to only update/add.
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from <errno.h>, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from <errno.h> or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.
This is also what systemd does, except that systemd only returns
(negative) integers from <errno.h>, while we merge our own error codes
into the range of <errno.h>.
The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from <errno.h>, but it did not entail error codes from netlink.
While nm_utils_inet*_ntop() accepts a %NULL buffer to fallback
to a static buffer, don't do that.
I find the possibility of using a static buffer here error prone
and something that should be avoided. There is of course the downside,
that in some cases it requires an additional line of code to allocate
the buffer on the stack as auto-variable.
We want that all code paths assert strictly and gracefully.
That means, if we have function nm_platform_link_get() which calls
nm_platform_link_get_obj(), then we don't need to assert the same things
twice. Don't have the calling function assert itself, if it is obvious
that the first thing that it does, is calling a function that itself
asserts the same conditions.
On the other hand, it simply indicates a bug passing a non-positive
ifindex to any of these platform functions. No longer let
nm_platform_link_get_obj() handle negative ifindex gracefully. Instead,
let it directly pass it to nmp_cache_lookup_link(), which eventually
does a g_return_val_if_fail() check. This quite possible enables
assertions on a lot of code paths. But note that g_return_val_if_fail()
is graceful and does not lead to a crash (unless G_DEBUG=fatal-criticals
is set for debugging).
nm_platform_link_delete() will soon assert against positive ifindex
argument.
nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME));
will result in an assertion, if the link does not exist.
Extend nmtstp_link_delete() to gracefully skip deleting the link
so that it can be used in such situations.
Also, rename nmtstp_link_del() to nmtstp_link_delete(), because it's
closer to nm_platform_link_delete().
Sometimes the test fail:
$ make -j 10 src/platform/tests/test-address-linux
$ while true; do
NMTST_DEBUG=d ./tools/run-nm-test.sh src/platform/tests/test-address-linux 2>&1 > log.txt || break;
done
fails with:
ERROR: src/platform/tests/test-address-linux - Bail out! test:ERROR:src/platform/tests/test-common.c:790:nmtstp_ip_address_assert_lifetime: assertion failed (adr <= lft): (1001 <= 1000)
That is, because of a wrong check. Fix it.
In the past, the headers "linux/if.h" and "net/if.h" were incompatible.
That means, we can either include one or the other, but not both.
This is fixed in the meantime, however the issue still exists when
building against older kernel/glibc.
That means, including one of these headers from a header file
is problematic. In particular if it's a header like "nm-platform.h",
which itself is dragged in by many other headers.
Avoid that by not including these headers from "platform.h", but instead
from the source files where needed (or possibly from less popular header
files).
Currently there is no problem. However, this allows an unknowing user to
include <net/if.h> at the same time with "nm-platform.h", which is easy
to get wrong.
