It has no actual dependency on the crypto library. All it does, is
to be careful about not leaking secrets in memory. We have code
for that in libnm-glib-aux already. Move.
The goal is to reduce the number of places where we use libnm-crypto,
because that has a large dependency. libnm-glib-aux is a very light
dependency instead.
nm_utils_bin2hexstr() is part of public libnm API.
That means, if we want to use this function, we need to link with
libnm-core-impl.
This is used by "nm-crypto.c". That file is currently part of
libnm-core, but that will change.
Move the implementation to libnm-glib-aux, so that we can use this code
from all our glib-based code (because all our glib-based code is allowed
to link with libnm-glib-aux).
If __VA_ARGS__ contains odd arguments, it's not clear that N_ARG() gives
the same as the array initialization. Add a static assert that the
numbers agree to catch wrong usage of the macro.
For example:
nm_gobject_notify_together(setting, a, b, );
g_idle_add() uses G_PRIORITY_DEFAULT_IDLE priority. Most of the time we don't
care much about the priority.
But at the places that this patch changes, I think that using
G_PRIORITY_DEFAULT_IDLE (and following g_idle_add()) is more correct. The
reason for this is not very strong, except that it's probably the better
choice. And the old choice was made because I didn't realize that
g_idle_add() uses another default priority. Hence, the old choice was not
for good reasons either.
nm_g_idle_add_source() is supposed to work like g_idle_add(). Use the correct
priority.
I think this causes little actual problems, because usually we don't
carefully tune the priorities and would be mostly fine with either.
Fixes: 6b18fc252d ('shared: add nm_g_{idle,timeout}_add_source() helpers')
For one, this API is only available since 2.72, thus we must not use
it (unless we would add a compat implementation to nm-glib.h).
But also, g_alloca0() evaluates the size argument multiple times,
making it non-function like. That seems highly undesirable and error
prone.
Also, we should be very careful about alloca() and the potential
for stack overflow. We use alloca() at times, but usually with
macros that are named "*_a()" (to make the danger clearer) and compile
time checks for the size. These glib functions make this slightly
less safe.
Just prevent us from using this API.
What nm_uuid_generate_from_string() does, is pretty straight forward.
What nm_crypto_md5_hash() does, is not.
Just directly use GChecksum, it seems clearer.
Also, sometimes the compiler is adamant to warn about uninitialized variables.
The workaround from commit cb9ca67901 ('glib-aux: workaround maybe-uninitialized
warning with LTO in nm_uuid_generate_from_string_str()') does not always work.
Try to solve that this way.
Note that we have plenty of unit tests for our UUID generation. This is
covered by tests.
Also, there is now only one caller of nm_crypto_md5_hash() left. Which
is good, because that function is rather non-obvious and special purpose.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1129
gcc-12.0.1-0.8.fc36 is annoying with false positives.
It's related to g_error() and its `for(;;) ;`.
For example:
../src/libnm-glib-aux/nm-shared-utils.c: In function 'nm_utils_parse_inaddr_bin_full':
../src/libnm-glib-aux/nm-shared-utils.c:1145:26: error: dangling pointer to 'error' may be used [-Werror=dangling-pointer=]
1145 | error->message);
| ^~
/usr/include/glib-2.0/glib/gmessages.h:343:32: note: in definition of macro 'g_error'
343 | __VA_ARGS__); \
| ^~~~~~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1133:31: note: 'error' declared here
1133 | gs_free_error GError *error = NULL;
| ^~~~~
/usr/include/glib-2.0/glib/gmessages.h:341:25: error: dangling pointer to 'addrbin' may be used [-Werror=dangling-pointer=]
341 | g_log (G_LOG_DOMAIN, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
342 | G_LOG_LEVEL_ERROR, \
| ~~~~~~~~~~~~~~~~~~~~~~~
343 | __VA_ARGS__); \
| ~~~~~~~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1141:13: note: in expansion of macro 'g_error'
1141 | g_error("unexpected assertion failure: could parse \"%s\" as %s, but not accepted by "
| ^~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1112:14: note: 'addrbin' declared here
1112 | NMIPAddr addrbin;
| ^~~~~~~
I think the warning could potentially be useful and prevent real bugs.
So don't disable it altogether, but go through the effort to suppress it
at the places where it currently happens.
Note that NM_PRAGMA_WARNING_DISABLE_DANGLING_POINTER macro only expands
to suppressing the warning with __GNUC__ equal to 12. The purpose is to
only suppress the warning where we know we want to. Hopefully other gcc
versions don't have this problem.
I guess, we could also write a NM_COMPILER_WARNING() check in
"m4/compiler_options.m4", to disable the warning if we detect it. But
that seems too cumbersome.
New gcc-12.0.1-0.8.fc36 on Fedora rawhide likes to emit false
"-Wdangling-pointer" warnings with some g_error() uses. It seems
related to g_error()'s `for(;;) ;`.
As workaround, add a macro to suppress the warning.
But only do that for gcc-12. This bug hopefully gets fixed
and we don't want to suppress useful warnings too eagerly.
https://bugzilla.redhat.com/show_bug.cgi?id=2056613
Also, combine the different macros in the same #if/#else block.
The point of this is if you have a macro that does conditionally
NM_PRAGMA_WARNING_DISABLE(), then we need a way to balance the
push/pop.
G_TYPE_STRV is the last property type in NMSetting that is implemented
by directly accessing the GObect property. Note that we have lots of
override, non-default implementations that still use GObject properties,
but I am talking here about properties that don't have a special
implementation and use a G_TYPE_STRV GObject property.
Add a "direct" implementation also for strv arrays.
The advantage is that we no longer call g_value_get() for various
operations, which requires a deep-copy of the strv array. The other
advantage is that we will get a unified approach for implementing strv
properties. In particular strv arrays need a lot of code to implement,
and most settings do it differently. By adding a general mechanism,
this code (and behavior) can be unified.
Showcase it on "match.interface-name".
First of all, all of NMVariantAttributeSpec is internal API. We only
expose the typedef itself as public API, but not its fields nor
their meaning. So we can change things.
Change "str_type" to "type_detail", so that it can work for any kind of
attribute, not only for strings. Usually, we want to avoid special
cases and treat all attributes the same, based on their GVariant type.
But sometimes, it is necessary to do something special with an
attribute. This is what the "type_detail" encodes, but it's not only
relevant for strings.
We don't run glib-mkenums for certain sources like "core" and
"libnm-glib-aux".
These annotations have no effect. Drop them.
They also mess with the automated formatting.
In function 'nm_uuid_unparse',
inlined from 'nm_uuid_generate_from_string_str' at src/libnm-glib-aux/nm-uuid.c:393:12,
inlined from 'nm_uuid_generate_from_strings.constprop' at src/libnm-glib-aux/nm-uuid.c:430:16:
src/libnm-glib-aux/nm-uuid.h:37:12: error: 'uuid' may be used uninitialized [-Werror=maybe-uninitialized]
37 | return nm_uuid_unparse_case(uuid, out_str, FALSE);
| ^
src/libnm-glib-aux/nm-uuid.c: In function 'nm_uuid_generate_from_strings.constprop':
src/libnm-glib-aux/nm-uuid.c:20:1: note: by argument 1 of type 'const struct NMUuid *' to 'nm_uuid_unparse_case.constprop' declared here
20 | nm_uuid_unparse_case(const NMUuid *uuid, char out_str[static 37], gboolean upper_case)
| ^
src/libnm-glib-aux/nm-uuid.c:390:12: note: 'uuid' declared here
390 | NMUuid uuid;
| ^
lto1: all warnings being treated as errors
The problem are code paths with failed g_return*() assertions. Being in
a bad state already, they don't bother to ensure proper return values,
and with LTO the compiler might think there are valid code paths wrongly
handled. Work around.
nm_hash_str() has the proper name and signature for that it does.
That is, it has a "nm_hash_*" prefix and the parameter is of type
"const char *".
nm_hash_str() has this name because it is primarily about hashing.
For hash tables, glib has g_str_hash() and g_str_equal(). We want
to replace g_str_hash() with our implementation (nm_hash_str()) because
that uses siphash24 with a random seed.
But in those cases we want to use the more familiar name "nm_str_hash()",
which reminds of g_str_hash() and follows the pattern of g_str_equal().
Thus:
g_hash_table_new(nm_str_hash, g_str_equal);
is preferable over
g_hash_table_new(nm_hash_str, g_str_equal);
Hence, we have (and had) nm_str_hash() effectively an alias to
nm_hash_str.
The question is: which name is preferable? Or should they be both present
for their slightly distinct uses? The approach taken here is to have
both names, to reflect their purpose better.
But as the usage of nm_str_hash is as function pointer for GHashTable, it was
not an inline function and we'd pay a small overhead with this approach of
aliasing. Avoid that overhead by defining nm_str_hash with the C
preprocessor.
For similar reasons, do that for nm_direct_hash.
We avoid printing pointer values directly, instead we usually call
NM_HASH_OBFUSCATE_PTR(). This hashes the pointers with a random seed
so they are not directly visible.
That obviously makes it harder to debug. Add an environment variable
to disable that.
$ NM_OBFUSCATE_PTR=0 LIBNM_CLIENT_DEBUG=trace,stdout nmcli
Note that this flag is only honored in debug builds (WITH_MORE_ASSERTS>0).
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
nm_ether_addr_equal() already accepts NULL addresses and makes a
distinction between NULL and a zero address.
Introduce a similar behavior for nm_ip_addr_equal(), as it's useful in
some cases.
"flags" are a g_param_spec_flags() and correspond to G_TYPE_FLAGS type.
They are internally stored as guint, and exported on D-Bus as "u" (32 bit
integer).
Give a consistent name.
A bit odd are now the names nm_g_bytes_hash() and nm_g_bytes_equal()
as they go together with nm_pg_bytes_hash()/nm_pg_bytes_equal().
But here the problem is more with the naming of "nm_p*_{equal,hash}()"
functions, which probably should be renamed to "nm_*_ptr_{equal,hash}()".
There is g_idle_add(), g_timeout_add() and g_timeout_add_seconds().
We have alternatives nm_g_idle_add_source() and
nm_g_timeout_add_source().
I find the previous name nm_g_timeout_add_source_seconds() inconsistent
with the pattern, and get it always wrong on first try. Rename.
The plain nm_strv_equal(), nm_strv_equal_n(), nm_strv_cmp_n() functions
treat NULL strv arrays (without specified length) different than strv
arrays of length zero. They do so, because that is useful sometimes.
Sometimes it is not useful and we want to treat empty arrays the same
as a NULL array. Add helpers for that.
