With gcc-10.2.1-9.fc33.s390x we get a (false positive) warning:
src/platform/tests/test-common.c: In function nmtstp_acd_defender_new:
src/platform/tests/test-common.c:2688:15: error: probe may be used uninitialized in this function [-Werror=maybe-uninitialized]
2688 | *defender = (NMTstpAcdDefender){
| ^
src/platform/tests/test-common.c:2656:56: note: probe was declared here
2656 | NAcdProbe * probe;
| ^
When building without "more-asserts" and LTO enabled, we can get
a warning about uninitalized "obj" variable:
src/platform/nm-linux-platform.c: In function 'ip_route_add':
src/platform/nm-platform.c:4761:24: warning: 'MEM[(struct NMPlatformIPRoute *)&obj + 24B].rt_source' may be used uninitialized in this function [-Wmaybe-uninitialized]
4761 | route->rt_source = nmp_utils_ip_config_source_round_trip_rtprot(route->rt_source);
| ^
src/platform/nm-platform.h:2139:25: warning: 'BIT_FIELD_REF <MEM[(const struct NMPlatformIPRoute *)&obj + 24B], 8, 72>' may be used uninitialized in this function [-Wmaybe-uninitialized]
2139 | return r->table_any ? 254u /* RT_TABLE_MAIN */
|
That is due to the "default" switch case which was unhandled
when building without more-asserts". Avoid that by reworking the
code.
On copr builds, the unit tests sometimes fail to create a veth
interface. In those cases, kernel rejects the netlink request
with EPERM. copr uses mock on Fedora 33 hosts.
I think this is a kernel bug. Add a workaround by retrying a few times.
Linux headers and some libc headers have overlapping defines
for network types and functions.
In the past years, glibc and linux headers were improved to cooperate
so you could include either one, in any order.
With musl and possibly some older glibc versions that doesn't work so
well.
Reorder and change includes to make it work better. Yes, this looks
pretty random and unmotivated. The includes are changed in order to
successfully build on various libc/kernel versions, with the goal
of not using #if.
Currently libnm headers include <linux/if_{ether,infiniband,vlan}.h>.
These are public headers, that means we drag in the linux header to all
users of <NetworkManager.h>.
Often the linux headers work badly together with certain headers from libc.
Depending on the libc version, you have to order linux headers in the right
order with respect to libc headers.
We should do better about libnm headers. As a first step, assume that
the linux headers don't get included by libnm, and explicitly include
them where they are needed.
I want to add more such accessors, because they are the base for
the corresponding for-each macros.
Add a helper macro _nm_platform_dedup_multi_iter_next() to do that,
which should make it simpler to add these nm_platform_dedup_multi_iter_next*()
functions.
Note that previously these functions were inline functions, now they are
macros. I think there is very little difference here. Also before those
functions could be entirely inlined. By using the macro the result
doesn't really change.
One difference is that we now require an "out" pointer. Previously that
was not required, but I guess it makes little sense otherwise.
Wext is anyway deprected. Our NMWifiUtilsClass should not have API
to accomodate it. That means, we don't need dedicated get_rate(),
get_bssid(), get_qual() hooks, when they all are only called by
get_station().
Instead, push the Wext specific code down.
These are unused now so remove them and revert most of
e0394689b3 which attempted to fix the same
issue of the platform wifi API not mapping well the nl80211 commands
resulting in redundant netlink commands being used.
In the wext driver there are still three individual getters for the
three values and nm_wifi_utils_get_station() uses either these or the
collective get_station method depending on the driver.
Merge nm_platform_wifi_get_bssid, nm_platform_wifi_get_quality,
nm_platform_wifi_get_rate into one utility, nm_platform_wifi_get_station
that uses the single NL80211_CMD_GET_STATION command dump when the
nl80211 driver is used. With wext each function mapped to one ioctl
while with nl80211 all three can be obtained with one netlink command.
The new function should use the minimum number of calls with either
driver.
Add a parameter to the 'link_add()' virtual function so that
the MTU will be configured (via netlink) by the kernel when
creating the link.
https://bugzilla.redhat.com/show_bug.cgi?id=1778590
Signed-off-by: Antonio Cardace <acardace@redhat.com>
- we commonly use "int addr_family" as parameters to functions.
But then inside the function, we often need to do something for
IPv4 or IPv6 specifically. Instead of having lots of redundant
"if (addr_family == AF_INET)" checks, prefer to have a variable
IS_IPv4 and/or use NM_IS_IPv4() macro.
- don't make the "IS_IPv4" variable a gboolean but an int. gboolean
is a typedef for int, so it's in practice exactly the same. However,
we use "IS_IPv4" as index to arrays of length 2, where at position
"1" we have the value related to IPv4. Using a gboolean to index
an array is a bit odd. Maybe a "int" is preferable here.
This is more about doing consistently one or the other. There are
no strong reasons to prefer gboolean or int.
The DHCP client likes to order multiple default routes by adding
them with different, increasing metric.
To support that, let "metric_any" not completely disable the "metric"
field, but instead interpret it as an offset that should be added to
the default metric.
The wifi backends call nm_platform_wifi_get_quality and
nm_platform_wifi_get_rate one after another in periodic_update (every
6s) and the same information is queried twice, synchronously. For the
lack of a better mechanism to decide whether we're still inside the same
periodic_update call, store the timestamp in msecs and reuse the data
for 500ms.
As an optimization, use the NL80211_CMD_GET_STATION dump data instead
of the NL80211_CMD_GET_SCAN dump + GET_STATION command (non-dump) to
implement the following methods:
wifi_nl80211_get_bssid
wifi_nl80211_get_rate
wifi_nl80211_get_qual
GET_STATION records vary in size from a few hundred bytes to a few kB.
GET_SCAN records are usually on the few hundred bytes side, but there
can be many of them. In managed mode there will only be one
GET_STATION record. In AdHoc mode there may be more. These methods are
not used in AP or Mesh mode.
So without that patch we'd have a GET_SCAN dump that could be quite big
and then a GET_STATION with one record. Now it should be a GET_STATION
dump with one record or a few records, in any case fewer synchronous
commands is better. Additionally this should now not depend on the
currently-connected BSS being in the kernel's scan result cache.
The downside is that the signal strength is "optional" in the
GET_STATION records, depends on the driver's capabilities. Most
mainline drivers do seem to include it (the mac80211 based ones and a
few full-mac ones) but I can't know if all of them do.
As an optimization, implement wifi_nl80211_get_freq() using the
GET_INTERFACE nl8022 command instead of the GET_SCAN dump.
The GET_SCAN dump can be over 10kB of data that the kernel has to build
and we have to parse. Additionally the GET_SCAN dump is not guaranteed
to contain the currently-connected BSS if there was no recent scan (30s),
or if the recent scan missed the beacon from the current BSS, or if the
recent scan was for a subset of channels/SSIDs/BSSIDs etc. and the last
full scan was already flushed. Scan results are flushed after (I think)
30 seconds or if a new scan has the flush flag set.
In IWD we do occasionally do partial scans (on a subset of channels or
for a specific SSID) with the flush flag. In that case the previous
wifi_nl80211_get_freq() logic would probably return 0.
It is not yet used, but it will be used to mark instances that
are not supposed to be configured in platform, because ACD is
either still pending of failed.
As one of the arguments in unsigned, the calculation is performed as
unsigned integers. That can actually lead to the wrong result. Fix it by
casting to the right (signed) types.
When we (for example) receive a DHCP lease, we track the routes that
should be configured via NMPlatformIP[46]Route instances. Thus, this
structure does not only track the routes that are configured (and
cached in NMPlatform), but it is also used to track the routes that
we want to configure.
This is also the case with the "rt_source" field, which represents the
NMIPConfigSource enum for routes that we want to configure, but
for routes in the cache it corresponds to rtm_protocol.
Note that NMDhcpClient creates NMIP4Config instances, which tracks the
routes as NMPlatformIP4Route instances. Previously, NMDhcpClient didn't
have any way to leave the table/metric undecided, but this information
isn't part of the DHCP lease tself. Instead, NMDevice knows the table/metric
to use. This has various problems:
- NMDhcpClient needs to know the table/metric, for no other purpose
than to set the value when creating the NMIP4Config instance for the
lease. We first pass the information down, only so that it can be
returned with the lease information.
- during reapply or when connectivity check changes, the effectively
used table/metric can change. Previously, we would have to
re-generate the NMIP4Config instances.
Improve that by allowing to leave the table/metric undecided. Higher
layers can decide the effective metric to use.
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
nm_utils_inet4_ntop() is public API of libnm. Also, it accepts a
%NULL buffer to use a static buffer. That is error prone and we
should not use such convenience behavior for our own code.
