The code was passing the gpointer alias of the GValue, rather than the
GValue* itself. This doesn’t matter normally, but broke an experimental
patch in GLib to remove a cast from G_VALUE_TYPE.
We’ve reverted the patch in GLib (see
https://bugzilla.gnome.org/show_bug.cgi?id=793186), but this should be
fixed in NetworkManager anyway.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://bugzilla.gnome.org/show_bug.cgi?id=793302
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.
NM_API_VERSION is a better name. It's not the current stable
version, but the version number of the API which the current
NM_VERSION provides. In practice, NM_API_VERSION is either identical
to NM_VERSION (in case of a release) or NM_VERSION is a development
version leading up the the upcoming NM_API_VERSION.
For example, with the new name the check
#if NM_VERSION != NM_API_VERSION
# warning this is an development version
#endif
makes more sense.
We don't need to have two version defines "CUR" and "NEXT".
The main purpose of these macros (if not their only), is to
make NM_AVAILABLE_IN_* and NM_DEPRECATED_IN_* macros work.
1) At the precise commit of a release, "CUR" and "NEXT" must be identical,
because whenever the user configures NM_VERSION_MIN_REQUIRED and
NM_VERSION_MAX_ALLOWED, then they both compare against the current
version, at which point "CUR" == "NEXT".
2) Every other commit aside the release, is a development version that leads
up the the next coming release. But as far as versioning is concerned, such
a development version should be treated like that future release. It's unstable
API and it may or may not be close to later API of the release. But
we shall treat it as that version. Hence, also in this case, we want to
set both "NM_VERSION_CUR_STABLE" and again NEXT to the future version.
This makes NM_VERSION_NEXT_STABLE redundant.
Previously, the separation between current and next version would for
example allow that NM_VERSION_CUR_STABLE is the previously release
stable API, and NM_VERSION_NEXT_STABLE is the version of the next upcoming
stable API. So, we could allow "examples" to make use of development
API, but other(?) internal code still restrict to unstable API. But it's
unclear which other code would want to avoid current development.
Also, the points 1) and 2) were badly understood. Note that for our
previousy releases, we usually didn't bump the macros at the stable
release (and if we did, we didn't set them to be the same). While using
two macros might be more powerful, it is hard to grok and easy to
forget to bump the macros a the right time. One macro shall suffice.
All this also means, that *immediately* after making a new release, we shall
bump the version number in `configure.ac` and "NM_VERSION_CUR_STABLE".
We also unconditionally use them with autotools.
Also, the detection for have_version_script does
not seem correct to me. At least, it didn't work
with clang.
The strings holding the names used for libraries have also been
moved to different variables. This way they would be less error
as these variables can be reused easily and any typing error
would be quickly detected.
Some targets are missing dependencies on some generated sources in
the meson port. These makes the build to fail due to missing source
files on a highly parallelized build.
These dependencies have been resolved by taking advantage of meson's
internal dependencies which can be used to pass source files,
include directories, libraries and compiler flags.
One of such internal dependencies called `core_dep` was already in
use. However, in order to avoid any confusion with another new
internal dependency called `nm_core_dep`, which is used to include
directories and source files from the `libnm-core` directory, the
`core_dep` dependency has been renamed to `nm_dep`.
These changes have allowed minimizing the build details which are
inherited by using those dependencies. The parallelized build has
also been improved.
These files are a template how to add a new nm-setting-* implementation.
We are not going to add new files to the deprecated libnm-util library,
hence a template for libnm-util is pointless.
libnm-core doesn't have a corresponding template file. Personally, I
don't think such a template are a great idea either, because
- People are not aware that it exists. Hence, it's unused, badly
maintained and quite possibly does not follow current best practice.
- Just copy an actual settings implementation and start from there.
That seems better to me than having a template.
Tests are commonly created via copy&paste. Hence, it's
better to express a certain concept explicitly via a function
or macro. This way, the implementation of the concept can be
adjusted at one place, without requiring to change all the callers.
Also, the macro is shorter, and brevity is better for tests
so it's easier to understand what the test does. Without being
bothered by noise from the redundant information.
Also, the macro knows better which message to expect. For example,
messages inside "src" are prepended by nm-logging.c with a level
and a timestamp. The expect macro is aware of that and tests for it
#define NMTST_EXPECT_NM_ERROR(msg) NMTST_EXPECT_NM (G_LOG_LEVEL_MESSAGE, "*<error> [*] "msg)
This again allows the caller to ignore this prefix, but still assert
more strictly.
Note that:
- we compile some source files multiple times. Most notably those
under "shared/".
- we include a default header "shared/nm-default.h" in every source
file. This header is supposed to setup a common environment by defining
and including parts that are commonly used. As we always include the
same header, the header must behave differently depending
one whether the compilation is for libnm-core, NetworkManager or
libnm-glib. E.g. it must include <glib/gi18n.h> or <glib/gi18n-lib.h>
depending on whether we compile a library or an application.
For that, the source files need the NETWORKMANAGER_COMPILATION #define
to behave accordingly.
Extend the define to be composed of flags. These flags are all named
NM_NETWORKMANAGER_COMPILATION_WITH_*, they indicate which part of the
build are available. E.g. when building libnm-core.la itself, then
WITH_LIBNM_CORE, WITH_LIBNM_CORE_INTERNAL, and WITH_LIBNM_CORE_PRIVATE
are available. When building NetworkManager, WITH_LIBNM_CORE_PRIVATE
is not available but the internal parts are still accessible. When
building nmcli, only WITH_LIBNM_CORE (the public part) is available.
This granularily controls the build.
Source files for enum types are generated by passing segments of the
source code of the files to the `glib-mkenums` command.
This patch removes those parameters where source code is used from
meson build files by moving those segmeents to template files.
https://mail.gnome.org/archives/networkmanager-list/2017-December/msg00057.html
There are some tests located in different directories which are
using the same name. To avoid any confussion a prefix was used to
name the test and the target.
This patch uses the prefix just for the target, to avoid any
collision that may happen, and uses the `test-` pattern as the
name.
https://mail.gnome.org/archives/networkmanager-list/2017-December/msg00051.html
The `libnm_linking` test that belongs to the libnm-util's general
tests is failing because the test is not able to find the
`test-libnm-linking` binary, which is executed as a child process.
The problem lies to the `BUILD_DIR` macro definition which is
used to set the place to find the binary, and is wrongly defined
with the source directory.
This patch changes its value to the build directory that fixes
the problem.
https://mail.gnome.org/archives/networkmanager-list/2017-December/msg00049.html
30. NetworkManager-1.9.2/src/settings/plugins/keyfile/nms-keyfile-writer.c:218:
check_return: Calling "g_mkdir_with_parents" without checking return
value (as is done elsewhere 4 out of 5
times).
25. NetworkManager-1.9.2/src/platform/nm-linux-platform.c:3969:
check_return: Calling "_nl_send_nlmsg" without checking return value (as
is done elsewhere 4 out of 5 times).
34. NetworkManager-1.9.2/src/nm-core-utils.c:2843:
negative_returns: "fd2" is passed to a parameter that cannot be negative.
26. NetworkManager-1.9.2/src/devices/wwan/nm-modem-broadband.c:897:
check_return: Calling "nm_utils_parse_inaddr_bin" without checking
return value (as is done elsewhere 4 out of 5 times).
3. NetworkManager-1.9.2/src/devices/bluetooth/nm-bluez5-manager.c:386:
check_return: Calling "g_variant_lookup" without checking return value
(as is done elsewhere 79 out of 83 times).
16. NetworkManager-1.9.2/libnm-util/nm-setting.c:405:
check_return: Calling "nm_g_object_set_property" without checking return
value (as is done elsewhere 4 out of 5 times).
rpmdiff complains about uses of inet_aton, inet_makeaddr, inet_netof,
inet_ntoa under the IPv6 section:
usr/sbin/NetworkManager on aarch64 i686 x86_64 ppc ppc64 ppc64le s390 s390x uses function inet_aton, which may impact IPv6 support
I think the warning is bogus, but refactor our code to avoid it.
Note that systemd code still uses them, so it don't avoid the rpmdiff
warning. But let's not diverge our systemd import from upstream for this.
- for NMSettingBond:validate_ip() also avoid g_strsplit_set() which
allocates a full strv. Instead, we can do with one g_strdup().
- for test-resolvconf-capture.c, replace the functions with macros.
Macros should be avoided usually, but for test asserts they are
more convenient as they preserved the __FILE__:__LINE__ of where
the assertion fails.
There are very few places where we actually use floating point
or #include <math.h>.
Drop that library, although we very likely still get it as indirect
dependency (e.g. on my system it is still dragged in by libsystemd.so,
libudev.so and libnl-3.so).
In practice, this should only matter when there are multiple
header files with the same name. That is something we try
to avoid already, by giving headers a distinct name.
When building NetworkManager itself, we clearly want to use
double-quotes for including our own headers.
But we also want to do that in our public headers. For example:
./a.c
#include <stdio.h>
#include <nm-1.h>
void main() {
printf ("INCLUDED %s/nm-2.h\n", SYMB);
}
./1/nm-1.h
#include <nm-2.h>
./1/nm-2.h
#define SYMB "1"
./2/nm-2.h
#define SYMB "2"
$ cc -I./2 -I./1 ./a.c
$ ./a.out
INCLUDED 2/nm-2.h
Exceptions to this are
- headers in "shared/nm-utils" that include <NetworkManager.h>. These
headers are copied into projects and hence used like headers owned by
those projects.
- examples/C
priv->path_cost and priv->priority can only be set as GObject properties,
which already does the same range check. Hence, the checks are never reached.
This also avoids a compiler warning:
libnm-core/nm-setting-bridge-port.c: In function ‘verify’:
libnm-core/nm-setting-bridge-port.c:132:22: error: comparison is always false due to limited range of data type [-Werror=type-limits]
if (priv->path_cost > BR_MAX_PATH_COST) {
^
The -Wimplicit-fallthrough=3 warning is quite flexible of accepting
a fall-through warning.
Some comments were missing or not detected correctly.
Thereby, also change all other comments to follow the exact
same pattern.
Previously, due to a bug in "tools/enums-to-docbook.pl", enum values
without explicit numeric value were wrongly parsed. That is fixed,
but still explicitly set the value in the public header.
IPv4 already allows setting an address, reusing its prefix for the network
it shares connection with. Additionally, for IPv6, the NDP can also share
the DNS configuration.
We should enable tests by default, probably we even should drop
the configure flags to enable tests and just always build them.
Anyway, at this point there is no use in guarding check-local
with a check for ENABLE_TESTS. A user who does't want to run
the tests, should just not call `make check`.
crypto_verify_private_key_data() must try to decrypt the key only when
a password is supplied.
Previously the decrypt test always passed because we detected an
unsupported cipher and faked success. Now since version 3.5.4 gnutls
supports PBES1-DES-CBC-MD5 and the key is actually decrypted when a
password is supplied.
Also, don't assert that a wrong password works because we're now able
to actually verify it (only with recent gnutls).
https://bugzilla.gnome.org/show_bug.cgi?id=771623
- don't include "nm-default.h" in header files. Every source file must
include as first header "nm-default.h", thus our headers get the
default include already implicitly.
- we don't support compiling NetworkManager itself with a C++ compiler. Remove
G_BEGIN_DECLS/G_END_DECLS from internal headers. We do however support
users of libnm to use C++, thus they stay in public headers.
We use statement expressions all over the place without explicitly
marking them. If that would be a problem, we'd have to change a
*lot* of code. We simply require that as a mandatory feature from
our compiler.
The "shared" directory contains files that are possibly used by all components
of NetworkManager repository.
Some of these files are even copied as-is to other projects (VPN plugins, nm-applet)
and used there without modification. Move those files to a separate directory.
By moving them to a common directory, it is clearer that they belong
together. Also, you can easier compare the copied versions to their
original via
$ diff -r ./shared/nm-utils/ /path/to/nm-vpn-plugin/shared/nm-utils/
A failure to g_return*() by default prints a g_critical() with stringifing the
condition. Add a macro NMTST_G_RETURN_MSG() that reproduces that line to more
accurately match the failure message.
The linking test causes a crash to check whether libnm and libnm-util
are both linked. If abrt or systemd-coredump are enabled, the core
dump processing will take a long time when the address sanitizer is
enabled, due to the huge process address space. It seems a good choice
to disable the test when NM was compiled with -fsanitize=address.
