It would be better if we would be able to use NMLinkType enum
as an index (e.g. into an array of LinkDesc structures). For that,
it is necessary that the enum is just consecutive numbers.
Don't assign special meaning to the enum. Also, this was only
used at two places, that we can solve differently.
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
The _GET_PRIVATE() macros are all implemented based on
_NM_GET_PRIVATE(). That macro tries to be more type safe and uses
_Generic() to do the right thing. Explicitly casting is not only
unnecessary, it defeats these (static) type checks.
Don't do that.
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)
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘nmp_utils_ethtool_get_permanent_address’:
src/platform/nm-platform-utils.c:854:29: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[0]’} [-Werror=zero-length-bounds]
854 | if (NM_IN_SET (edata.e.data[0], 0, 0xFF)) {
./shared/nm-glib-aux/nm-macros-internal.h:731:20: note: in definition of macro ‘_NM_IN_SET_EVAL_N’
Fix this warning.
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘ethtool_get_stringset’:
src/platform/nm-platform-utils.c:355:27: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[0]’} [-Werror=zero-length-bounds]
355 | len = sset_info.info.data[0];
| ~~~~~~~~~~~~~~~~~~~^~~
In file included from src/platform/nm-platform-utils.c:12:
/usr/include/linux/ethtool.h:647:8: note: while referencing ‘data’
647 | __u32 data[0];
| ^~~~
Fix this warning.
I think this solution is not right, because "char buf" is not guaranteed
to have the correct alignment. Revert, and solve it differently.
This reverts commit 6345a66153.
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘nmp_utils_ethtool_get_permanent_address’:
src/platform/nm-platform-utils.c:854:29: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[0]’} [-Werror=zero-length-bounds]
854 | if (NM_IN_SET (edata.e.data[0], 0, 0xFF)) {
./shared/nm-glib-aux/nm-macros-internal.h:731:20: note: in definition of macro ‘_NM_IN_SET_EVAL_N’
Fix this warning.
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘ethtool_get_stringset’:
src/platform/nm-platform-utils.c:355:27: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[0]’} [-Werror=zero-length-bounds]
355 | len = sset_info.info.data[0];
| ~~~~~~~~~~~~~~~~~~~^~~
In file included from src/platform/nm-platform-utils.c:12:
/usr/include/linux/ethtool.h:647:8: note: while referencing ‘data’
647 | __u32 data[0];
| ^~~~
Fix this warning.
and _nm_utils_inet6_ntop() instead of nm_utils_inet6_ntop().
nm_utils_inet4_ntop()/nm_utils_inet6_ntop() are public API of libnm.
For one, that means they are only available in code that links with
libnm/libnm-core. But such basic helpers should be available everywhere.
Also, they accept NULL as destination buffers. We keep that behavior
for potential libnm users, but internally we never want to use the
static buffers. This patch needs to take care that there are no callers
of _nm_utils_inet[46]_ntop() that pass NULL buffers.
Also, _nm_utils_inet[46]_ntop() are inline functions and the compiler
can get rid of them.
We should consistently use the same variant of the helper. The only
downside is that the "good" name is already taken. The leading
underscore is rather ugly and inconsistent.
Also, with our internal variants we can use "static array indices in
function parameter declarations" next. Thereby the compiler helps
to ensure that the provided buffers are of the right size.
This is also what iproute2 does ([1]) when creating a default broadcast address
with `ip addr add 192.168.1.5/24 brd + dev eth0`.
Also, kernel does in fib_add_ifaddr() ([2]):
```
__be32 addr = ifa->ifa_local;
__be32 prefix = ifa->ifa_address & mask;
...
/* Add broadcast address, if it is explicitly assigned. */
if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF))
fib_magic(RTM_NEWROUTE, RTN_BROADCAST, ifa->ifa_broadcast, 32,
prim, 0);
if (!ipv4_is_zeronet(prefix) && !(ifa->ifa_flags & IFA_F_SECONDARY) &&
(prefix != addr || ifa->ifa_prefixlen < 32)) {
if (!(ifa->ifa_flags & IFA_F_NOPREFIXROUTE))
fib_magic(RTM_NEWROUTE,
dev->flags & IFF_LOOPBACK ? RTN_LOCAL : RTN_UNICAST,
prefix, ifa->ifa_prefixlen, prim,
ifa->ifa_rt_priority);
/* Add network specific broadcasts, when it takes a sense */
if (ifa->ifa_prefixlen < 31) {
fib_magic(RTM_NEWROUTE, RTN_BROADCAST, prefix, 32,
prim, 0);
fib_magic(RTM_NEWROUTE, RTN_BROADCAST, prefix | ~mask,
32, prim, 0);
}
}
```
Which means by default kernel already adds those special broadcast routes which
are identical to what we configure with IFA_BROADCAST. However, kernel too bases
them on the peer (IFA_ADDRESS).
[1] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/ipaddress.c?id=d5391e186f04214315a5a80797c78e50ad9f5271#n2380
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/fib_frontend.c?id=bef1d88263ff769f15aa0e1515cdcede84e61d15#n1109
- track the broadcast address in NMPlatformIP4Address. For addresses
that we receive from kernel and that we cache in NMPlatform, this
allows us to show the additional information. For example, we
can see it in debug logging.
- when setting the address, we still mostly generate our default
broadcast address. This is done in the only relevant caller
nm_platform_ip4_address_sync(). Basically, we merely moved setting
the broadcast address to the caller.
That is, because no callers explicitly set the "use_ip4_broadcast_address"
flag (yet). However, in the future some caller might want to set an explicit
broadcast address.
In practice, we currently don't support configuring special broadcast
addresses in NetworkManager. Instead, we always add the default one with
"address|~netmask" (for plen < 31).
Note that a main point of IFA_BROADCAST is to add a broadcast route to
the local table. Also note that kernel anyway will add such a
"address|~netmask" route, that is regardless whether IFA_BROADCAST is
set or not. Hence, setting it or not makes very little difference for
normal broadcast addresses -- because kernel tends to add this route either
way. It would make a difference if NetworkManager configured an unusual
IFA_BROADCAST address or an address for prefixes >= 31 (in which cases
kernel wouldn't add them automatically). But we don't do that at the
moment.
So, while what NM does has little effect in practice, it still seems
more correct to add the broadcast address, only so that you see it in
`ip addr show`.
In several cases, the layer 2 and layer 3 type are very similar, also from
kernel's point of view. For example, "gre"/"gretap" and "ip6tnl"/"ip6gre"/"ip6gretap"
and "macvlan"/"macvtap".
While it makes sense that these have different NMLinkType types
(NM_LINK_TYPE_MACV{LAN,TAP}) and different NMPObject types
(NMPObjectLnkMacv{lan,tap}), it makes less sense that they have
different NMPlatformLnk* structs.
Remove the NMPlatformLnkMacvtap typedef. A typedef does not make things simpler,
but is rather confusing. Because several API that we would usually have, does
not exist for the typedef (e.g. there is no nm_platform_lnk_macvtap_to_string()).
Note that we also don't have such a typedef for NMPlatformLnkIp6Tnl
and NMPlatformLnkGre, which has the same ambiguity between the link type
and the struct with the data.
Previously NetworkManager would wrongly add a broadcast address for the
network prefix that would collide with the IP address of the host on
the other end of the point-to-point link thus exhausting the IP address
space of the /31 network and preventing communication between the two
nodes.
Configuring a /31 address before this commit:
IP addr -> 10.0.0.0/31, broadcast addr -> 10.0.0.1
If 10.0.0.1 is configured as a broadcast address the communication
with host 10.0.0.1 will not be able to take place.
Configuring a /31 address after this commit:
IP addr -> 10.0.0.0/31, no broadcast address
Thus 10.0.0.0/31 and 10.0.0.1/31 are able to correctly communicate.
See RFC-3021. https://tools.ietf.org/html/rfc3021https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/295https://bugzilla.redhat.com/show_bug.cgi?id=1764986
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.
This is necessary on Travis/Ubuntu 16.04, otherwise the test
fails with
# NetworkManager-MESSAGE: <warn> [1575301791.7600] platform-linux: do-add-link[nm-test-device/team]: failure 95 (Operation not supported)
Aborted (core dumped)
# test:ERROR:../src/platform/tests/test-link.c:353:test_software: assertion failed: (software_add (link_type, DEVICE_NAME))
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
- systemd-networkd and initscripts both support it.
- it seems suggested to configure routes with scope "link" on AWS.
- the scope is only supported for IPv4 routes. Kernel ignores the
attribute for IPv6 routes.
- we don't support the aliases like "link" or "global". Instead
only the numeric value is supported. This is different from
systemd-networkd, which accepts names like "global" and "link",
but no numerical values. I think restricting ourself only to
the aliases unnecessarily limits what is possible on netlink.
The alternative would be to allow aliases and numbers both,
but that causes multiple ways to define something and has
thus downsides. So, only numeric values.
- when setting rtm_scope to RT_SCOPE_NOWHERE (0, the default), kernel
will coerce that to RT_SCOPE_LINK. This ambiguity of nowhere vs. link
is a problem, but we don't do anything about it.
- The other problem is, that when deleting a route with scope RT_SCOPE_NOWHERE,
this acts as a wild care and removes the first route that matches (given the
other route attributes). That means, NetworkManager has no meaningful
way to delete a route with scope zero, there is always the danger that
we might delete the wrong route. But this is nothing new to this
patch. The problem existed already previously, except that
NetworkManager could only add routes with scope nowhere (i.e. link).
Track whether IP addresses were added by NM or externally. In this way
it becomes possible in a later commit to add prefix route only for
addresses added by NM.
The targets that involve the use of the `NetworkManager` library,
built in the `src` build file have been improved by applying a set
of changes:
- Indentation has been fixed.
- Set of objects used in targets have been grouped together.
- Aritificial dependencies used to group dependencies and custom
compiler flags have been removed and their use replaced with
proper dependencies and compiler flags to avoid any confussion.
