There should be no change in behavior, but this way seems nicer.
Now _nmc_mangle_connection() doesn't return FALSE, it always
will try to mangle the connection and requires the caller to
first check whether that is appropriate.
Just move some code outside of _nmc_mangle_connection() and let
the caller check for the skip first.
The point is consistency, as the caller already does some checks to
whether skip the reapply. So it should do all the checks, so that
"mangle" never fails/skips.
Externally added IP addresses/routes should be preserved by
nm-cloud-setup. This allows other tools to also configure the interface
and the Reapply() call from nm-cloud-setup would not interfere
with those tools.
https://bugzilla.redhat.com/show_bug.cgi?id=2132754
In the end, it turned out I don't need them. They still seem useful,
because they show how to use this API. In particular for how the
bitfield should be parsed.
Reapply() is supposed to make sure that the system (the interface)
is configured as indicated by the applied-connection. That means,
it will remove/add configuration to make the system match the requested
configuration.
Add a flag "preserve-external-ip" which relaxes this. During reapply,
IP addresses/routes that exist on the interface and which are not known
(or added) by NetworkManager will be left alone.
This will be used by nm-cloud-setup, so that it can reconfigure the
interface in a less destructive way, which does not conflict with
external `ip addr/route` calls.
Note that the previous commit just adds "VersionInfo" and the
possibility to expose capabilities (patch-level). This is not used
for the new reapply flag, because, while we might backport the
reapply flag, we won't backport the "VersionInfo" property. Exposing
new capabilities via the "VersionInfo" property will only become useful
in the future, where we can backport a capability to older NM versions
(but those that have "VersionInfo" too).
Changing an error code is an API change. But, so far no flags existed,
so it's unlikely that somebody would send invalid flags or care about
the return code.
This exposes NM_VERSION as number (contrary to the "Version", which is a
string). That is in particular useful, because the number can be
compared with <> due to the encoding of the version.
While at it, don't make it a single number. Expose an array of numbers,
where the following numbers are a bitfield of capabilities.
Note that before commit 3c67a1ec5e ('cli: remove version check against
NM'), we used to parse the "Version" string to detect the version. As
such, the information that "VersionInfo" exposes now, was already
(somewhat) available, you just had to parse the string. The main benefit of
"VersionInfo" is that it can expose capabilities (patched behavior) in
in a lightweight bitfield. To include the numerical version there is
just useful on top.
Currently no additional capabilities are exposed. The idea is of course
to have a place in the future, where we can expose additional
capabilities. Adding a capability flag is most useful for behavior that we
backport to older branches. Otherwise, we could just check the daemon version
alone. But since we only add "VersionInfo" property only now, we cannot backport
any capability further than this, because the "VersionInfo" property itself
won't be backported. As such, this will only be useful in the future by having
a place where we can add (and backport) capabilities.
Note that there is some overlap with the existing "Capability" property
and NMCapability enum. The difference is that adding a capability via "VersionInfo"
is only one bit, and thus cheaper. Most importantly, having it cheaper means
the downsides of adding a capability flag is significantly removed. In
practice, we could live without capabilities for a long time, so they
must be very cheap for them to be worth to add. Another difference might be,
that we will want that the VersionInfo is about compile time defaults (e.g.
a certain patch/behavior that is in or not), while NM_CAPABILITY_TEAM depends on
whether the team plugin is loaded at runtime.
Introduce a "vlan.protocol" property that specifies the protocol of a
VLAN, which controls the tag (EtherType) used for encapsulation.
Regular VLANs use 802.1Q (tag 0x8100). To implement VLAN stacking it's
sometimes useful to have 802.1ad VLANs with tag 0x88A8.
The property is a string instead of e.g. an enum because this allows
maximum flexibility in the future. For example, it becomes possible to
specify an arbitrary number in case if the kernel ever allows it.
With gcc-12.2.1-4.fc37 on i686 we get:
./src/libnm-platform/nmp-object.h: In function 'nmp_object_ref':
./src/libnm-platform/nmp-object.h:626:12: error: cast increases required alignment of target type [-Werror=cast-align]
626 | return (const NMPObject *) nm_dedup_multi_obj_ref((const NMDedupMultiObj *) obj);
| ^
cc1: all warnings being treated as errors
Work around that be increasing the alignment of NMDedupMultiObj.
It has no downsides, because we usually put a NMDedupMultiObj in heap
allocated memory, which is already suitably aligned. Or we put it on
the stack, where wasting a few bytes for the alignment doesn't matter.
We basically never embed NMDedupMultiObj in an array where the increase
of alignment would waste additional space.
There are two benefits:
- the returned (allocated) string will have exactly the required
length and no excess buffer that was used to build the string.
- the string is (most likely) short enough to fit in 488 bytes on the
stack. There is no re-allocation necessary to grow the buffer.
The warning "-Wcast-align=strict" seems useful and will be enabled
next. Fix places that currently cause the warning by using the
new macro NM_CAST_ALIGN(). This macro also nm_assert()s that the alignment
is correct.
GArray.data is a char pointer. Most of the time we track other data in
a GArray. Casting that pointer can trigger "-Wcast-align=strict"
warnings.
Avoid them. Most of the time, instead use the nm_g_array*() helpers,
which also assert that the expected element size is correct.
We put all these structs inside the tagged union NMPObject.
Also, in a sense NMPlatformObject is the base "type" of all
these structs, meaning, it should be able to up and downcast.
Ensure the alignment matches.
This helps to avoid "-Wcast-align" warnings when trying to cast
a (NMPlatformObject*) to another (NMPlatformXXX *) type. Something
we commonly do.
nm_g_array_first_p() is a convenient helper to get the pointer to
the first index. But this one should also accept that the array is NULL,
has array->data as NULL or is empty.
nm_utils_get_ipv6_interface_identifier() has non-obvious requirements on
the hardware address. If the caller passes a wrong length, it will
trigger an assertion or even cause out of bound read. This would mean
that the caller needs to carefully check the length. Such requirements
on the caller are wrong.
Also, in practice the hardware length comes from platform/kernel. We
don't want to trust that what kernel tells us always has the required
address length, so the caller would always have to double check before
calling the function.
Instead, handle unexpected address lengths.
Fixes: e2270040c0 ('core: use Interface Identifiers for IPv6 SLAAC addresses')
Fixes: 1d396e9972 ('core-utils: use 64-bit WPAN address for a 6LoWPAN IID')
For link type NM_LINK_TYPE_6LOWPAN, nm_utils_get_ipv6_interface_identifier()
expects 8 bytes hardware address. It even just accesses the buffer
without checking (that needs to be fixed too).
For 6lowpan devices, the caller might construct a fake ethernet MAC
address, which is only 6 bytes long. So wrong.
Fixes: 49844ea55f ('device: generate pseudo 48-bit address from the WPAN short one')
- with nm_assert(), if the argument is a compile time constant
always check it (regardless of NDEBUG, G_DISABLE_ASSERT)
and mark the failure as _nm_unreachable_code(). We do this,
even if we usually would not evaluate run time checks with
NDEBUG/G_DISABLE_ASSERT.
- with nm_assert_se(), if assertions are disabled with NDEBUG
and G_DISABLE_ASSERT, still mark the path as _nm_unreachable_code().
This is useful, because it can avoid compiler warnings that are
emitted if the compiler things that the code can be reached.
_nm_assert_fail() can clearly never be reached (unless a bug happens).
When compiling we can disable assertion checks with
NDEBUG/G_DISABLE_ASSERT, but if we know that an assertion must not be
hit (for example with nm_assert_not_reached()) then we still want to
mark the path as unreachable, even if assert() does not abort the
process.
This allows the compiler to see that nm_assert(0) is unreachable code.
That is because nm_assert(0) calls NM_LIKELY(0), which calls
NM_BOOLEAN_EXPR(0). The latter was a statement expression, which
to the compiler was not a constant expression. Hence, this may trigger
compiler warnings about uninitialized variables.
Let NM_BOOLEAN_EXPR() to be constant, if the arguments are.
This can avoid compiler warnings in some cases.
Note that __builtin_choose_expr(__builtin_constant_p(...), ...) does
not properly work with gcc 4.8 ([1]). Hence only do macro shenanigans
with a newer gcc. Then entire point of NM_BOOLEAN_EXPR() is anyway
to preserve the "-Wparentheses" warning (while only evaluating the
argument once, being safe with nested invocations, propagate constness).
If we don't care about "-Wparentheses", it should be the same as
(!!(expr)). We can ignore that on non-recent gcc.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
Glib 2.58+ improved the implementation of the g_clear_pointer() macro,
and indirectly of g_clear_object(), which uses it.
Note that we don't use the 2.58+ version, because our GLIB_VERSION_MAX_ALLOWED
is too old.
Also note that we don't use g_clear_pointer() directly. Instead, we have
and use nm_clear_pointer() everywhere.
Still, it would be nice if also g_clear_object() uses the improved
variant. Arguably, this is less relevant, because g_clear_object() calls
g_unref_object() which accepts a void pointer and thus there isn't much
type-safety to gain. Still, there is a small gain, so do it.
We could:
1) replace all uses of g_clear_object() with nm_clear_g_object() and outlaw
both g_clear_object() and g_clear_pointer(). This is what's done for
nm_clear_pointer(), which should be used instead of g_clear_pointer().
The advantage is that we don't monkey-patch glib (which might surprise users).
The disadvantage is that g_clear_pointer() is well known, while nm_clear_pointer()
is not. This is mitigated by the fact that nm_clear_pointer() behaves very similar
to g_clear_pointer() and in all cases where you legally could use
g_clear_pointer(), nm_clear_pointer() works to the same effect (but not vice
versa).
2) silently redefine the glib helper to use our improved implementation. This is
done for g_clear_error(), which is redefined to nm_clear_error().
The advantage is that it appears as if we would use glib functionality.
The disadvantage is that this is not exactly the glib variant.
This too is mitigated by the fact that our patched g_clear_error()
should work the same, wherever you can legally use glib's variant (but not
vice versa).
Let's do 2).
In this case, let g_clear_pointer() behaves exactly like glib 2.58+'s variant,
and not like nm_clear_pointer(). This is to reduce any potential surprise.
nm_clear_pointer() is still better. Still use that over
g_clear_pointer(). This change is for g_clear_object().
The fields "l3cfg" and "l3cfg_" are union aliases. One of them is const,
the other is not. The idea is that all places that modify the field need
to use the special name "l3cfg_", and grepping for that will lead you to
all the relevant places.
This mistake happened, because g_clear_object() casts constness away.
Fixes: 58287cbcc0 ('core: rework IP configuration in NetworkManager using layer 3 configuration')
"connection" variable might be NULL, which fails an assertion in
g_dbus_connection_flush_sync(). Consequently, "error_flush" is also
NULL which leads to a crash of "nm-dhcp-helper".
Reported-by: Jules Maselbas <jmaselbas@zdiv.net>
Fixes: 240ec7f891 ('dhcp: implement ACD (address collision detection) for DHCPv4')
A loopback interface cannot be attached to a controller interface (in kernel).
Also, we have special handling for the loopback address 127.0.0.1. It's
not clear how that should behave when the loopback device would be
attached to another interface.
Just reject such configuration as invalid.
Fixes: e8618f03d7 ('support loopback interface')
