On CentOS 7, gcc.x86_64 0:4.8.5-44.el7 fails compilation:
In file included from ./src/libnm-glib-aux/nm-default-glib.h:69:0,
from ./src/libnm-glib-aux/nm-default-glib-i18n-lib.h:13,
from src/libnm-core-aux-extern/nm-libnm-core-aux.c:6:
./src/libnm-glib-aux/nm-shared-utils.h:1051:1: error: '__thread' before 'extern'
_nm_thread_local extern char _nm_utils_to_string_buffer[2096];
^
Fixes: fb94903444 ('glib-aux: mark _nm_utils_to_string_buffer at thread-local')
While NMUtilsIPv6IfaceId is only 8 bytes large, it seems unidiomatic to
pass the plain struct around.
With a "const NMUtilsIPv6IfaceId *" argument it is more clear what the
meaning of this is.
Change to use pointers.
We possibly should refactor our code to no use _nm_utils_to_string_buffer,
but instead always provide a suitable (stack allocated?) buffer.
Until that is done, make the buffer thread local so that it avoids
the most obvious problem (of resulting in non-thread-safe code).
The point is rather special, and the macros themselves are basically
simple wrappers around memmove().
When having a sorted array (for example, a strv array that is searched
using nm_strv_find_binary_search()), then we want to insert/remove
elements at a particular place (via memmove()).
Getting the memmove() arguments is not terribly hard, but hard enough to
add two helper macros for that.
The "utils" part does not seem useful in the name.
Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).
Naming is important, because the name of a thing should give you a good
idea what it does. Also, to find a thing, it needs a good name in the
first place. But naming is also hard.
Historically, some strv helper API was named as nm_utils_strv_*(),
and some API had a leading underscore (as it is internal API).
This was all inconsistent. Do some renaming and try to unify things.
We get rid of the leading underscore if this is just a regular
(internal) helper. But not for example from _nm_strv_find_first(),
because that is the implementation of nm_strv_find_first().
- _nm_utils_strv_cleanup() -> nm_strv_cleanup()
- _nm_utils_strv_cleanup_const() -> nm_strv_cleanup_const()
- _nm_utils_strv_cmp_n() -> _nm_strv_cmp_n()
- _nm_utils_strv_dup() -> _nm_strv_dup()
- _nm_utils_strv_dup_packed() -> _nm_strv_dup_packed()
- _nm_utils_strv_find_first() -> _nm_strv_find_first()
- _nm_utils_strv_sort() -> _nm_strv_sort()
- _nm_utils_strv_to_ptrarray() -> nm_strv_to_ptrarray()
- _nm_utils_strv_to_slist() -> nm_strv_to_gslist()
- nm_utils_strv_cmp_n() -> nm_strv_cmp_n()
- nm_utils_strv_dup() -> nm_strv_dup()
- nm_utils_strv_dup_packed() -> nm_strv_dup_packed()
- nm_utils_strv_dup_shallow_maybe_a() -> nm_strv_dup_shallow_maybe_a()
- nm_utils_strv_equal() -> nm_strv_equal()
- nm_utils_strv_find_binary_search() -> nm_strv_find_binary_search()
- nm_utils_strv_find_first() -> nm_strv_find_first()
- nm_utils_strv_make_deep_copied() -> nm_strv_make_deep_copied()
- nm_utils_strv_make_deep_copied_n() -> nm_strv_make_deep_copied_n()
- nm_utils_strv_make_deep_copied_nonnull() -> nm_strv_make_deep_copied_nonnull()
- nm_utils_strv_sort() -> nm_strv_sort()
Note that no names are swapped and none of the new names existed
previously. That means, all the new names are really new, which
simplifies to find errors due to this larger refactoring. E.g. if
you backport a patch from after this change to an old branch, you'll
get a compiler error and notice that something is missing.
We commonly have strv arrays as (char **), (const char*const*) or
(const char **). We thus need to frequently cast the argument to
nm_utils_strv_find_first().
Explicit casts in C don't make the code more typesafe, because
they silently allow completely wrong casts too. On the other hand,
changing the function argument to (const void *) also allows any
pointer, and not just strv pointers.
NM_CAST_STRV_CC() casts the the pointer to a (const char*const*)
strv pointer. It uses _Generic() to only cast a string array, and
not completely unrelated pointers.
As such, it is more convenient to use, as it requires the user no longer
to cast the strv argument, while still being strict about which types
are accepted.
Configuration can have [device*] and [connection*] settings and both
can include a 'match-device=' key, which is a list of device-specs.
Introduce a new 'allowed-connections' key for [device*] sections,
which specifies a list of connection-specs to indicate which
connections can be activated on the device.
With this, it becomes possible to have a device configuration like:
[device-enp1s0]
match-device=interface-name:enp1s0
allowed-connections=except:origin:nm-initrd-generator
so that NM in the real root ignores connections created by the
nm-initrd-generator, and starts activating a persistent
connection. This requires also setting 'keep-configuration=no' to not
generate an assumed connection.
g_idle_add() is discouraged, and the checkpatch.pl script warns
about it.
Sometimes there is a legitimate use of it, when you want to always
schedule an idle action (without intent to cancel or track it). That
makes more sense for g_idle_add() than it does for g_timeout_add(),
because a timeout really should be tracked and cancelled if necessary.
Add a wrapper to rename the legitimate uses. This way, we can avoid the
checkpatch.pl warnings, and can grep for the remaining illegitimate uses.
These functions have overlap with g_ascii_is*() functions.
However g_ascii_is*() (and the is* functions from <ctype.h>) are
always confusing to me, in the sense that it's not clearly stated
which characters qualify for a certain category. And review is not
easy either, because they are implemented via a table lookup.
E.g. were you aware that 127 is considered g_ascii_iscntrl()? Probably
you were, but it's not clear to see that anywhere.
The main point of our own functions is to have is easier to see how
characters get categorized, by using comparison instead of table lookup.
Also, several existing code did in fact not use the g_ascii_is*()
macros, possibly because of the (perceived) difficulty to understand
their exact meaning. As a consequence, several checks got wrong.
For example, (ch < ' ') is not a valid check for testing whether
the character is a ASCII control character, for two reasons:
- if char is a signed type (as likely it is), then this also evaluates
to TRUE for all non-ASCII, UTF-8 characters that are greater than
127.
- it does not consider DEL character (127) a control character.
I missed that we already have a gettid() wrapper. Drop the duplicated
again and use nm_utils_gettid().
Fixes: e874c5bf6b ('random: Provide missing gettid() declaration')
_nm_thread_local is very neat, but when we allocate resources
we need to make sure that they are destroyed when the thread
exits.
We can use pthread_setspecific() for that, but using it is cumbersome.
Add a helper function to make that simpler.
Also, the number of possible pthread_key_t keys is limited. With this
way, we only need one key in total.
This function is badly named, because it has no NMHostnameManager self
argument. It's just a simple function that entirely operates on a string
argument.
Move it away from "nm-hostname-manager.h" to "libnm-glib-aux/nm-shared-utils.h".
Hostname handling is complicated enough. Simple string validation
functions should not obscure the view on the complicated parts.
I think this is non-obvious API, and should be pointed out. As we don't
really have a good place for this comment, the place is a bit unmotivated.
Still, add a comment.
The previous implementation of these macros simply relied on the
__VA_ARGS__ to be expended and joined with ','. That make that work
inside the switch statement, the macros expanded to
switch (val) {
(void) 0, (void) 0;
case 0x1:
s = " ""value" "";
break;
(void) 0, (void) 0;
};
Those NOP statements cause lgtm.com to complain "Dead code due to goto
or break statement".
Implement these macros differently using NM_VA_ARGS_JOIN().
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/function-in-block
Functions should always be declared at file scope. It is confusing
to declare a function at block scope, and the visibility of the function
is not what would be expected.
