If we change the the MTU of an ovs interface only through netlink, the
change could be overridden by ovs-vswitchd at any time when other
interfaces change. Set the MTU also in the ovsdb to prevent such
changes.
Note that if the MTU comes from the connection, we already set the
ovsdb MTU at creation time and so this other update becomes
useless. But it is needed when changing the MTU at runtime (reapply)
or when the MTU comes from a different source (e.g. DHCP).
The ovs-vswitchd.conf.db(5) man page says about the the mtu_request
column in the Interface table:
"Requested MTU (Maximum Transmission Unit) for the interface. A
client can fill this column to change the MTU of an
interface [...] If this is not set and if the interface has
internal type, Open vSwitch will change the MTU to match the
minimum of the other interfaces in the bridge."
Therefore, if the connection specifies a MTU, set it early when adding
the interface to the ovsdb so that it will not be changed to the
minimum of other interfaces.
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.
nm_str_hash() uses siphash24. We want to use that everywhere.
The only concern would be that it is slower. But if that's the concern,
then we would have many more performance critical places to care
about first.
Our glib based code should also include our static utility library
libnm-glib-aux. This is our basic utility library that we want to
have around everywhere. Since we link statically, the linker will weed
out the unused stuff at compile time. So, there is no overhead, except
for the things that we actually use.
It's unnecessary and makes the function unnecessarily not thread safe.
Of course, also ndp_msg_opt_route_prefix() uses static variables, so
it's still not thread safe.
Fixes: c3a4656a68 ('rdisc: libndp implementation')
We should handle kernel command line like systemd does, with its
ConditionKernelCommandLine= setting.
For example, it tokenizes words between various white space characters,
not only space. Use nm_utils_strsplit_set_full() for that.
Note that we currently don't yet have a tokenizer that supports
quotation, like systemd does. We should extend
nm_utils_strsplit_set_full() for that.
NM_ASCII_SPACES contains the ASCII characters according to
g_ascii_isspace().
Add NM_ASCII_WHITESPACES which differs from NM_ASCII_SPACES by not
including "\f". In some cases, that character shall be excluded.
For example, this is what systemd uses as "WHITESPACE" define at
various places.
Also, reorder the spaces string so that plain space comes first. It is
expected that ' ' is much more frequently than newlines or tabs. While
the order here shouldn't matter, it seems preferably to order frequent
characters in front.
Otherwise, we just see "returncode: -11", which isn't very clear.
By default, our test nmcli invocations should never exit by a signal.
Usually, when we encounter a signal, it indicates to a bug and a crash
during the test.
Print more information about the exit code.
returncode: -11 (SIGNAL SIGSEGV)
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
Showcase nm_client_dbus_set_property().
Thereby, also print error messages and return an error if
the command fails.
Also, enable PolicyKit authentication (although, I think there are
some bugs with this still).
Previously, we would call the synchronous nm_client_networking_set_enabled()
method. There were 3 problems:
1) nmcli ignored the return value, that means, if the request failed with
access denied it would just silently pretend that it succeeded.
2) nmcli first called nmc_start_polkit_agent_start_try(), but when
invoking the synchronous method, the main context is busy and a
polkit request cannot possibly be handled.
3) nm_client_networking_set_enabled() is deprecated.
Fix all of these, by calling the D-Bus method directly.
Policykit authentication requests are only handled partly. There
seems to be an unrelated race/bug. Now it works sometimes.
This is more a showcase for using nm_client_dbus_call(), than a
real use.
In this case, nmcli was mostly fine to just invoke the synchronous API
and not care about the problems that it had.
Still, replace it, and show the suggested alternative.
With 1.22, various synchronous functions for invoking D-Bus methods were
deprecated. The reason was that D-Bus is fundamentally asynchronous, and
providing synchronous API in NMClient is inherently wrong. That is
because NMClient essentially is a cache of the D-Bus API, and invoking
g_dbus_connection_call_sync() messes up the order of events from D-Bus.
In particular, when the synchronous function completes, the content of
the cache does not yet reflect the change.
Since they got deprecated, the question is with what to replace them.
Instead of adding a (e.g.) nm_client_networking_set_enabled_async()
for nm_client_networking_set_enabled(), just expect the user to call
D-Bus directly.
D-Bus itself defines a reasonable API, and with GDBusConnection it
is fine (and convenient) to just call D-Bus operations directly.
Often libraries try to abstract D-Bus by providing convenience
wrappers around D-Bus API. I think that often is wrong and unnecessary.
Note that libnm's NMClient does a lot more than just wrapping simple
D-Bus calls. It provides a complete client-side cache of the D-Bus
interface. As such, what libnm's NMClient does is more than simple
wrappers around D-Bus. NMClient is a reasonable thing to do.
However, it is unnecessary to add API like nm_client_networking_set_enabled_async()
that only calls g_dbus_connection_call(). Don't pretend that we would need such
trivial wrappers in libnm.
Instead, recommend to use g_dbus_connection_call(). Or alternatively,
the convenience wrappers nm_client_dbus_call() and
nm_client_dbus_set_property().
Similar to nm_client_dbus_call(), but useful for setting a D-Bus
property on NetworkManager's D-Bus interface.
Note that we currently have various synchronous API for setting D-Bus
properties (like nm_client_networking_set_enabled()). Synchronous
API does not play well with the content of NMClient's cache, and was
thus deprecated. However, until now no async variant exists.
Instead of adding multiple async operations, I think it should be
sufficient to only add one nm_client_dbus_set_property() property.
It's still reasonably convenient to use for setting a property.
Add an API for calling D-Bus methods arbitrary objects of
NetworkManager's API.
Of course, this is basically just a call to g_dbus_connection_call(),
using the current name owner, nm_client_get_dbus_connection() and
nm_client_get_main_context().
All of this could also be achieved without this new API. However,
nm_client_dbus_call() also gracefully handles if the current name
owner is %NULL.
It's a valid concern whether such API is useful, as the users already
have all pieces to do it themself. I think it is.
Move local variables to inner scope.
Also, drop code comment that doesn't give additional information
beyond what is already plainly visible in source code.
When we receive a "InterfaceRemoved" signal, we will end up calling
set_state_down(). That emits a "state" change signal, which causes
NMDeviceWifi to unref the supplicant interface. This may already
give up the last reference, and we cleanup the supplicant state
(by again calling set_state_down()). When we return, set_state_down()
will crash because it operates on an already destroyed instance.
Avoid that by keeping a reference to the interface during set_state_down().
Fixes: b83f07916a ('supplicant: large rework of wpa_supplicant handling')
https://bugzilla.redhat.com/show_bug.cgi?id=1815058
This solves a bug exposed by the following cmds:
$ nmcli c add type bond ifname bond0 con-name bond0
$ nmcli c modify bond0 +bond.options miimon=100
$ nmcli -f bond.options c show bond0
bond.options: mode=balance-rr
Here we just added the option 'miimon=100', but it doesn't get saved in
because nm_settings_connection_set_connection() which is responsible for
actually updating the connection compares the new connection with old
one and if and only if the 2 are different the update is carried out.
The bug is triggered because when comparing, if default values are taken into
account, then having 'miimon=100' or not having it it's essentially the
same for compare(). While this doesn't cause a bond to have a wrong
setting when activated it's wrong from a user experience point of view
and thus must be fixed.
When this patch is applied, the above
commands will give the following results:
$ nmcli c add type bond ifname bond0 con-name bond0
$ nmcli c modify bond0 +bond.options miimon=100
$ nmcli -f bond.options c show bond0
bond.options: mode=balance-rr,miimon=100
Fix unit tests and also add a new case covering this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=1806549
Since ifcfg-rh doesn't write out to file the 'connection.timestamp' property
let's add it before comparing an updated connection with the plugin's reread
one otherwise the comparison operation would always fail.
The fix is not necessary for the keyfile plugin, because the reader/writer
correctly reads/writes the connection timestamp.
