Support the creation of parameterless 'prio' qdiscs. The kernel needs
a TCA_OPTIONS attribute initialized with default values. We currently
don't support modifying the qdisc parameters.
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
nm_platform_ethtool_init_ring() only has one caller. It's simpler to
drop the function and implement it at the only place where it is needed.
Maybe there could be a place for a function to initialize NMEthtoolRingState,
one option after the other. However, at the moment there is only one
user, so don't implement it.
This fixes various minor issues:
- the function had a NMPlatform argument, although the argument
is not used. Thus function merely operates on a NMEthtoolRingState
instance and shouldn't have a nm_platform_*() name.
- nm_platform_ethtool_init_ring() returned a boolean, but all
code paths (except assertion failures) returned success.
- as the function returned an error status, the caller was compelled
to handle an error that could never happen.
- the option was specified by name, although we already have a more
efficient way to express the option: the NMEthtoolID. Also, the
caller already needed to resolve the name to the NMEthtoolID, so
there was no need to again lookup the ID by name.
Rework qdisc synchronization. The previous implementation added all
known qdiscs and removed unneeded ones from platform; this had some
problems:
- kernel doesn't allow to add (with exclusive flag) a qdisc if one
with the same parent already exists;
- if we use the replace flag instead of add, then it becomes possible
to add a new qdisc with the same parent of an existing one. However
if the existing qdisc is of the same kind, kernel will try to to
change() it, which fails for some qdiscs (e.g. sfq).
- kernel doesn't allow to delete a qdisc with handle of zero because
that is the default qdisc and can only be replaced;
Fix that.
When searching an element that is lower than the first list element (for
example RTNL type "batadv"), imax will be -1 after the last iteration.
Use int instead of unsigned to make the termination condition imin > imax
work in this case. This fixes NetworkManager crashing due to an
out-of-bounds array access whenever interfaces of such types exist.
Fixes: 19ad044359 ('platform: use binary search to lookup NMLinkType for rtnl_type')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/515
We already have NMEthtoolID to handle coalesce options in a way that is
convenient programmatically. That is, we can iterate over all valid
coalesce options (it's just an integer) and use that in a more generic
way.
If NMEthtoolCoalesceState names all fields explicitly, we need explicit
code that names each coalesce option. Especially since NMEthtoolCoalesceState
is an internal and intermediate data structure, this is cumbersome
and unnecessary.
Thereby it also fixes the issue that nm_platform_ethtool_init_coalesce() has a
NMPlatform argument without actually needing it.
nm_platform_ethtool_init_coalesce() does not operate on a NMPlatform
instance, and should not have the appearance of being a method of
NMPlatform.
All other users name a similar variable "coalesce" (or "coalesce_new",
"coalesce_old"). Rename the variables, to don't use different names for
similar uses.
Only in one moment we need the old and requested settings together:
during _ethtool_coalesce_set(). But for that we shouldn't track both
states in "NMEthtoolCoalesceState".
Simplify "NMEthtoolCoalesceState" to only contain one set of options.
By tracking less state, the code becomes simpler, because you don't
need to wonder where the old and requested state is used.
Add nm_utils_invoke_on_timeout() beside nm_utils_invoke_on_idle().
They are fundamentally similar, except one schedules an idle handler
and the other a timeout.
Also, use the current g_main_context_get_thread_default() as context
instead of the singleton instance. That is a change in behavior, but
the only caller of nm_utils_invoke_on_idle() is the daemon, which
doesn't use different main contexts. Anyway, to avoid anybody being
tripped up by this also change the order of arguments. It anyway
seems nicer to first pass the cancellable, and the callback and user
data as last arguments. It's more in line with glib's asynchronous
methods.
Also, in the unlikely case that the cancellable is already cancelled
from the start, always schedule an idle action to complete fast.
When we must synchronize IPv6 addresses, we compare the order of
addresses to set with what is currently set on platform. Starting from
addresses with lower priority, when a mismatch is found we remove it
from platform and also remove all following addresses, so that we can
re-add them in the right order.
Since kernel keeps addresses internally sorted by scope, we should
consider each scope separately in order to avoid unnecessary address
deletions. For example, if we want to configure addresses
fe80::1/64,2000::1/64 and we currently have on platform 2000::1/64,
it's not necessary to remove the existing address; we can just add the
link-local one.
Co-authored-by: Thomas Haller <thaller@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1814557
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
Surisingly, the compiler may detect the remaining obj_type in
the default switch. Then, inlining nmp_class_from_type() it may detect
that this is only possible to hit with an out or range access to
_nmp_classes array.
Rework the code to avoid that compiler warning. It's either way not
supposed to happen.
Also, drop the default switch case and explicitly list the enum values.
Otherwise it is error prone to forget a switch case.
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)
