Having a NMCmdLine implementation here is wrong.
For one, it local to nm-dnsmasq-manager.c and not reusable.
If there is anything of value in such an implementation, then it should
possibly also be useful at other places that create command line
arguments.
Note that in the end, command line arguments are just strv arrays.
There are different ways how to construct that strv array. For example,
do we need to clone the strings that we add? How to do that most
elegantly and efficiently? The previous implementation for example used a
GStringChunk for that (quite creative!). The point is, there are pros and
cons about how to create strv arrays. But constructing command line options
shouldn't be abstracted in a NMCmdLine API. It should use a suitable API
for creating an strv array. Otherwise, it's too much abstraction.
Drop NMCmdLine and use GPtrArray directly. Together with a few helper
functions nm_strv_ptrarray_*() that is our preferred way to create such
strv arrays. Is it perfect? No, we still g_strdup() static strings.
That could be optimized. But then we would want an optimized API for
constructing strv arrays, not NMCmdLine.
Checking whether the link exists in the cache, before talking to kernel
serves no purpose.
- in all cases, the caller already has a good indication that the link
in fact exists. That is, because the caller makes decisions on what to
do, based on what platform told it earlier. Thus, the check usually succeeds
anyway.
- in the unexpected case it doesn't succeed, we
- should not silently return without logging at least a message
- we possibly still want to send the netlink message to kernel,
just to have it fail. Note that the ifindex is indeed the identifier
for the link, so there is no danger of accidentally killing the
wrong link.
Well, theoretically there is, because the kernel's ifindex counter can
wrap or get reused when moving links between namespaces. But checking
the cache would not protect against that anyway! Worst case, the cache
would already have the impostor link and would not prevent from doing
the wrong thing. After all, they do have the same identifier, so how
would we know that this is in fact a different link?
We want that all code paths assert strictly and gracefully.
That means, if we have function nm_platform_link_get() which calls
nm_platform_link_get_obj(), then we don't need to assert the same things
twice. Don't have the calling function assert itself, if it is obvious
that the first thing that it does, is calling a function that itself
asserts the same conditions.
On the other hand, it simply indicates a bug passing a non-positive
ifindex to any of these platform functions. No longer let
nm_platform_link_get_obj() handle negative ifindex gracefully. Instead,
let it directly pass it to nmp_cache_lookup_link(), which eventually
does a g_return_val_if_fail() check. This quite possible enables
assertions on a lot of code paths. But note that g_return_val_if_fail()
is graceful and does not lead to a crash (unless G_DEBUG=fatal-criticals
is set for debugging).
nm_platform_link_delete() will soon assert against positive ifindex
argument.
nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME));
will result in an assertion, if the link does not exist.
Extend nmtstp_link_delete() to gracefully skip deleting the link
so that it can be used in such situations.
Also, rename nmtstp_link_del() to nmtstp_link_delete(), because it's
closer to nm_platform_link_delete().
We want to assert for valid input arguments, but we don't want
multiple assertions for the same.
Move the assertion from nm_platform_link_get() to
nm_platform_link_get_obj().
That way, nm_platform_link_get_obj() also checks the input arguments.
At the same time, nm_platform_link_get() gets simpler and still does
the same amount of assertions.
In nmp_cache_lookup_link_full(), we may have multiple candidates that match.
Continue searching, until we find a visible one. That way, visible results
are preferred.
Note that for links, nmp_object_is_visible() checks whether the link is
visible in netlink (instead of only udev).
This code will be used later.
We want to remember which keyfiles are currently loaded (or hidden).
With the addition or multiple keyfile directories (soon), there are
two cases where this matters:
- if there are multiple keyfiles which reference the same UUID,
we can only load one of them. That is already a problem today
with only one keyfile directory, where multiple files can reference
the same UUID.
The implementation will pick the file based on priorities (like
the file modification date). However, the user may call explicitly
call `nmcli connection load`. In that case, we cannot reload
all files to find out whether the to be loaded file is hidden
according to the defined priorities. We cannot do that, because we
must not make decisions based on files on disk, which we are not told
to reload. So, during a `nmcli connection load` we must look at
unrelated files, to determine how to load the file.
Instead, we do allow the user to load any file, even if it would be
shadowed by other files. When we do that, we may want to persist which
file is currently loaded, so that a service restart and a `nmcli connection
reload` does not undo the load again. This can be later later be solved by
writing a symlink
"/var/run/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile"
which targets the currently active file.
- if a profile was loaded from read-only persistant storage, the user
may still delete the profile. We also need to remember the deletion
of the file. That will be achieved by symlinking "/dev/null" as
"/etc/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile".
Add helper functions to read and write these symlinks.
Add a new CON_DEFAULT() macro that places a property name into a
special section used at runtime to check whether it is a supported
connection default.
Unfortunately, this mechanism doesn't work for plugins so we have to
enumerate the connection defaults from plugins in the daemon using
another CON_DEFAULT_NOP() macro.
Correct the spelling across the *entire* tree, including translations,
comments, etc. It's easier that way.
Even the places where it's not exposed to the user, such as tests, so
that we learn how is it spelled correctly.
There's no reason the mesh shouldn't autoconnect. Almost.
The mesh and regular Wi-Fi shares the same radio. There, in the first
place, probably shouldn't have been separate NMDevices. Not sure whether
we can fix it at this point, but we can surely avoid unnecessary
competition between the two devices: give the regular Wi-Fi priority and
only connect mesh if the regular companion stays disconnected.
For the record; connections shipped on XO-1 laptops all have
autoconnect=off and thus are not affected by this.
If the client wants to pinpoint the connection to a particular device
they can just add an appropriate property.
That said, MAC address probably even doesn't count as appropriate; an
interface name is supposed to stay stable and could be used in such
cases.
This fixes the case where "nmcli d wifi connect ..." ends up with a
connection tied to a rather random device that happened to be around
even without the "ifname" argument.
A lot of drivers actually support changing the MAC address of a link
without taking it down.
Taking down a link is very bad, because kernel will remove routes
and IPv6 addresses.
For example, if the user used a dispatcher script to add routes,
these will be lost. Note that we may change the MAC address of a
device any time. For example, a VLAN device watches the parent's
MAC address and configures it (with a logging message "parent hardware
address changed to ...").
Try first whether we can change the MAC address without taking the
link down. Only if that fails, retry with taking it down first.
https://bugzilla.redhat.com/show_bug.cgi?id=1639274
Add helper nm_platform_link_get_ifi_flags() to access the
ifi-flags.
This replaces the internal API _link_get_flags() and makes it public.
However, the return value also allows to distinguish between errors
and valid flags.
Also, consider non-visible links. These are links that are in netlink,
but not visible in udev. The ifi-flags are inherrently netlink specific,
so it seems wrong to pretend that the link doesn't exist.
Working on NetworkManager 1.12.4 and sometimes (rarely), when creating
a NM client object before NetworkManager service start, this object will
never be running.
In that case, we can see the following log:
"[GLIB-GLib-GIO WARN] Error calling GetManagedObjects() when name
owner :1.5 for name org.freedesktop.NetworkManager came back:
GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod:
No such interface 'org.freedesktop.DBus.ObjectManager' on object
at path /org/freedesktop".
Object Manager object shall be registered before requesting dbus name
to be sure that 'org.freedesktop.Dbus.ObjectManager' interface is present
when name owner change is received by libnm.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/51
dhcp_identifier_set_iaid() is no longer called. Replace
the code with an assertion and always fail.
The function was already annoying previously, because it would
require udev API to generate the IAID. So, we were already required
to patch this function.
During start, sd_dhcp6_client would call client_ensure_iaid(),
to initialize the IAID.
First of all, the IAID is important to us, we should not leave the
used IAID up to an implementation detail. In the future, we possibly
want to make this configurable.
The problem is that client_ensure_iaid() calls dhcp_identifier_set_iaid()
which looks up the interface name via if_indextoname() (in our fork).
The problem is that dhcp_identifier_set_iaid() looks up information by
quering the system, which makes it hard for testing and also makes it
unpredictable. It's better to use our implementation nm_utils_create_dhcp_iaid(),
which is solely based on the interface-name, which is given as a well
defined parameter to the DHCP client instance.
config_parse_iaid(), dhcp_identifier_set_iaid() and sd_dhcp6_client_set_iaid() all
allow for the IAID to be zero. Also, RFC 3315 makes no mention that zero
would be invalid.
However, client_ensure_iaid() would take an IAID of zero as a sign that
the values was unset. Fix that by keeping track whether IAID is
initialized.
https://github.com/systemd/systemd/pull/108950e408b82b8
nm_utils_get_monotonic_timestamp*() inherrently use static data. Let's
initialize it in a thread safe manner.
nm_utils_get_monotonic_timestamp*() are a fundamental utility function
that should work correctly in all cases. Such a low level function should
be thread safe.
Sometimes the test fail:
$ make -j 10 src/platform/tests/test-address-linux
$ while true; do
NMTST_DEBUG=d ./tools/run-nm-test.sh src/platform/tests/test-address-linux 2>&1 > log.txt || break;
done
fails with:
ERROR: src/platform/tests/test-address-linux - Bail out! test:ERROR:src/platform/tests/test-common.c:790:nmtstp_ip_address_assert_lifetime: assertion failed (adr <= lft): (1001 <= 1000)
That is, because of a wrong check. Fix it.
g_object_set_property() cannot fail nor signal an error reason when
invalid arguments are passed. Use our wrapper nm_g_object_set_property()
instead.
Note that the input argument comes from untrusted (although authenticated)
source.
Upside:
- it may avoid a D-Bus call altogether.
- currently there is no API to bind/unbind an existing NMActiveConnection,
it can only be bound initially with AddAndActivate2.
That makes sense when the calling applicatiion only wants to keep the
profile active for as long as it lives. It never intends to extend the
lifetime beyond that. In such a case, it is expected that eventually
we need to be sure whether the D-Bus client is still alive, as we
make a decision based on that. In that case, we eventually will call
GetNameOwner and nothing is saved.
A future feature could add D-Bus API to bind/unbind existing active
connections after creation. That would allow an application to
activate profiles and -- if anything goes wrong -- to be sure
that the profile gets deactivted. Only in the success case, it
would unbind the lifetime in a last step and terminate. Un such
a case, binding to a D-Bus client is more of a fail-safe mechanism
and commonly not expected to come into action.
This is an interesting feature, because ActivateConnection D-Bus call
may take a long time, while perfroming interactive polkit authentication.
That means, `nmcli connection up $PROFILE` can fail with timeout before
the active connection path is even created, but afterwards the profile may
still succeed to activate. That seems wrong. nmcli could avoid that,
by binding the active connection to itself, and when nmcli exits
with failure, it would make sure that the active connection gets
disconnected as well.
Downside:
- the code is slightly more complicated(?).
- if the D-Bus name is indeed gone (but the NMKeepAlive is still alive
for other reasons), we will not unregister the D-Bus signal, because we
never learn that it's gone. It's not a leak, but an unnecessary
signal subscription.
- during nm_keep_alive_set_dbus_client_watch() we don't notice right
away that the D-Bus client is already gone. That is unavoidable as
we confirm the state asynchronously.
If the NMKeepAlive is kept alive for other reasons but only later
depends on presence of the D-Bus client, then in the non-lazy approach
we would have noticed already that the client is gone, and would disconnect
right away. With the lazy approach, this takes another async D-Bus call to
notice.
When we subscribe the signal fo NameOwnerChanged, we are not sure
that the name-owner is currently alive.
Issue a GetNameOwner call to make sure.
Co-authored-by: Thomas Haller <thaller@redhat.com>
Fixes: 37e8c53eee
"bind" specifically binds the lifetime of the activation (NMActiveConnection).
In combination with "persist=volatile", the lifetime of the NMSettingsConnection
is indirectly bound to the NMActiveConnection. But still these concepts make sense
independently.
In the future, it may make sense to also bind the lifetime of the NMSettingsConnection
to the D-Bus client. Hence, rename the option to allow for the distinction.
Also, belatedly fix libnm comment about "bind" only working with
"persist" "volatile".
Fixes: eb883e34a5
- use nm_streq() instead of g_strcmp0(). I think streq() is easier
to understand.
- the strings that are checked here must never be %NULL, because they come
from string variants. Use nm_streq() instead of nm_streq0() or g_strcmp0().
- don't add a "." to the GError messages. GError messages are commonly
embedded in a larger message, and shoult not themself contain the dot.
Previously, @bind_lifetime was a string. While parsing the @options, we
would set the string to the content of the parsed GVariant. Note that
the GVariant is unrefed before we access @bind_lifetime, thus it's
not guaranteed that it will still exist.
Arguably, the string GVariant's lifetime is tied to the @options
dictionary, so indeed it lives long enough. But that is not-obviously
the case.
Fix that by using a boolean instead. Also, rename @bind_lifetime to
@bind_dbus_client.
For one, there was a bug here: we cannot "goto error" without setting
the @error variable.
Anyway, restricting "bind" "dbus-client" only to profiles that are
"persist" mode "volatile" seems wrong. The "bind" option as it is,
limits the lifetime of the active-connection. This has no direct relation
with the lifetime of the setting-connection. Indeed, if the
settings-connection's lifetime is itself set to "volatile", then
it will indeed go away with the active-connection. However, these
two concepts are not strictly related.
In the future, we might add an option to limite the lifetime of
a settings-connection to a D-Bus client ("bind-setting"). Possibly
we should thus rename "bind" to "bind-activation", to make the
distinction clearer.
Most (not all) functions that can fail and report the reason with
an GError are required to set the error if they fail. It's a bug
to claim to fail without returning the GError reason.
Hence, our callers usually don't check whether a GError is present but
just access it.
Likewise, for better or worse, our GError codes are often not meaningful
(unless explicitly documented). Meaning, logging the error code number
is not helpful. Instead, error messages should be written in a manner
that one can find the source code location where it happened.
Also, return-early to reduce the indentation level of the code.
Also, drop the code comment. It seems to just describe what is obviously
visible by reading the source. It doesn't explain much beside that the
"doesn't have a reason", but not really why.
- always issue a _notify_alive(), just to be sure. At least
in case where we clear a dbus-client watch, the alive state
could change.
- avoid the logging in cleanup_dbus_watch(), if there is nothing
to cleanup.
- in nm_keep_alive_set_settings_connection_watch_visible(), abort
early when setting the same connection again (or %NULL again).
- nm_keep_alive_set_settings_connection_watch_visible() would (already
before) correctly disconnect the signal handler from the previous
connection.
As we anyway do explict disconnects, avoid the overhead of a weak
pointer with g_signal_connect_object(). Just reuse the same
setter to disconnect the signal in dispose().
- fix leaking priv->connection in nm_keep_alive_set_settings_connection_watch_visible().
- use g_signal_handlers_disconnect_by_func(), to be more specific about
which signal we want to disconnect.
The alive state is composed from various parts, let's only emit
a _notify (self, PROP_ALIVE) when it actually changes.
For that, cache the alive state and let _notify_alive() determine
whether it changed and possibly emit the signal.
Some trivial changes:
- move nm_keep_alive_new() after nm_keep_alive_init(), so that the
functions that initialize the instance are beside each other.
- prefer nm_streq*() over strcmp().
- wrap some lines.
- remove some empty lines.
This allows binding the lifetime of the created connection to the
existance of the requesting dbus client. This feature is useful if one
has a service specific connection (e.g. P2P wireless) which will not be
useful without the specific service.
This is simply a mechanism to ensure proper connection cleanup if the
requesting service has a failure.
