The test spawns processes and tries to kill them, with timeouts and retry.
That is inherently racy, and it's hard to deterministically test the
interesting cases, without having unstable tests.
Try to adjust the timeout, to make it more stable:
14:02:27 /builds/NetworkManager/NetworkManager/tools/run-nm-test.sh --called-from-make /builds/NetworkManager/NetworkManager/build --launch-dbus=auto /builds/NetworkManager/NetworkManager/build/src/tests/test-core-with-expect
--- stdout ---
# random seed: R02S7748fae8fc946b7a755b72efb5815250
1..5
# Start of general tests
ok 1 /general/nm_utils_monotonic_timestamp_as_boottime
# NetworkManager-DEBUG: <debug> [1601992953.4091] kill child process 'test-s-1-3' (18615): sending SIGKILL...
# NetworkManager-DEBUG: <debug> [1601992953.4242] kill child process 'test-s-1-3' (18615): waiting for process to terminate after sending no signal (0) and SIGKILL...
# NetworkManager-DEBUG: <debug> [1601992953.4257] kill child process 'test-s-1-3' (18615): after sending no signal (0) and SIGKILL, process 18615 exited by signal 9 (20807 usec elapsed)
Bail out! GLib:ERROR:../src/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process 'test-s-1-3' (*): waiting up to 1 milliseconds for process to terminate normally after sending no signal (0)...
Bail out! test:ERROR:../src/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
--- stderr ---
**
GLib:ERROR:../src/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process 'test-s-1-3' (*): waiting up to 1 milliseconds for process to terminate normally after sending no signal (0)...
**
test:ERROR:../src/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
/builds/NetworkManager/NetworkManager/tools/run-nm-test.sh: line 279: 18325 Aborted "${NMTST_DBUS_RUN_SESSION[@]}" "${NMTST_LIBTOOL[@]}" "$NMTST_VALGRIND" --quiet --error-exitcode=$VALGRIND_ERROR --leak-check=full --gen-suppressions=all "${NMTST_SUPPRESSIONS[@]}" --num-callers=100 --log-file="$LOGFILE" "$TEST" "$@"
Emitting signals is relatively expensive, because the arguments have to be packed
into a GValue. Avoid some overhad by only passing one signal argument: the notify-data
which also contains the type. Also with this we can use g_cclosure_marshal_VOID__POINTER.
Also, it's nice to have the type field part of the notify-data. Because clearly
the notify-data union is unusable without knowing the type. That means, if a user
passes the notify-data to a function, they anyway would also need to pass along
the type.
- add nm_l3cfg_platform_commit_on_idle_schedule() so that internal (and
external) code can schedule a commit on an idle handler. This already
existed, but is exposed now.
- rename nm_l3cfg_platform_commit() to simply nm_l3cfg_commit(). There
is no other form than "platform" commit, so the name was
unnecessarily long.
- also don't let nm_l3cfg_commit() return a boolean success. It's not
useful, because commits can be triggered internally (by NML3Cfg
itself) or by other users. Instead, there is the "post-commit" event,
and anybody who cares about such a failure would need to handle it
there.
Our NML3Cfg instance is the IP configuration manage of one ifindex.
Often users have an NML3Cfg instance at hand, but they still need to
react to platform signals. Instead of requiring those users to register
their own signal (which also gets notifications about uninteresting
interfaces), re-emit the signal from NML3Cfg.
We already had NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE which
does something similar, but collects multiple changes and emits them
on an idle handler.
Now that NMPlatformIP[46]Route can contain a wildcard table/metric, we
can set the effectivey table/metric per NML3ConfigData that we merge.
Pass it to nm_l3cfg_add_config().
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
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.
For simple matches like match.interface-name, match.driver, and
match.path, arguably what we had was fine. There each element
(like "eth*") is a wildcard for a single name (like "eth1").
However, for match.kernel-command-line, the elements match individual
command line options, so we should have more flexibility of whether
a parameter is optional or mandatory. Extend the syntax for that.
- the elements can now be prefixed by either '|' or '&'. This makes
optional or mandatory elements, respectively. The entire match
evaluates to true if all mandatory elements match (if any) and
at least one of the optional elements (if any).
As before, if neither '|' nor '&' is specified, then the element
is optional (that means, "foo" is the same as "|foo").
- the exclamation mark is still used to invert the match. If used
alone (like "!foo") it is a shortcut for defining a mandatory match
("&!foo").
- the backslash can now be used to escape the special characters
above. Basically, the special characters ('|', '&', '!') are
stripped from the start of the element. If what is left afterwards
is a backslash, it also gets stripped and the remainder is the
pattern. For example, "\\&foo" has the pattern "&foo" where
'&' is no longer treated specially. This special handling of
the backslash is only done at the beginning of the element (after
the optional special characters). The remaining string is part
of the pattern, where backslashes might have their own meaning.
This change is mostly backward compatible, except for existing matches
that started with one of the special characters '|', '&', '!', and '\\'.
(cherry picked from commit 824ad6275d)
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.
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
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.
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.
The `nm-default.h` header is used widely in the code by many
targets. This header includes different headers and needs different
libraries depending the compilation flags.
A new set of `*nm_default_dep` dependencies have been created to
ease the inclusion of different directorires and libraries.
This allows cleaner build files and avoiding linking unnecessary
libraries so this has been applied allowing the removal of some
dependencies involving the linking of unnecessary libraries.
These are deprecated. Also, they are nowadays implemented as macros
that expand to
#define g_main_run(loop) g_main_loop_run(loop) GLIB_DEPRECATED_MACRO_IN_2_26_FOR(g_main_loop_run)
This can cause compilation failure (in some environments).
If the @append_force argument is set and the object is already in the
list, it must be moved at the end.
Fixes: 22edeb5b69 ('core: track addresses for NMIP4Config/NMIP6Config via NMDedupMultiIndex')
Add test to show a wrong result of ip_ipX_config_replace() due to a
bug in _nm_ip_config_add_obj(). When an address is added to the tail
of the index and another address with the same id already exists, the
existing object is left at the same place, breaking the order of
addresses.
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.
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.
Don't create a heap allocated GString to hold the static
result of nm_logging_all_domains_to_string().
Instead, use a static buffer of the exactly required size.
The main reason to do this, is to get the exact size of
"_all_logging_domains_to_str" buffer. This is the upper
boundary for the size of a string buffer to hold all domain
names.
We will need that boundary in the next commit. The attractive
thing here is that we will have a unit-test failure if this
boundery no longer matches (--with-more-asserts). That means,
this boundary is guarded by unit tests and we don't accidentally
get it wrong when the domains change.
Also, take care to initialize the buffer in a thread-safe manner.
It's easy enough to get right, so there is no excuse for having
non-thread-safe code in logging.
The library is called "libnm_core". So the dependency should be called
"libnm_core_dep", like in all other cases.
(cherry picked from commit c27ad37c27)
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.
The test should check the behavior with "const typeof(a)" in a macro,
where "a" itself is const. For that we don't need a double const
declaration of v2.
Also, that fixes an actual compiler warning:
../src/tests/test-general.c: In function ‘test_duplicate_decl_specifier’:
../src/tests/test-general.c:1669:8: warning: duplicate ‘const’ declaration specifier [-Wduplicate-decl-specifier]
const const int v2 = 3;
^~~~~
NetworkManager is single-threaded and uses a mainloop.
However, sometimes we may need multiple threads. For example, we will
need to write sysctl values asynchronously, using the glib thread-pool.
For that to work, we also need to switch the network-namespace of the
thread-pool thread. We want to use NMPNetns for that. Hence it's better
to have NMPNetns thread-safe, instead of coming up with a duplicate
implementation. But NMPNetns may want to log, so we also need nm-logging
thread-safe.
In general, code under "shared/nm-utils" and nm-logging should be usable
from multiple threads. It's simpler to make this code thread-safe than
re-implementing it. Also, it's a bad limitation to be unable to log
from other threads. If there is an error, the best we can often do is to
log about it.
Make nm-logging thread-safe. Actually, we only need to be able to log
from multiple threads. We don't need to setup or configure logging from
multiple threads. This restriction allows us to access logging from the
main-thread without any thread-synchronization (because all changes in
the logging setup are also done from the main-thread).
So, while logging from other threads requires a mutex, logging from the
main-thread is lock-free.
The only purpose of using alloca() to avoid the overhead of heap-allocation
and possible save a line in source code for managing/freeing the heap allocation.
For tests we don't care about performance, and (in this case)
the code does not get any shorter.
Avoid alloca() in tests, because alloca() is something to search for
when reviewing code for stack overflows. No need to have such false
positives show up in tests.
