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.
libnm exposes simplified variants of hexstr2bin in its public API. I
think that was a mistake, because libnm should provide NetworkManager
specific utils. It should not provide such string functions.
However, nmcli used to need this, so it was added to libnm.
The better approach is to add it to our internally shared static
library, so that all interested components can make use of it.
G_VARIANT_BUILDER_INIT() was only added in glib 2.50, hence we cannot use
it.
Maybe nm-glib.h should provide a compat macro, but the macro relies
on the magic number GVSB_MAGIC_PARTIAL, which is private to glib.
It's not clear that we can savely provide such a compat implementation
for older glib variants.
Fixes: 642f15f2f6
This API allows setting the global WFDIEs property of wpa_supplicant.
Ideally it would be better if this property was per-device, but this is
a limitation of wpa_supplicant.
Using strtol() correctly proves to be hard.
Usually, we want to also check that the end pointer is points to the end
of the string. Othewise, we silently accept trailing garbage.
For static functions inside a module, the compiler determines on its own
whether to inline the function.
Also, "inline" was used at some places that don't immediatly look like
candidates for inlining. It was most likely a copy&paste error.
When SAE key managmenet is used, the supplicant can still use the "psk"
property. Only when the pass phrase doesn't conform to WPA-PSK
limitations, the sae_password must be used.
When the supplicant does not support PMF we wrongly set
'ieee80211w=0', but since the option is not recognized the connection
fails. Don't set it when unsupported.
Fixes: a9ab50efb1
wpa_supplicant is going to change the global default for PMF from 0
(disabled) to 1 (optional) [1], so NM code needs to be adjusted to
work with all wpa_supplicant versions. Furthermore, it is better to
set optional PMF using the 'Pmf' property instead of the 'ieee80211w'
configuration option because the former better handles missing support
in driver [2].
Note that each interface in wpa_supplicant has its own copy of global
configuration and so 'global' options must still be set on each
interface. So, let's set Pmf=1 when each interface gets created and
override it with ieee80211w={0,2} if needed during association.
[1] http://lists.infradead.org/pipermail/hostap/2018-November/039009.html
[2] http://lists.infradead.org/pipermail/hostap/2019-January/039215.htmlhttps://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/104
The timeout is limited to be in the range of 1-600s. This is arbitrary,
but the point is that a timeout of 0 is not permitted to prevent a
client from making us run a find continuously simply by forgetting to
call the stop method.
wpa_supplicant will create a new interface for P2P devices. In this case
we need to fetch the supplicant interface using the object path and then
fetch the interface name via dbus to setup the IP interface of the P2P
device later.
The wpa_supplicant interface has a P2PDevice when P2P is supported.
Create a proxy for this and wait for it to be ready before marking the
interface as ready.
The signal was not handled, potentially creating corner cases where
NetworkManager may not notice an interface removal. Add a handler and
ensure the supplicant interface is brought down when it is removed from
wpa_supplicant for a reason other than NetworkManager requesting it.
To allow connections that mirror IWD's configured WPA-Enterprise
networks to be seen as valid by NM, add a new value for the eap key in
802-1x settings. 802-1x.eap stores EAP method names. In the IWD
connections we don't know what EAP method is configured and we don't
have any of the other 802-1x properties that would be required for the
settings to verify.
These connections can't be activated on devices managed by wpa_supplicant.
gboolean is a typedef for "int".
While older compilers might treat such bitfields as unsigned ([1]),
commonly such a bitfield is signed and can only contain the values 0
and -1.
We only want to use numeric 1 for TRUE, hence, creating such bitfields
is wrong, or at least error prone.
In fact, in this case it's a bug, because later we compare
it with a regular gboolean
if (priv->scanning != new_scanning)
[1] https://lgtm.com/rules/1506024027114/
Fixes: e0f9677018
Use GBytes instead of GBytesArray. GBytes is immutable and
can be shared.
It is also the type that we natively get from
nm_setting_wireless_get_ssid(). This way we avoid some
conversions.
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
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[@]}"
Use two common defines NM_BUILD_SRCDIR and NM_BUILD_BUILDDIR
for specifying the location of srcdir and builddir.
Note that this is only relevant for tests, as they expect
a certain layout of the directories, to find files that concern
them.
Coccinelle:
@@
expression a, b;
@@
-a ? a : b
+a ?: b
Applied with:
spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .
With some manual adjustments on spots that Cocci didn't catch for
reasons unknown.
Thanks to the marvelous effort of the GNU compiler developer we can now
spare a couple of bits that could be used for more important things,
like this commit message. Standards commitees yet have to catch up.
There are multiple tests with the same in different directories; add a
unique prefix to test names so that it is clear from the output which
one is running.
The g_clear_pointer() macro already contains a cast to GDestroyNotify. No
need to do it ourself. In fact, with the cast, this only works with the
particular g_clear_pointer() implementation, that first assigns the
destroy function to a local variable.
See-also: https://bugzilla.gnome.org/show_bug.cgi?id=674634#c52
We commonly only allow tabs at the beginning of a line, not
afterwards. The reason for this style is so that the code
looks formated right with tabstop=4 and tabstop=8.
