We want to use this by "shared/nm-platform", which should have
no dependency on "libnm-core".
Move "libnm-core/nm-ethtool-utils.h" to "libnm/nm-ethtool-utils.h" so
that it is only used by libnm. This file contains the defines for
the option names.
Also, symlink "libnm/nm-ethtool-utils.h" as "shared/nm-base/nm-ethtool-utils-base.h".
We want to use the same defines also internally. Since they are both
public API (must be in libnm) and should be in "shared/nm-base", this
is the way.
We want to use these defines for option names also in "shared/nm-base"
(and in turn in "shared/nm-platform), which cannot include "libnm-core".
However, they are also public API of libnm.
To get this done, in a first step, move these defines to a new header
"libnm-core/nm-ethtool-utils.h".
Since now the name "nm-ethtool-utils.h" is taken, also rename
nm-libnm-core-intern files.
There should be a clear hierarchie of dependency. That is,
"nm-platform.h" may use "nm-platform-utils.h", but not the
other way around.
Move nm_platform_link_duplex_type_to_string().
Currently src/platform depends on libnm-core. libnm-core is large
optimally we have a better separation between our code. That means
libnm-core does not depend on platform and vice versa.
However, nm-platform re-uses some enums from libnm-core for internal code.
To avoid that dependency, add _NMSettingWiredWakeOnLan as a duplicate to
nm-base/nm-base.h. nm-base can both be used by libnm-core, nm-platform
and src/platform.
The only problem is that NMSettingWiredWakeOnLan is also part of public
API of libnm. It means, we must duplicate the enum. But with several
static assertions in unit tests I think that is not a problem to do.
Our dependencies are complicated.
Currently "src/platform" uses parts of libnm-core and is relatively
strongly entangled with core. It would be nice to have that part
clearly independent from "src" and from "libnm-core".
Also, "src/platform/nm-platform-utils.h" uses NMEthtoolID enum, which
previously was defined in "libnm-core/nm-libnm-core-intern/nm-ethtool-utils.h".
Move that to a new place "shared/nm-base/nm-base.h".
Note that we have "libnm-core/nm-libnm-core-intern", which is
libnm/core related code which uses and is used by libnm-core.
There is a need for a library which is used by libnm-core, but
does not depend on libnm-core itself. Here comes "shared/nm-base".
Yes, many libraries. But the goal is to entangle the dependencies
and have a clear hierarchy of includes. And to have "shared/nm-platform"
independent of libnm-core.
NetworkManager core is huge. We should try to split out
parts that are independent.
Platform code is already mostly independent. But due to having it
under "src/", there is no strict separation/layering which determines
the parts that can work independently. So, while the code is mostly
independent (in practice), that is not obvious from looking at the
source tree. It thus still contributes to cognitive load.
Add a shared library "shared/nm-platform", which should have no
dependencies on libnm-core or NetworkManager core.
In a first step, move the netlink code there. More should follow.
This is the same as libnm's nm_utils_hwaddr_aton(), which however
is public API.
We want to use this function also without libnm(-core). Hence add
the helper to "shared/nm-glib-aux".
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.
