When doing announcements, use the the timeout specified by RFC
5227. Note that timeout_multiplier might be 0.
This aligns behavior to upstream version of n-acd.
At various places we sort our D-Bus paths. For example,
server sorts them before exporting them on D-Bus.
Server knows well, that a lot of these paths are build
by attaching an incrementing number as last component.
It looks nicer to sort by this number, instead of strictly
lexical with strcmp().
Note that this handles the cases correctly where paths have
different prefixes, or where they don't end with a number.
tools/test-networkmanager-service.py is our NetworkManager stub server.
NetworkManager uses libnm(-core) heavily, for example to decide whether
a connection verifies (nm_connection_verify()) and for normalizing
connections (nm_connection_normalize()).
If the stub server wants to mimic NetworkManager, it also must use these
function. Luckily, we already can do so, by loading libnm using python
GObject introspection.
We already correctly set GI_TYPELIB_PATH search path, so that the
correct libnm is loaded -- provided that we build with introspection
enabled.
We still need to gracefully fail, if starting the stub server fails.
That requries some extra effort. If the stub server notices that
something is missing, it shall exit with status 77. That will cause
the tests to g_test_skip().
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.
It is meant to be rather similar in nature to isblank() or
g_ascii_isspace().
Sadly, isblank() is locale dependent while g_ascii_isspace() also considers
vertical whitespace as a space. That's no good for configuration files that
are strucutured into lines, which happens to be a pretty common case.
...so that its prototype is compatible with GDestroyNotify:
src/devices/nm-acd-manager.c: In function ‘destroy_address_info’:
/usr/include/glib-2.0/glib/gmem.h:120:31: error: cast between incompatible function types from ‘NAcd * (*)(NAcd *)’ {aka ‘struct NAcd * (*)(struct NAcd *)’} to ‘void (*)(void *)’ [-Werror=cast-function-type]
GDestroyNotify _destroy = (GDestroyNotify) (destroy); \
^
src/devices/nm-acd-manager.c:430:2: note: in expansion of macro ‘g_clear_pointer’
g_clear_pointer (&info->acd, n_acd_free);
^~~~~~~~~~~~~~~
The same change was done upstream, so the subsequent subtree pull of n-acd
won't mess this up.
For one, these functions are not often needed. No need to define them in the
"nm-macros-internal.h" header, which is included everywhere. Move them to
"nm-shared-utils.h", which must be explicitly included.
Also, these functions are usually not called directly, but by passing their
function pointer to a sort function or similar. There is no point in having
defined in the header file.
If the main loop is quit before the timeout expires, we leave the
timeout source running on the main loop context. Since we usually
create the main loop using the default context, the source will fire
on the next main loop we create during the test.
Therefore, destroy the timeout source if it is still active.
Fixes: 766f31507b
The README states that a kernel >= 3.0 is enough, however
CLOCK_BOOTTIME is only available since kernel 3.15.
Fall back to CLOCK_MONOTONIC when CLOCK_BOOTTIME is not available.
See: https://github.com/nettools/n-acd/pull/3
Signed-off-by: Beniamino Galvani <bgalvani@redhat.com>
Eventually, we should replace our uses of libgsystem's gsystem-local-alloc.h
by glib's g_auto*. As a first tiny step, add a compat implementation for g_autofree,
so that we could at least go ahead and use it instead of gs_free.
https://bugzilla.gnome.org/show_bug.cgi?id=794294
Previously, NM_PTRARRAY_LEN() would not work if the pointer type is
an opaque type, which is common. For example:
NMConnection *const*connections = ...;
Add an alternative to g_clear_pointer(). The differences are:
- nm_clear_pointer() is more type safe as it does not cast neither the
pointer nor the destroy function. Commonly, the types should be compatible
and not requiring a cast. Casting in the macro eliminates some of the
compilers type checking. For example, while
g_clear_pointer (&priv->hash_table, g_ptr_array_unref);
compiles, nm_clear_pointer() would prevent such an invalid use.
- also, clear the destination pointer *before* invoking the destroy
function. Destroy might emit signals (like weak-pointer callbacks
of GArray clear functions). Clear the destination first, so that
we don't leave a dangling pointer there.
- return TRUE/FALSE depending on whether there was a pointer to clear.
I tested that redefining g_clear_pointer()/g_clear_object() with our
more typesafe nm_* variants still compiles and indicates no bugs. So
that is good. It's not really expected that turning on more static checks
would yield a large number of bugs, because generally our code is in a good
shape already. We have few such bugs, because we already turn all all warnings
and extra checks that make sense. That however is not an argument for
not introducing (and using) a more resticted implementation.
It's slightly more correct to first clear the pointer location
before invoking the destroy function. The destroy function might
emit other callbacks, and at a certain point the pointer becomes
dangling. Avoid this danling pointer, by first clearing the
memory, and then destroing the instance.
Add macros that cast away the constness of a pointer, but
ensure that the type of the pointer is as expected.
Unfortunately, there is no way (AFAIK) to remove the constness of
a variable, without explicitly passing @type to the macro.
GCC 8.0's -Wcast-function-type objects casting function pointers to ones
with incompatible prototypes. Sometimes we do that on purpose though.
Notably, the g_source_set_callback()'s func argument can point to functions
of various prototypes. Also, libnm-glib/nm-remote-connection is perhaps
just not worth reworking, that would just be a waste of time.
A cast to void(*)(void) avoids the GCC warning, let's use it.
This makes its prototype compatible with GDestroyNotify so that GCC 8.0
won't warn.
The return value is not used anywhere and the unref() functions typically
don't return any.
When pushing a warning disable with clang, always disable
-Wunknown-warning-option first -- it might be that clang wouldn't warn
of what we're trying to disable because it doesn't recognize it in the
first place. That is entierely okay.
With clang-5.0.0:
CC libnm/tests/libnm_tests_test_secret_agent-test-secret-agent.o
In file included from libnm/tests/test-secret-agent.c:29:
In file included from ./shared/nm-test-libnm-utils.h:23:
./shared/nm-utils/nm-test-utils.h:432:3: error: unknown warning group '-Wunused-but-set-variable', ignored [-Werror,-Wunknown-warning-option]
NM_PRAGMA_WARNING_DISABLE("-Wunused-but-set-variable")
^
./shared/nm-utils/nm-macros-internal.h:223:9: note: expanded from macro 'NM_PRAGMA_WARNING_DISABLE'
_Pragma(_NM_PRAGMA_WARNING_DO(warning))
^
<scratch space>:204:25: note: expanded from here
GCC diagnostic ignored "-Wunused-but-set-variable"
^
1 error generated.
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 have a well defined versioning scheme and a defined way how
the version number indicates a stable version. We can simply
calculate NM_VERSION_CUR_STABLE based on the version numbers.
It already defaults to the right value. We only need to define
NM_VERSION_MIN_REQUIRED, so that parts of our internal build
can make use of deprecated API.
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".
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.
A cmp() implementation, for sorting an array with pointers, where each
pointer is an inteter according to GPOINTER_TO_INT().
That cames for example handy, if you have a GHashTable with keys
GINT_TO_POINTER(). Then you get the list of keys via
g_hash_table_get_keys_as_array() and want to sort them.
Sometimes, we want to use CList to track a simple data item. But contrary
to GList/GSList, we need to define a structure to hold the data pointer
and the CList member.
Add a generic NMCListElem type that can be used for such simple uses.
Before you ask: why not use GList/GSList? Because even simple operations
like g_list_append() is O(n), which kinda defeats the purpose of having
a doubly linked list.
This code is added to a new header file nm-c-list.h, the reason is that
there is no other good place:
- "nm-utils/c-list.h" is a clone of upstream, it should not deviate.
- "nm-utils/c-list-util.h" contains our utils functions for c-list.h
but should be plain C, independent of glib.
- "nm-utils/nm-shared-utils.h" contains our glib related utilities,
but it should not drag in "c-list.h".
So, "nm-c-list.h" is a utility libray that extends "c-list.h" and
requires glib.
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.
This is still the very same approach (in the way the array is split
and how elements are compared). The only difference is that the
recursive implementation is replaced by a non-recursive one.
It's (still) stable, top-down merge-sort.
The non-recursive implementation better, because it avoids the overhead
of the function call to recurse.
The compat implementations return a (transfer none) strv instead of a
(transfer container) one. This has caused double frees in nm-applet:
https://bugs.archlinux.org/task/56772
Don't copy the keys and don't free the container later.
[thaller@redhat.com: patch adjusted to avoid compiler warning]
Fixes: 272439cb20
At several places we create strv arrays where the
strings themself are not deep-copied.
This helper function iterates over such an "const char **"
array, clones the strings, and updates the strv array
inplace to be a "char **" strv array.
This helper function is to reduce code duplication.
At various places we get the (string) keys of a GHashTable.
Add a helper function that does that, including an argument
for optional sorting.
The helper function is there to get reduce code duplication.
Systemd instroduces a macro _fallthrough_, see
https://github.com/systemd/systemd/pull/7389.
However, it does not yet seem conclusive how to
handle this properly in ever situation.
While shared/nm-utils/siphash24.c makes use of
the new macro, don't do that in our fork. siphash24.h
does not include all systemd headers, hence _fallthrough_
is not defined. We could re-implement it as _nm_fallthrough,
but given the open questions, that doesn't seem the
Systemd introduced a _fallthrough_ macro in
https://github.com/systemd/systemd/pull/7389.
There might still be some issue with it, but as
I am going to re-import the latest systemd code,
we get them too.
We need it, because "shared/nm-utils/siphash24.c"
will use it too, and that source file does not include
the other systemd macros. So, we will need to re-define
it.
NMUtilsNamedValue's purpose is precisely to create
a list and sort by entires.
Add nm_utils_named_values_from_str_dict() as helper
function to do that.
It is common to have some data indexed by a name.
If you want to sort a list of such data, you would
have to re-implement your own compare function each time.
Instead, add NMUtilsNamedEntry which as first field has
the name. So, you can create your own struct:
struct my_data {
const char *name;
... other fields
}
and compare them with with nm_utils_named_entry_cmp().
For convenience, add another struct NMUtilsNamedValue, which
has only one data field, a pointer.
This allows the compiler to inline the siphash24*() functions
for nm_hash_ptr() and nm_hash_str() (even without LTO).
This of course only applies to nm_hash_ptr() and nm_hash_str(),
which are implemented in "nm-hash-utils.c" itself. All other
nm_hash_*() functions are inline functions in "nm-hash-utils.h",
and thus these functions can be inlined instead. That is, in
other cases, the nm_hash_*() function instead can be inlined.
For nm_hash_ptr() and nm_hash_str() instead we want to inline the
siphash24*() functions.
So, no longer compile "siphash24.c" directly. Instead, only
build "nm-hash-utils.c" which internally #include "siphash24.c".
Next we will use siphash24() instead of the glib version g_direct_hash() or
g_str_hash(). Hence, the "nm-utils/nm-hash-utils.h" header becomes very
fundamental and will be needed basically everywhere.
Instead of requiring the users to include them, let it be included via
"nm-default.h" header.
siphash24() mixes the bits much better then our naive xor.
Don't bypass siphash24(). We supposedly use it for the
better hashing properties, so use it also for pointers.
When using siphash24(), the hash value depends on the hashed input
and the key from _get_hash_key(). If the input is static, so is also
the result of siphash24(), albeit the bits are scrabbled more.
Add a nm_hash_static() to get such a static key, but without actually
doing siphash24(). The static key is also xored with a static_seed.
For that, also mangle the first byte of the hash key using siphash24()
itself. That is, because nm_hash_static() only uses the first guint of the
random key. Hence, we want that this first guint has all the entropy
of the entire key. We use siphash24() itself, to mangle all bits
of the 16 byte key into the first guint.
The nm_close() wrapper should behave exactly the same as calling
close() directly. This is well known, documented behavior.
The only addition on top of that, should be the nm_assert() to catch
double-closing.
Prevously, when passing a negative file descriptor, we wouldn't properly
set errno. Also, the call would not show up in strace, which it should
(at least, if libc's close actually makes the syscall).
I would argue, that passing a negative file descriptor is a bug already
and we should never do that. Maybe we should even assert non-negative
fds. I don't do that now, because I am not sufficiently confident.
Anyway, the change should have not practical effect, because we
shouldn't actually pass negative fds already.
