NM_FLAGS_HAS() uses a static-assert that the second argument is a
single flag (power of two). With a single flag, NM_FLAGS_HAS(),
NM_FLAGS_ANY() and NM_FLAGS_ALL() are all identical.
The second argument must be a compile time constant, and if that is
not the case, one must not use NM_FLAGS_HAS().
Use NM_FLAGS_ANY() in these cases.
We're going to need that one for TC filter & action support.
<linux/tc_act/tc_defact.h> was moved to user-space API only in 2013
by commit 5bc3db5c9ca8407f52918b6504d3b27230defedc. Our travis CI currently
fails to build due to that.
Re-implement the header.
It only makes sense to call delete() with NMPObjects that
we obtained from the platform cache. Otherwise, if we didn't
get it from the cache in the first place, we wouldn't know
what to delete.
Hence, the input argument is (almost) always an NMPObject
in the first place. That is different from add(), where
we might create a new specific NMPlatform* instance on the
stack. For add() it makes slightly more sense to have different
functions depending on the type. For delete(), it doesn't.
We also do this for libnm, where it causes visible changes
in behavior. But if somebody would rely on the hashing implementation
for hash tables, it would be seriously flawed.
The file descriptor is owned by the netlink socket instance,
which we close in finalize. We most not close it when destroying
the IO channel, otherwise the file descriptor gets closed twice.
Closing an invalid file descriptor (or a descriptor that is already closed)
is a serious bug, because the integer values are re-used, so there is a race
that the close might affect an innocent file descriptor instead of just
failing with EBADF.
The "onlink" flag for IPv4 routes is part of the route ID.
Consider it in nm_platform_ip4_route_cmp().
Also, allow configuring the flag when adding a route.
Note that for IPv6, the onlink flag is still ignored.
Pretty much like kernel does.
We need to pass more alias-types. Instead of having numbered
versions, use variadic number of macro arguments.
Also, fix build failure with old compiler:
In file included from src/nm-ip6-config.c:24:
./src/nm-ip6-config.h:44:29: error: controlling expression type 'typeof (ipconf_iter->current->obj)' (aka 'const void *const') not compatible with any generic association type
*out_address = has_next ? NMP_OBJECT_CAST_IP6_ADDRESS (ipconf_iter->current->obj) : NULL;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: b1810d7a68
_NM_GET_PRIVATE() used typeof() to propagate constness of the @self
pointer. However, that means, it could only be used with a self pointer
of the exact type. That means, you explicitly had to cast from (GObject *)
or from (void *).
The requirement is cumbersome, and often led us to either create @self
pointer we didn't need:
NMDeviceVlan *self = NM_DEVICE_VLAN (device);
NMDeviceVlanPrivate *priv = NM_DEVICE_VLAN_GET_PRIVATE (self);
or casting:
NMDeviceVlanPrivate *priv = NM_DEVICE_VLAN_GET_PRIVATE ((NMDevice *) device);
In both cases we forcefully cast the source variable, loosing help from
the compiler to detect a bug.
For "nm-linux-platform.c", instead we commonly have a pointer of type
NMPlatform. Hence, we always forcefully cast the type via _NM_GET_PRIVATE_VOID().
Rework the macro to use _Generic(). If compiler supports _Generic(), then we
will get all compile time checks as desired. If the compiler doesn't support
_Generic(), it will still work. You don't get the compile-time checking of course,
but you'd notice that something is wrong once you build with a suitable
compiler.
30. NetworkManager-1.9.2/src/settings/plugins/keyfile/nms-keyfile-writer.c:218:
check_return: Calling "g_mkdir_with_parents" without checking return
value (as is done elsewhere 4 out of 5
times).
25. NetworkManager-1.9.2/src/platform/nm-linux-platform.c:3969:
check_return: Calling "_nl_send_nlmsg" without checking return value (as
is done elsewhere 4 out of 5 times).
34. NetworkManager-1.9.2/src/nm-core-utils.c:2843:
negative_returns: "fd2" is passed to a parameter that cannot be negative.
26. NetworkManager-1.9.2/src/devices/wwan/nm-modem-broadband.c:897:
check_return: Calling "nm_utils_parse_inaddr_bin" without checking
return value (as is done elsewhere 4 out of 5 times).
3. NetworkManager-1.9.2/src/devices/bluetooth/nm-bluez5-manager.c:386:
check_return: Calling "g_variant_lookup" without checking return value
(as is done elsewhere 79 out of 83 times).
16. NetworkManager-1.9.2/libnm-util/nm-setting.c:405:
check_return: Calling "nm_g_object_set_property" without checking return
value (as is done elsewhere 4 out of 5 times).
Setting the MTU failes under regular conditions, for example when
setting the MTU of a master larger then the MTU of the slaves.
Logging a warning it too alarming.
When comparing an unsigned and a signed integer, the signed integer
is promoted to unsigned, resulting in a very large number.
See the checks "nwrote < len - 1", where nwrote might be -1
to indicate failure. The condition would not be TRUE due to
promoting -1 to the max int value.
Hence, sysctl_set() was rather wrong.
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
These static variables really never be modified.
Mark them as const, which allows the linker to mark them as
read-only.
The problem is libnl3's API, which has these parameters
not as const. Add a workaround for that. Clearly libnl3 is
not gonna modify the policy, that the API was fixed too [1]
[1] b4802a17a7
Kernel does not allow to add a route with table 0 (RT_TABLE_UNSPEC). It
effectively is an alias for the main table. We must consider that when
comparing routes sementically.
No need for duplicate log lines
<debug> [1506146476.8462] platform: link: adding tap tap0 owner 107 group -1
<debug> [1506146476.8462] platform-linux: link: add tap tap0 owner 107 group -1
Merge them.
Also, for consistency change the logging output for adding generic
interfaces in nm_platform_link_add().
After commit 5a69b27a64 ("platform: let platform operations only
consider kernel response") the platform only relies on kernel messages
and doesn't check if a deleted object is gone from the cache. For IPv6
addresses it can happen that the RTM_DELADDR comes after the ack, and
this causes random failures in test /address/ipv6/general-2:
[10.8009] platform: address: deleting IPv6 address 2001:db8:a🅱️1:2:3:4/64, ifindex 12 dev nm-test-device
[10.8009] platform-linux: delayed-action: schedule wait-for-nl-response (seq 55, timeout in 0.199999680, response-type 0)
[10.8009] platform-linux: delayed-action: handle wait-for-nl-response (any)
[10.8009] platform-linux: netlink: recvmsg: new message (2), flags 0x0100, seq 55
[10.8009] platform-linux: delayed-action: complete wait-for-nl-response (seq 55, timeout in 0.199980533, response-type 0, success)
[10.8009] platform-linux: do-delete-ip6-address[12: 2001:db8:a🅱️1:2:3:4]: success
**
NetworkManager:ERROR:src/platform/tests/test-common.c:1127:_ip_address_del: assertion failed: (external_command)
Use the same workaround in place for the addition of IPv6 addresses,
i.e. refetch the object if the address is still present after the ack.
When unenslaving an interface from a bridge, kernel sends a RTM_DELLINK
message with ifi_family AF_BRIDGE. We only care about regular
RTM_DELLINK/RTM_NEWLINK messages, ignore all but ifi_family AF_UNSPEC.
There is also test_nl_bugs_spuroius_dellink(), added in commit
8a87a91813 for related bug rh#1302037.
The workaround was masking a bug in NetworkManager (to not ignore AF_BRIDGE
messages) and can now be removed as well.
Also downgrade <error> logging messages to <warn>. An external
condition should never be able to trigger an <error>, and clearly
there is always a external race that can cause a netlink command
to fail.
Let nm_platform_ip_route_add() and friends return an NMPlatformError
failure reason.
Also, do_add_addrroute() did not return the response from kernel.
Instead, it determined success/failure based on the presence of the
object in the cache. That is racy and does not allow to give a failure
reason from kernel.
Instead, determine success solely based on the netlink reply from
kernel. The received errno shall be authorative, there is no need
to second guess the response.
There is a problem that netlink is not a reliable protocol. In case
of receive buffer overflow, the response is lost and we don't know
whether the command succeeded (it likely did). It's unclear how to fix
that, but for now just return "unspecified" error. We probably avoid
that already by having a huge buffer size.
Also, downgrade the error message to <warn> level. <error> is really
for bugs only.
Inspired from iproute2. As such, don't use libnl3's "struct nl_msg", but
add _nl_addattr_l() and use a stack-allocated "struct nlmsghdr". With
this, we are closer to the raw netlink API. It really is simple enough.
The complicated part of the patch is that we re-use the existing netlink
socket for events. Hence, we must process the socket via our common
event_handler_recvmsgs(). That also means, that we get the netlink
response a few layers down the stack and have to return the result
via DelayedActionWaitForNlResponseData.
This will make us stop worry how relevant are chunks of compat code with
older kernels when deciding whether it's worth supporting/testing them.
As if we actually were testing old kernels.
- cache the result in NMPlatformPrivate. No need to call the virtual
function every time. The result is not ever going to change.
- if we are unable to detect support, assume support. Those features
were added quite a while ago to kernel, we should default to "support".
Note, that we detect support based on the presence of the absence of
certain netlink flags. That means, we will still detect no support.
The only moment when we actually use the fallback value, is when we
didn't encounter an RTM_NEWADDR or AF_INET6-IFLA_AF_SPEC message yet,
which would be very unusual, because we fill the cache initially and
usually will have some addresses there.
- for no strong reason, track "undetected" as numerical value zero,
and "support"/"no-support" as 1/-1. We already did that previously for
_support_user_ipv6ll, so this just unifies the implementations.
The minor reason is that this puts @_support_user_ipv6ll to the BSS
section and allows us to omit initializing priv->check_support_user_ipv6ll_cached
in platforms constructor.
- detect _support_kernel_extended_ifa_flags also based on IPv4
RTM_NEWADDR messages. Originally, extended flags were added for IPv6,
and later to IPv4 as well. Once we see an IPv4 message with IFA_FLAGS,
we know we have support.
