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()')
Properly initialize 'overload' when the space in the file section
ends.
shared/n-dhcp4/src/n-dhcp4-outgoing.c: In function ‘n_dhcp4_outgoing_append’:
shared/n-dhcp4/src/n-dhcp4-outgoing.c:198:17: error: ‘overload’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
Move back to INIT state after the lease expires, as per section 4.4.5
of RFC 2131. Previously the client just moved to EXPIRED, closed the
connection and cleared the probe, leaving to the caller of the library
the choice to create a new client instance and to start from
scratch. However, it seems more useful that the client, once
initialized, always tries to get a lease even after an expiration.
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
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.
nmtstc_client_new() exists to test creating a GInitiable/GAsyncInitiable
in different GMainContext combinations.
This is not only useful for NMClient but will also be useful for
NMSecretAgentOld. Add nmtstc_context_object_new() to allow for that.
Also, allow passing parameters when creating the object.
The resulting nmtstc_context_object_new() is relatively complex. But
this is only testing code, that aims to construct the respective GObject
instance in various manners (randomly using the sync or async initialization).
It is complex, but delivers at testing various code paths of the
underlying code. The API that it provides however is simple.
Also drop _nmtstc_client_new_extra_context() to create the instance with
a different context. For one, this requires that the internal context is
integrated as long as the context-busy-watcher exists. That was not
handled correctly. Also, creating a NMClient instance with a different
context than the current thread default at construct time has
implications to the test later. The tests don't want this variant, and
don't handle them properly. So drop this.
nmtst_main_context_iterate_until*() iterates until the condition is
satisfied. If that doesn't happen within timeout, it fails an assertion.
Rename the function to make that clearer.
If the server sends a packet with multiple instances of the same
option, they are concatenated during n_dhcp4_incoming_linearize() and
evaluated as a single option as per section 7 of RFC 3396.
However, there are broken server implementations that send
self-contained options in multiple copies. They are reassembled to
form a single instance by the nettools client, which then fails to
parse them because they have a length greater than the expected one.
This problem can be reproduced by starting a server with:
dnsmasq --bind-interfaces --interface veth1 -d
--dhcp-range=172.25.1.100,172.25.1.200,1m
--dhcp-option=54,172.25.1.1
In this way dnsmasq sends a duplicate option 54 (server-id) when the
client requests it in the 'parameter request list' option, as
dhcp=systemd and dhcp=nettools currently do.
While this is a violation of the RFC by the server, both isc-dhcp and
systemd-networkd client implementations have mechanisms to deal with
this situation. dhclient simply takes the first bytes of the
aggregated option. systemd-networkd doesn't follow RFC 3396 and
doesn't aggregate multiple options; it considers only the last
occurrence of each option.
Change the parsing code to accept options that are longer than
necessary.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/324
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.
"nm-dbus-compat.h" was GPL licensed. That is a problem, because we use it from
libnm (which is LGPL).
The history of this file in NetworkManager source tree:
$ git shortlog -n -s e055bdbbc3 -- shared/nm-std-aux/nm-dbus-compat.h include/nm-dbus-compat.h shared/nm-dbus-compat.h
5 Thomas Haller
1 Dan Winship
1 Lubomir Rintel
Note that commit dd0e198955 ('include: add nm-dbus-compat.h')
introduced this file from dbus sources ([1]). Hence, originally
the file is (like all of dbus sources) dual-licensed under GPL-2.0+
and Academic Free License 2.1 (AFL-2.1). That makes it problematic to
change the license of this file to LGPL also because of the old history
of the file.
Instead, drop everything from the header except the bits that we
actually use. I claim the remainder is trivial and only contains
defines for documented D-Bus API. I don't think that the remainder
is copyrightable and hence get rid of the copy-right notice and the
GPL license.
[1] 39ea37b587/dbus/dbus-shared.h
