- add more code comments
- refactor the code flow in _get_hash_key_init() to follow a simpler
code path.
- use c_siphash_hash() instead of 3 separate steps.
- Drop "?: static_seed" from nm_hash_static(). It's not useful, because
the only _get_hash_key() for which _get_hash_key()^static_seed is zero
is ~static_seed. That means, only one value of all the static seeds
can result in zero here. At that point, we can just coerce that value
to 3679500967u directly.
Sometimes these function may set errno to unexpected values like EAGAIN.
This causes confusion. Avoid that by using our own wrappers that retry
in that case. For example, in rhbz#1797915 we have failures like:
errno = 0;
v = g_ascii_strtoll ("10", 0, &end);
if (errno != 0)
g_assert_not_reached ();
as g_ascii_strtoll() would return 10, but also set errno to EAGAIN.
Work around that by using wrapper functions that retry. This certainly
should be fixed in glib (or glibc), but the issues are severe enough to
warrant a workaround.
Note that our workarounds are very defensive. We only retry 2 times, if
we get an unexpected errno value. This is in the hope to recover from
a spurious EAGAIN. It won't recover from other errors.
https://bugzilla.redhat.com/show_bug.cgi?id=1797915
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
NM_ASCII_SPACES contains the ASCII characters according to
g_ascii_isspace().
Add NM_ASCII_WHITESPACES which differs from NM_ASCII_SPACES by not
including "\f". In some cases, that character shall be excluded.
For example, this is what systemd uses as "WHITESPACE" define at
various places.
Also, reorder the spaces string so that plain space comes first. It is
expected that ' ' is much more frequently than newlines or tabs. While
the order here shouldn't matter, it seems preferably to order frequent
characters in front.
Move the assertion for valid LIST first. It only checks static data,
and regardless of the entry_cmd, it should be done first.
Fixes: f4d12f7b59 ('shared: add NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE() macro for lookup of structs')
Depending on the type, OVS interfaces also have a corresponding netdev
in kernel (e.g. type "internal" does, type "patch" does not).
Such a case is neither NMU_IFACE_OVS nor NMU_IFACE_KERNEL (alone). There should
be a special type to represent those cases.
Add NMU_IFACE_OVS_OR_KERNEL for that.
nm_utils_ifname_valid() is to validate "connection.interface-name"
property. But the exact validation depends on the connection type.
Add "NMU_IFACE_ANY" to validate the name to check whether it would be
valid for any connection type.
This is for completeness and for places where the caller might not know
the connection type.
"all" and "default" never works.
"bonding_masters" works if you unload the bonding module. Well,
that should not really be called working...
Reject these names.
Generally, it's dangerous to reject values that were accepted
previously. This will lead to NetworkManager being unable to load
a profile from disk, which was loadable previously.
On the other hand, kernel would not have treated this setting as
it was intended. So, I would argue that the such a setting was not
working (as intended) anyway.
We can only hope that users don't configure arbitrary interface names.
It generally isn't a good idea to do, so "breaking" such things is less
of a concern.
To really use multiple NM_GOBJECT_PROPERTIES_DEFINE_BASE*() defines in
the same source file, several fixes to the suffix handling are
necessary. This fixes commit f13c7e3bbd ('shared: extend
NM_GOBJECT_PROPERTIES_DEFINE*() macros to append suffix to defined
names') to really work.
Fixes: f13c7e3bbd ('shared: extend NM_GOBJECT_PROPERTIES_DEFINE*() macros to append suffix to defined names')
The interface name might come from the command line or from a filename
(like during nm_vpn_wireguard_import()). It's not clear that this
is valid UTF-8. Asserting against that requires the caller to ensure
that the encoding is valid. That is cumbersome, especially since we anyway
check. Just report a regular error.
quoting 'man ovs-vswitchd.conf.db':
"The name must be alphanumeric and must not contain forward or backward
slashes."
OVS actually accepts a wider range of chars (all printable UTF-8 chars),
NetworkManager restricts this to ASCII char as it's a safer option for
now since OVS is not well documented on this matter.
https://bugzilla.redhat.com/show_bug.cgi?id=1788432
Fixes: e7d72a14f6 ('libnm-core: use different ifname validation function for OVS bridges, ports and interfaces')
The only affected caller is nm_modem_manager_get_modems(), which
is used by NMDeviceBt for DUN connections. This is rather bad.
Fixes: 4154d9618c ('bluetooth: refactor BlueZ handling and let NMBluezManager cache ObjectManager data')
Fixes: e688e70b37 ('shared: add nm_utils_hash_values_to_array() helper')
$ git shortlog -n -s a3e75f3294 -- shared/nm-glib-aux/nm-jansson.h shared/nm-utils/nm-jansson.h
7 Thomas Haller
1 Lubomir Rintel
1 Beniamino Galvani
1 Francesco Giudici
All contributors agreed to the relicensing according to "RELICENSE.md".
$ git shortlog -n -s a3e75f3294 -- shared/nm-glib-aux/nm-glib.h shared/nm-utils/nm-glib.h shared/nm-glib.h include/nm-glib.h include/nm-glib-compat.h
35 Thomas Haller
12 Dan Winship
5 Dan Williams
3 Lubomir Rintel
2 Beniamino Galvani
1 Jan Kantert
1 Thomas Bechtold
The last two contributions by Thomas and Jan are no longer present in any
form, because they were for an older version of glib which is no longer
supported. All other contributors agree to the licences change according
to "RELICENSE.md" file.
For one, we by now require glib >= 2.34.0, so this is not used.
Also, I think G_DEFINE_QUARK() is rather ugly because it constructs
the defined function name (so you cannot grep for it). We should use
NM_CACHED_QUARK_FCN() instead.
Several macros are used to define function. They had a "_STATIC" variant,
to define the function as static.
I think those macros should not try to abstract entirely what they do.
They should not accept the function scope as argument (or have two
variants per scope). This also because it might make sense to add
additional __attribute__(()) to the function. That only works, if
the macro does not pretend to *not* define a plain function.
Instead, embrace what the function does and let the users place the
function scope as they see fit.
This also follows what is already done with
static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark)
When looking at nm_utils_array_find_binary_search(), we see that binary
search really isn't complicated. In nm_utils_array_find_binary_search()
it looks complicated, because that is our general purpose function which
accepts arbitrary lists, uses an opaque compare function, accepts a user
data argument, and returns the insertion position.
This is unnecessary for the narrow purpose in NM_UTILS_STRING_TABLE_LOOKUP_DEFINE*().
When we inline the binary search, it can be simplified, and the remaining
parts is simple enough to prefer this duplication of binary search over
using our general purpose function.
Also, this gives the compiler more chance for optimization. For
example, we can use "unsigned" as index type instead of gssize, because
we know (at compile time), that this type will always be large enough
for our LIST. Also, we can directly call strcmp().
The result is that the macro's implementation is also fast in the best
case (where the needle is found with only one strcmp()) and in the cases
where there is a small number of items to search.
It thus alleviates concerns that using the macro might be slower than
an optimized implementation.
The binary size of the defined function increases slightly (from 112
bytes to 192 bytes, on x86_64, GCC, -O2). But according to my tests it
is slightly and measurably faster.
The _NM_GET_PRIVATE_PTR() macro is possibly used from other macros. And
"_self" is a pretty good name to use. Don't let the lower layer macro
use this name.
Most callers would pass FALSE to nm_utils_error_is_cancelled(). That's
not very useful. Split the two functions and have nm_utils_error_is_cancelled()
and nm_utils_error_is_cancelled_is_disposing().
There are crashes where this assertion fails, but it's not clear
how that could happen (because the input text seems a usual IPv4 address).
Try to collect some more information about what failed. It's only
enabled with NM_MORE_ASSERTS anyway.
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1797915
G_SOURCE_FUNC has attribute GLIB_AVAILABLE_MACRO_IN_2_58, which means
that the compiler will emit a warning when GLIB_VERSION_MAX_ALLOWED <
GLIB_VERSION_2_58. We currently define GLIB_VERSION_MAX_ALLOWED as
GLIB_VERSION_2_40. Redefine the macro to fix the following build error
when using glib >= 2.63.5 (the version in which the attribute was
added):
CC shared/nm-glib-aux/libnm_glib_aux_la-nm-shared-utils.lo
shared/nm-glib-aux/nm-shared-utils.c: In function ‘nm_g_unix_fd_source_new’:
shared/nm-glib-aux/nm-shared-utils.c:3679:13: error: Not available before [-Werror]
3679 | g_source_set_callback (source, G_SOURCE_FUNC (source_func), user_data, destroy_notify);
Fixes: 9c5741ccd2 ('shared/nm-glib: add compat implementation for G_SOURCE_FUNC()')
This should give the compiler more possibilities to warn about wrong
use of the API.
In practice, my current compiler wouldn't flag any issues. However,
some compilers (or compile options) might.
It seems to complicate things more than helping. Drop it. What we still have
is a wrapper around plain g_dbus_connection_signal_subscribe(). That one is
trivial and helpful. The previous wrapper seems to add more complexity.
The purpose is to clear the entire available buffer, not only
up to the first '\0'. This is done, because otherwise we might
leak sensitive data that happens to be after the first '\0',
or we might give away the length of the secrets.
Of course, those are very (very) minor concerns. But avoiding them is
easy enough.
We cannot know the key/value free functions, hence, our compat implementation
cannot free the values if they are not requested. The "solution" is to require
the caller to fetch all values, always.
Found by covscan:
NetworkManager-1.22.0/shared/nm-glib-aux/nm-dbus-aux.c:361:
missing_va_end: va_end was not called for "ap".
Fixes: ce36494c0a ('shared: add nm_dbus_error_is() helper')
"nm-glib-aux/nm-logging-fwd.h" provides macros like _LOGD() to be reused
by various parts which implement logging (by defining _NMLOG() accordingly).
libnm also has logging, however it uses different logging levels
aside LOGD_DEBUG.
Instead, implement _LOGD() using a define _LOGL_DEBUG, so that libnm can
redefine thos _LOGL_DEBUG defines and use the _LOGD() macro.
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
"shared/nm-utils" got long renamed and split into separate parts. The remaining
tests are really to test nm-std-aux and nm-glib-aux (no libnm dependencies). Move
the tests to the appropriate place.
inet_aton() also supports IPv4 addresses in octal (with a leading '0')
or where not all 4 digits of the address are present.
Add nm_utils_parse_inaddr_bin_full() to optionally fallback to
parse the address with inet_aton().
Note taht inet_aton() also supports all crazy formats, including
ignoring trailing garbage after a whitespace. We don't want to accept
that in general.
Note that even in legacy format we:
- accept everything that inet_pton() would accept
- additionally, we also accept some forms which inet_aton() would
accept, but not all.
That means, the legacy format that we accept is a superset of
inet_pton() and a subset of inet_aton(). Which is desirable.
We have our NM specific logging and log levels. Maybe we should
not have that, and instead only rely on syslog (like systemd)
or glog(). Anyway, currently we have one way and it makes sense
that this is also used outside from "src".
Move the helper function to parse log levels from string to
"nm-logging-base.h" so that we can use the same logging levels
outside of core.
This moves code that is currently GPL2+ licensed to
LGPL2.1+. However as far as I see, this code was entirely written
by Red Hat employees who would not object with this change. Also,
it's as obvious and trivial as it gets.
We have "nm-logging-fwd.h", which (as the name implies) is header-only.
Add instead a "nm-logging-base.c", which also contains implementation for
logging functions that are not only useful under "src/nm-logging.c"
We will rework NMClient entirely. Then, the synchronous initialization will also use
the asynchronous code paths. The difference will be that with synchronous initialization,
all D-Bus interaction will be done with an internal GMainContext as current thread default,
and that internal context will run until initialization completes.
Note that even after initialization completes, it cannot be swapped back to the user's
(outer) GMainContext. That is because contexts are essentially the queue for our
D-Bus events, and we cannot swap from one queue to the other in a race
free manner (or a full resync). In other words, the two contexts are not in sync,
so after using the internal context NMClient needs to stick to that (at least, until
the name owner gets lost, which gives an opportunity to resync and switch back to the
user's main context).
We thus need to hook the internal (inner) GMainContext with the user's (outer) context,
so when the user iterates the outer context, events on the inner context get dispatched.
Add nm_utils_g_main_context_create_integrate_source() to create such a GSource for
integrating two contexts.
Note that the use-case here is limited: the integrated, inner main context must
not be explicitly iterated except from being dispatched by the integrating
source. Otherwise, you'd get recursive runs, possible deadlocks and general
ugliness. NMClient must show restrain how to use the inner context while it is
integrated.
Some compilers don't convert arrays as _Generic() type selectors to
their pointer type. That means, for those compilers the generic type
would be an array and not a pointer. Work around that by adding zero
to the pointer/array argument.
Also, I cannot get this to work with "clang-3.4.2-9.el7". Disable it
for that compiler. The value of the generic check is anyway that it only
needs to work with some compiler combinations. That will trigger a
compilation failure and we can fix the implementation also for compilers
that don't support the macro.
See-also: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1930.htm
There are two macros: NM_GOBJECT_PROPERTIES_DEFINE_BASE() and
NM_GOBJECT_PROPERTIES_DEFINE(). The former just defines the
property enums and the obj_properties array. The latter also
defines the functions _notify() and _nm_gobject_notify_together_impl().
That means, depending on whether you actually use _notify(), you have
to choose one of the macros. I think that is unnecessarily cumbersome.
Let's mark the function as _nm_unused so that the compiler doesn't
complain about the unused function. I don't think it's a problem
to use NM_GOBJECT_PROPERTIES_DEFINE() even if you don't actually use
_notify().
We preferably should use our convenience macros like _LOGD().
Since those macros expand to _NMLOG() (which needs to be defined
separately), we can move it to "nm-logging-fwd.h" and reuse.
glib really likes the numeric source IDs. That is, g_idle_add(), g_timeout_add(),
etc. return those IDs, that can then be destroyed with g_remove_source() (or
nm_clear_g_source()).
I think these numeric IDs are really not great.
- API like g_idle_add() and g_remove_source() only works with the g_main_context_get_default()
instance. That means, you cannot use this API for any other contexts. If you'd insist on using
numeric IDs, you'd need to call g_main_context_find_source_by_id() on the right context
first (but you'd also have to track the context alongside the ID).
- g_remove_source() requires first a call to g_main_context_find_source_by_id(). This involves
taking a mutex and doing an extra hash lookup.
Instead, it often seems preferable to use the GSource instance directly. It works
with any context, it can be referenced and unreferenced, and it can be destroyed, and
avoids the overhead of g_main_context_find_source_by_id().
The only downside really is that keeping a GSource pointer takes one pointer size, while
the guint source ID is usually only 4 bytes.
Anyway, I think we should deal more with GSource instances directly. Hence, add this
convenience macro, that works like nm_clear_g_source().
Note that we should always set the source-tag of our GTask.
This allows us to better assert that the user uses the right
_finish() method for the task.
The plain g_task_new() does not have a souce-tag argument. Hence, we would
always need to explicitly call g_task_set_source_tag().
Likewise, to check the source tag, we would always need to write
g_return_val_if_fail (g_task_is_valid (result, self), FALSE);
g_return_val_if_fail (g_async_result_is_tagged (result, tag), FALSE);
Actually, g_async_result_is_tagged() uses the GAsyncResultIface to
call iface->is_tagged(). This has unnecessary overhead, so we should
just call g_task_get_source_tag() directly.
Add helper functions for that.
Several points.
- We spawn the dnsmasq process directly. That has several downsides:
- The lifetime of the process is tied to NetworkManager's. When
stopping NetworkManager, we usually also stop dnsmasq. Or we keep
the process running, but later the process is no longer a child process
of NetworkManager and we can only kill it using the pidfile.
- We don't do special sandboxing of the dnsmasq process.
- Note that we want to ensure that only one dnsmasq process is running
at any time. We should track that in a singletone. Note that NMDnsDnsmasq
is not a singleton. While there is only one instance active at any time,
the DNS plugin can be swapped (e.g. during SIGHUP). Hence, don't track the
process per-NMDnsDnsmasq instance, but in a global variable "gl_pid".
- Usually, when NetworkManager quits, it also stops the dnsmasq process.
Previously, we would always try to terminate the process based on the
pidfile. That is wrong. Most of the time, NetworkManager spawned the
process itself, as a child process. Hence, the PID is known and NetworkManager
will get a signal when dnsmasq exits. The only moment when NetworkManager should
use the pidfile, is the first time when checking to kill the previous instance.
That is: only once at the beginning, to kill instances that were
intentionally or unintentionally (crash) left running earlier.
This is now done by _gl_pid_kill_external().
- Previously, before starting a new dnsmasq instance we would kill a
possibly already running one, and block while waiting for the process to
disappear. We should never block. Especially, since we afterwards start
the process also in non-blocking way, there is no reason to kill the
existing process in a blocking way. For the most part, starting dnsmasq
is already asynchronous and so should be the killing of the dnsmasq
process.
- Drop GDBusProxy and only use GDBusConnection. It fully suffices.
- When we kill a dnsmasq instance, we actually don't have to wait at
all. That can happen fully in background. The only pecularity is that
when we restart a new instance before the previous instance is killed,
then we must wait for the previous process to terminate first. Also, if
we are about to exit while killing the dnsmasq instance, we must register
nm_shutdown_wait_obj_*() to wait until the process is fully gone.
Having G_PID_FORMAT macro is useful, but it's only available in
recent glib versions. Add a compat implementation and a test that
our assumptions hold.
The leading underscore has the notion that this would be a private function.
It really isn't, and it would be fine for the user to call it directly.
Just like we have g_slice_free() and g_slice_free1().
I'd like to refactor libnm's caching. Note that cached D-Bus objects
have repeated strings all over the place. For example every object will
have a set of D-Bus interfaces (strings) and properties (strings) and an
object path (which is referenced by other objects). We can save a lot of
redundant strings by deduplicating/interning them. Also, by interning
them, we can compare them using pointer equality.
Add a NMRefString implementation for this.
Maybe an alternative name would be NMInternedString or NMDedupString, because
this string gets always interned. There is no way to create a NMRefString
that is not interned. Still, NMRefString name sounds better. It is ref-counted
after all.
Notes:
- glib has GQuark and g_intern_string(). However, such strings cannot
be unrefered and are leaked indefinitely. It is thus unsuited for
anything but a fixed set of well-known strings.
- glib 2.58 adds GRefString, but we cannot use that because we
currently still use glib 2.40.
There are some differences:
- GRefString is just a typedef to char. That means, the glib API
exposes GRefString like regular character strings.
NMRefString intentionally does that not. This makes it slightly
less convenient to pass it to API that expects "const char *".
But it makes it clear to the reader, that an instance is in fact
a NMRefString, which means it indicates that the string is
interned and can be referenced without additional copy.
- GRefString can be optionally interned. That means you can
only use pointer equality for comparing values if you know
that the GRefString was created with g_ref_string_new_intern().
So, GRefString looks like a "const char *" pointer and even if
you know it's a GRefString, you might not know whether it is
interned. NMRefString is always interned, and you can always
compare it using pointer equality.
- In the past I already proposed a different implementation for a
ref-string. That made different choices. For example NMRefString
then was a typedef to "const char *", it did not support interning
but deduplication (without a global cache), ref/unref was not
thread safe (but then there was no global cache so that two threads
could still use the API independently).
The point is, there are various choices to make. GRefString, the
previous NMRefString implementation and the one here, all have pros and
cons. I think for the purpose where I intend NMRefString (dedup and
efficient comparison), it is a preferable implementation.
Ah, and of course NMRefString is an immutable string, which is a nice
property.
There is really no reason for this to be a macro. Our hash-related
helpers (like nm_hash_update_val()) are macros because they do some
shenigans to accept arguments of different (compile-time) types. But
the arguments for nm_hash_obfuscate_ptr() are well known and expected
of a certain form.
Note that with "-O2" some quick testing shows that the compiler no
longer inlines the function. But I guess that's fine, probably the
compiler knows best anyway.
... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
Don't mix negative errno return value with a GError output. Instead,
return a boolean result indicating success or failure.
Also, optionally
- output GError
- set out_errsv to the positive errno (or 0 on success)
Obviously, the return value and the output arguments (contents, length,
out_errsv, error) must all agree in their success/failure result.
That means, you may check any of the return value, out_errsv, error, and
contents to reliably detect failure or success.
Also note that out_errsv gives the positive(!) errno. But you probably
shouldn't care about the distinction and use nm_errno_native() either
way to normalize the value.
nm_utils_file_set_contents() is a re-implementation of g_file_set_contents(),
as such it returned merely a boolean success value.
It's sometimes interesting to get the native error code. Let the function
deviate from glib's original g_file_set_contents() and return the error code
(as negative value) instead.
This requires all callers to change. Also, it's potentially a dangerous
change, as this is easy to miss.
Note that nm_utils_file_get_contents() also returns an errno, and
already deviates from g_file_get_contents() in the same way. This patch
resolves at least the inconsistency with nm_utils_file_get_contents().
- add nm_c_list_elem_find_first() macro that takes a predicate
and returns the first match.
This macro has a non-function-like behavior, which we often try to
avoid because macros should behave like functions. In this case it's
however convenient, so let's do it.
Also, despite being non-function-like, it should be pretty hard to
use wrongly.
- rename nm_c_list_elem_find_first() to nm_c_list_elem_find_first_ptr().
Coverity says
CID 202453 (#1 of 1): Wrong sizeof argument (SIZEOF_MISMATCH)suspicious_sizeof:
Passing argument user_data of type gconstpointer and argument (gsize)nargs * 8UL /* sizeof (gconstpointer) */ to function g_slice_free1 is suspicious.
Let's pass instead the "data" pointer. It's the same, but maybe that
avoids the warning.
(cherry picked from commit d76df4c139)
Coverity doesn't like us ignoring the return value, although
we really only care about the "p" output pointer.
Try casting the result to (void), maybe that silences Coverity.
(cherry picked from commit e6fa3ce2df)
- don't let no_auto_default_from_file() do any preprocessing of
the lines that it reads. It merely splits the lines at '\n'
and utf8safe-unescapes them.
This was previously duplicated also by NMConfigData's property
setter. We don't need to do it twice.
- sort the lines. This makes the entire handling O(n*ln(n)) instead
of O(n^2). Also, sorting effectively normalizes the content, and
it's desirable to have one true representation of what we write.
In particular when calling nm_utils_strv_sort() with a positive length
argument, then this is not a %NULL terminated strv arrary. That may mean
that it makes sense for the input array to contain %NULL strings.
Use a strcmp() function that accepts %NULL too.
While this is not used at the moment, I think nm_utils_strv_sort()
should accept %NULL strings beause otherwise it's a possibly unexpected
restriction of its API. The function should handle sensible input gracefully.
It is like strcmp(), but has a signature suitable for GCompareDataFunc.
This is necessary with nm_utils_ptrarray_find_binary_search()
to search a sorted strv array.
The fault is here really C, which doesn't allow inline static functions.
So, you need all kinds of slightly different flavors for the same
callbacks (with or without user-data).
Note that glib2 internally just casts strcmp() to GCompareDataFunc ([1]),
relying on the fact how arguments are passed to the function and
ignoring the additional user-data argument. But I find that really
ugly and probably not permissible in general C. Dunno whether POSIX
would guarantee for this to work. I'd rather not do such function
pointer casts.
[1] 0c0cf59858/glib/garray.c (L1792)
Using clock_gettime() directly is a bit inconvenient. We usually
want to combine the fields of struct timespec into one timestamp
(for example, in unit nanoseconds).
Add a helper function to do that.
nm_strndup_a() uses strncpy() because we want the behavior of clearing out
the memory after the first NUL byte. But that can cause a compiler warning:
CC src/settings/plugins/keyfile/libNetworkManager_la-nms-keyfile-utils.lo
In file included from ../../shared/nm-default.h:279,
from ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:20:
In function ‘_nm_strndup_a_step’,
inlined from ‘nms_keyfile_loaded_uuid_is_filename’ at ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:65:9:
../../shared/nm-glib-aux/nm-macros-internal.h:1661:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
1661 | strncpy (s, str, len);
| ^~~~~~~~~~~~~~~~~~~~~
../../src/settings/plugins/keyfile/nms-keyfile-utils.c: In function ‘nms_keyfile_loaded_uuid_is_filename’:
../../src/settings/plugins/keyfile/nms-keyfile-utils.c:48:8: note: length computed here
48 | len = strlen (filename);
| ^~~~~~~~~~~~~~~~~
It's true that the len argument of _nm_strndup_a_step() depends on the
string length of the source string. But in this case it's safe, because
we checked that the destination buffer is exactly the right size too.
By that reasoning we should use memcpy() or strcpy(), but both are
unsuitable. That is because we want nm_strndup_a() to behave like
strndup(), which means we need to handle cases where the len argument
is larger than the string length of the source string. That is, we want
always to return a buffer of size len+1, but we want to copy only the
characters up to the first NUL byte, and clear out the rest. That's what
strncpy() does for us.
Silence the warning.
If there is only one argument, we can assume this is a plain string.
That is especially the case, because g_set_error() is G_GNUC_PRINTF()
and would warn if this would be a format string with missing parameters.
This is for convenience. Previously, one was compelled to explicitly
choose between nm_utils_error_set_literal() and nm_utils_error_set().
Now, it automatically chooses.
Note that there are a few things that won't work, like
nm_utils_error_set (error, code, "bogus %u escape");
But that's good. You get a compiler warning (as you used to)
and it's clear in this case you really need
nm_utils_error_set_literal().
This follows a pointer to a pointer and compares them. In a sense
it's like nm_pstr_*(), which follow a pointer to a string. However,
these functions use direct pointer comparison.
The purpose is when you hash a key that has as first field a pointer
value (and then compare them by pointer equality).
Previously, nm_c_list_move_*() only allowed to move element inside the
same list. Relax that, it works just the same list to move the element
from one list into a different list.
Usually we avoid GSList, because I think it's not a great data type.
Anyway, our match-specs are just a GSList of strings, so we need some
API to handle them.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
Clang 3.4.2-9.el7 on CentOS7.6 complains about missing generic type match:
../dispatcher/nm-dispatcher.c:243:2: error: controlling expression type 'const Request *const' (aka 'const struct Request *const') not compatible with any generic association type
_LOG_R_D (request, "start running ordered scripts...");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: 17dc6a9da6 ('shared: add _NM_ENSURE_TYPE_CONST()')
The sole purpose of this is more type-safe macros.
An alternative solution would be to define a function instead of a
macro. But if the function is unused (currently!) you get a compiler
warning (on some compilers even when marking the function as "static
inline", if it's in the source file). A workaround for that would be
to mark the function as _nm_unused, or to use a macro instead.
_NM_ENSURE_TYPE_CONST() is to aid the macro solution.
"nm-value-type.h" is a header-only file, as it contains only small
accessors that should be inlined.
As such, the implementation of these functions is guarded by "#ifdef
NM_VALUE_TYPE_DEFINE_FUNCTIONS", so that one can use this header (and
NMValueType enum) with less overhead (at compile time).
Glib has GValue which used for boxing value.
Add NMValueType enum, which has a similar purpose, but it's much more
limited.
- contrary to GValue, the type must be tracked separately from the
user-data. That is, the "user-data" is only a pointer of appropriate
type, and the knowledge of the actual NMValueType is kept separately.
This will be used to have a static list of meta-data that knows the
value types, but keeping the values independent of this type
information. With GValue this would not be possible.
- the use case is much more limited. Just support basic integers,
boolean and strings. Nothing fancy.
Note that we already do something similar at muliple places. See for
example NMVariantAttributeSpec and nm_utils_team_link_watcher_to_string().
These could/should instead use NMValueType.
The type of the "data" pointer may not be compatible with the type of
the "to_free" / output pointer. This is due to constness, and that we
are unable in C to remove constness from a type.
For example,
{
const char *const *data = ...;
gs_free const char **cpy_to_free = NULL;
const char **cpy;
cpy = nm_memdup_maybe_a (300, data, NM_PTRARRAY_LEN (data) + 1, &cpy_to_free);
}
is prefectly valid , but would not have compiled.
It shows that "data" is not of type "*(&cpy_to_free)", but rather
it might be a non-const pointer of the same type.
Fixes: d0e1d0e626 ('shared: propagate types in nm_malloc_maybe_a(), nm_malloc0_maybe_a(), nm_memdup_maybe_a()')
We want the the hash-seed array is alined so it can be used both as
guint, guint32, and guint64 directly. Don't use _nm_alignas() but
instead just add the fields to the union so we get proper alignment.
While at at, also let the seed argument to c_siphash_init() be aligned
to 64 integers. c_siphash_init() does not require that, but it tries to
read the seed as (unaligned) LE 64 bit integers. So, it doesn't hurt.
We want to log pointer values to indicate the related parties of a
log message. But we should not, because plain pointer values can be
used to defeat ASLR.
Instead, we have nm_hash_obfuscate_ptr() to managle a pointer and give
a distinct (albeit not 100% unique) 64 bit integer for logging.
But for the logging messages to be meaning-full, all related parties
must use the same static-seed.
Add a macro NM_HASH_OBFUSCATE_PTR() that uses a particular seed.
... and nm_dbus_connection_call_get_name_owner().
We are going to use GDBusConnection more instead of GDBusProxy. Hence,
these two functions are the standard repertoire and used over and over.
Their arguments are complicated enough to warrant a small helper.
It will be used for "/var/lib/NetworkManager/seen-bssids" and
"/var/lib/NetworkManager/timestamps" which currently is implemented
in NMSettingConnection.
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
