C casts unconditionally force the type, and as such they don't
necessarily improve type safety, but rather overcome restrictions
from the compiler when necessary.
Casting a void pointer is unnecessary (in C), it does not make the
code more readable nor more safe. In particular for g_object_new(),
which is known to return a void pointer of the right type.
Drop such casts.
sed 's/([A-Za-z_0-9]\+ *\* *) *g_object_new/g_object_new/g' $(git grep -l g_object_new) -i
./contrib/scripts/nm-code-format-container.sh
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>
Seems with LTO the compiler can sometimes think that thes variables are
uninitialized. Usually those code paths are only after an assertion was
hit (g_return*()), but we still need to workaround the warning.
NMIP[46]Config will become much simpler than it is today.
It's sole responsibility will be to expose current settings
on D-Bus, in it's function as a NMDBusObject subtype.
However, it still make sense to let them share a common base class.
Add it.
This code will change, but in essence we will still need such a function
to convert a list of addresses/routes to D-Bus. Extract the code, so it
can be better reused and adjusted.
In this case, the functions are only called once. Having a helper
function that has no clear, unique purpose does not necessarily make the
code simpler.
Also, NMIP[46]Config is going to change completely. It will thereby move
this code (and change it). Doing that is simpler, if we see all the
relevant parts in one place.
This code is not specific to "nm-ip4-config.h"/"nm-ip6-config.h".
It applies to everybody who wants to iterate over a dedup-multi-index of
certain NMPObjects. Move it.
and rename to nm_utils_ip_route_attribute_to_platform(). The function is independent
from NMIP4Config. We also will use it outside of NMIP4Config. Also, "NetworkManagerUtils.c"
already has similar functions that parse libnm structures to internal structures.
_nm_ip_config_best_default_route_set() doesn't really do anything
special. Use the generic helper function for the same job.
Also because NMIP4Config in the current form will be replaced by
something else, and this code needs to change.
Currently, we would not mark non-unicast routes with their type, so they
would wrongly appear as unicast routes in the D-Bus API.
That is wrong. For now, just hide them.
Fixes: 5d0d13f570 ('platform: add support for local routes')
When using VRF devices we must pre-generate dependent local
routes in the VRF's table otherwise they will be incorrectly added
to the local table instead.
https://bugzilla.redhat.com/show_bug.cgi?id=1857133
Fixes: a199cd2a7d ('core: add dependent local routes configured by kernel')
Pre-generate routes in the local table that are configured
by kernel when an ip-address is assigned to an interface.
This helps NM taking into account routes that are not to be deleted
when a connection is reapplied (or deactivated) on an interface instead of only
ignoring (when pruning) IPv6 routes having metric 0 and routes belonging
to the local table having 'kernel' as proto.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
(cherry picked from commit 3e5fc04df3)
Userspace cannot add IPv6 routes with metric 0. Trying to do that, will
be coerced by kernel to route metric 1024. For IPv4 this is different,
and metric zero is commonly allowed.
However, kernel itself can add IPv6 routes with metric zero:
# ip -6 route show table local
local fe80::2029:c7ff:fec9:698a dev v proto kernel metric 0 pref medium
That means, we must not treat route metric zero special for most cases.
Only, when we want to add routes (based on user configuration), we must
coerce a route metric of zero to 1024.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/563
(cherry picked from commit 1b408e243d)
The profile's "ipv4.gateway" and "ipv6.gateway" has only one real
purpose: to define the next hop of a static default route.
Usually, when specifying a gateway in this way, the default route from
other addressing methods (like DHCPv4 or IPv6 autoconf) gets ignored.
If you have a WireGuard peer with "AllowedIPs=0.0.0.0/0" and
"wireguard.peer-routes" enabled, NetworkManager would automatically add
a route to the peer. Previously, if the user also set a gateway, that
route was suppressed.
That doesn't feel right. Note that configuring a gateway on a WireGuard
profile is likely to be wrong to begin with. At least, unless you take
otherwise care to avoid routing loops. If you take care, setting a
gateway may work, but it would feel clearer to instead just add an
explicit /0 manual route instead.
Also, note that usually you don't need a gateway anyway. WireGuard is a
Layer 3 (IP) tunnel, where the next hop is alway just the other side of
the tunnel. The next hop has little effect on the routes that you
configure on a WireGuard interface. What however matters is whether a
default route is present or not.
Also, an explicit gateway probably works badly with "ipv[46].ip4-auto-default-route",
because in that case the automatism should add a /0 peer-route route in a
separate routing table. The explicit gateway interferes with that too.
Nonetheless, without this patch it's not obvious why the /0 peer
route gets suppressed when a gateway is set. Don't allow for that, and
always add the peer-route.
Probably the profile's gateway setting is still wrong and causes the
profile not to work. But at least, you see all routes configured, and
it's clearer where the (wrong) default route to the gateway comes from.
(cherry picked from commit 115291a46f)
This will be useful to set future options on the NMIPConfig.
Yes, the code duplication of NMIP[46]Config is horrible. Needs
to be unified in the future.
(cherry picked from commit e8b86f8445)
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.
- 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).
If the user adds an address manually, kernel automatically adds a
prefix route for it unless the address has the NOPREFIXROUTE
flag. When ip_config_merge_and_apply() gets called, NM also adds its
prefix route and so we end up with two routes that differ only for the
metric.
This is a problem because the route added by NM is not removed if the
user removes the previously added address. Also, it seems confusing to
have multiple instances of the same routes.
This commit skips the addition of a prefix route for addresses added
manually outside of NetworkManager.
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.
If the @append_force argument is set and the object is already in the
list, it must be moved at the end.
Fixes: 22edeb5b69 ('core: track addresses for NMIP4Config/NMIP6Config via NMDedupMultiIndex')
Up until now, a default-route (with prefix length zero) could not
be configured directly. The user could only set ipv4.gateway,
ipv4.never-default, ipv4.route-metric and ipv4.route-table to influence
the setting of the default-route (respectively for IPv6).
That is a problematic limitation. For one, whether a route has prefix
length zero or non-zero does not make a fundamental difference. Also,
it makes it impossible to configure all the routing attributes that one can
configure otherwise for static routes. For example, the default-route could
not be configured as "onlink", could not have a special MTU, nor could it be
placed in a dedicated routing table.
Fix that by lifting the restriction. Note that "ipv4.never-default" does
not apply to /0 manual routes. Likewise, the previous manners of
configuring default-routes ("ipv4.gateway") don't conflict with manual
default-routes.
Server-side this all the pieces are already in place to accept a default-route
as static routes. This was done by earlier commits like 5c299454b4
('core: rework tracking of gateway/default-route in ip-config').
A long time ago, NMIPRoute would assert that the prefix length is
positive. That was relaxed by commit a2e93f2de4 ('libnm: allow zero
prefix length for NMIPRoute'), already before 1.0.0. Using libnm from
before 1.0.0 would result in assertion failures.
Note that the default-route-metric-penalty based on connectivity
checking applies to all /0 routes, even these static routes. Be they
added due to DHCP, "ipv4.gateway", "ipv4.routes" or "wireguard.peer-routes".
I wonder whether doing that unconditionally is desirable, and maybe
there should be a way to opt-out/opt-in for the entire profile or even
per-routes.
https://bugzilla.redhat.com/show_bug.cgi?id=1714438
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
Until now, all implemented NMPObject types have an ifindex field (from
links, addresses, routes, qdisc to tfilter).
The NMPObject structure contains a union of all available types, that
makes it easier to down-case from an NMPObject pointer to the actual
content.
The "object" field of NMPObject of type NMPlatformObject is the lowest
common denominator.
We will add NMPlatformRoutingRules (for policy routing rules). That type
won't have an ifindex field.
Hence, drop the "ifindex" field from NMPlatformObject type. But also add
a new type NMPlatformObjWithIfindex, that can represent all types that
have an ifindex.
Add a new argument to nm_ip_config_* helpers to also ignore addresses
similarly to what we already do for routes. This will be used in the
next commit; no change in behavior here.
For static functions inside a module, the compiler determines on its own
whether to inline the function.
Also, "inline" was used at some places that don't immediatly look like
candidates for inlining. It was most likely a copy&paste error.
There is really no problem here, allow it.
Previously we would assert against a non-zero prefix length.
But I am not sure that all callers really ensured that this
couldn't happen. Anyway, there is no problem we such addresses,
really.
Only we need to make sure that nm_ip4_config_add_dependent_routes()
and nm_ip6_config_add_dependent_routes() don't add prefix routes for
such addresses (which is the case now).
While nm_utils_inet*_ntop() accepts a %NULL buffer to fallback
to a static buffer, don't do that.
I find the possibility of using a static buffer here error prone
and something that should be avoided. There is of course the downside,
that in some cases it requires an additional line of code to allocate
the buffer on the stack as auto-variable.
Previously we had nm_ip{4,6}_config_dump() for debugging purposes, but
they were inconveniently printing to stdout and so the output was not
ordered in the journal.
Implement a unified nm_ip_config_dump() that logs through the usual
logging mechanism.
The DNS manager reacts to NM_DEVICE_IP4_CONFIG_CHANGED events, which
are generated when there is a relevant change to an IP4 configuration.
Until now, changes to the mdns,llmnr properties were not
considered relevant (and neither minor, this is already a bug).
Promote them to relevant so that the DNS manager is notified and will
rewrite the DNS configuration when one of this properties changes.
Note that the DNS priority should be considered relevant and added
into the checksum as well, but is a problem right now because in the
DNS manager we rely on the fact that an empty configuration (i.e. just
created) has a zero checksum. This is needed to avoid rewriting
resolv.conf when there is no change. The DNS priority initial value
depends on the connection type (VPN or not), so it's a bit difficult
to add it to checksum maintaining the assumption of checksum(empty)=0.
This should be improved in the future.
