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.
This option allows setting the rules for how long the connection should
be stored. Valid values are "disk" (the default), "memory" and
"volatile". If "memory" or "volatile" is selected, the connection will
not be saved to disk and with "volatile" it will be automatically
removed when it is deactivated again.
This adds a new routine to be able to handle an arbitrary set of further
options for AddAndActivateConnection. Note that no options are accepted
for now.
For P2P connections it makes sense to bind the connection to the status
of the operation that is being done. One example is that a wifi display
(miracast) P2P connection should be shut down when streaming fails for
some reason.
This new helper class allows binding a connection to the presence of a
DBus path meaning that it will be torn down if the process disappears.
nm_device_has_pending_action_reason() marks the device as busy, which in
turn delays "startup-complete" and NetworkManager-wait-online.service.
A device which has no carrier but is otherwise in activated state, is
clearly ready. I didn't test this, but I presume that can easily be the
case with static IP configuration (which can activate without the device
having carrier).
In certain cases the timeouts may not have been unref'ed before they
need to be re-added. Add the appropriate unref calls to ensure we don't
register the timeout multiple times.
This fixes possible cases where timeouts are triggered multiple times
and even on destroyed DHCPv6 clients.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/73
The hostname property is only initialized once, early on during
start. Move the initialization even earlier during object constructions.
This effectively makes the hostname an immutable property.
This also makes sense, because the hostname is used by IPv4 and
IPv6 DHCP instances alike.
Add a new mode for the DHCPv4 client identifier.
"duid" is what the internal (systemd) DHCP client already does by
default. It is also the same as used by systemd-networkd's
"ClientIdentifier=duid" setting. What we still lack (compared to
networkd) are a way to overwrite IAID and the DUID.
Previously, this mode was used by the internal DHCP plugin
by default. However, it could not be explicitly configured.
In general, our default values should also be explicitly selectable.
Now the "duid" client identifier can also be used with the "dhclient"
plugin.
We already had "${DEVICE}" which uses the interface name.
In times of predictable interface naming, that works well.
It allows the user to generate IDs per device which don't
change when the hardware is replaced.
"${MAC}" is similar, except that is uses the permanent MAC
address of the device. The substitution results in the empty
word, if the device has no permanent MAC address (like software
devices).
The per-device substitutions "${DEVICE}" and "${MAC}" are especially
interesting with "connection.multi-connect=multiple".
- use nm_auto to return early when something goes wrong
- don't modify NMDhcpClient's state until the end, when it looks
like we are (almost) started successfully.
- for IPv4, only attempt to load the lease if we actually are
interested in the address. Also, reduce the scope of the lease
variable, to the one place where we need it.
The client-id is something that we want to determine top-down.
Meaning, if the user specifies it via ipv4.dhcp-client-id, then it
should be used. If the user leaves it unspecified, we choose a
default stable client-id. For the internal DHCP plugin, this is
a node specific client-id based on
- the predictable interface name
- and /etc/machine-id
It's not clear, why we should allow specifying the client-id in
the lease file as a third source of configuration. It really pushes
the configuration first down (when we do DHCP without lease file),
to store an additional bit of configuration for future DHCP attempts.
If the machine-id or the interface-name changes, then so does the
default client-id. In this case, also "ipv4.dhcp-client-id=stable"
changes. It's fair to require that the user keeps the machine-id
stable, if the machine identity doesn't change.
Also, the lease files are stored in /var/lib/NetworkManager, which
is more volatile than /etc/machine-id. So, if we think that machine-id
and interface-name is not stable, why would we assume that we have
a suitable lease file?
Also, if you do:
nmcli connection add con-name "$PROFILE" ... ipv4.dhcp-client-id ''
nmcli connection up $PROFILE
nmcli connection modify "$PROFILE" ipv4.dhcp-client-id mac
nmcli connection up $PROFILE
nmcli connection modify "$PROFILE" ipv4.dhcp-client-id ''
nmcli connection up $PROFILE
wouldn't you expect that the original (default) client-id is used again?
Also, this works badly with global connection defaults in
NetworkManager.conf. If you configure a connection default, previously
already this would always force the client-id and overrule the lease.
That is reasonable, but in which case would you ever want to use
the client-id from the lease?
- if we leave the client-id of sd_dhcp_client unset, it will
anyway generate a node-specific client-id (and may fail if
"/etc/machine-id" is invalid).
Anticipate that, and don't let the client-id unset. In case
we have no client-id from configuration or lease, just generate
the id ourself (using the same algorithm). The advantage is,
that we know it upfront and can store the client-id in the
NMDhcpClient instance. We no longer need to peel it out from
the lease later.
- to generate the IPv4 client-id, we need a valid MAC address. Also,
sd_dhcp_client needs a MAC address for dhcp_network_bind_raw_socket()
as well. Just require that a MAC address is always needed. Likewise,
we need a valid ifindex and ifname set.
- likewise for IPv6 and IPv4, cleanup detecting the arptype and
checking MAC address length. sd_dhcp_client_set_mac() is overly
strict at asserting input arguments, so we must validate them anyway.
- also, now that we always initialize the client-id when starting
the DHCP client, there is no need to retroactively extract it
again when we receive the first lease.
A possible issue is that client_start() has about 136 arguments.
It doesn't get simpler by saving lines of code and writing them
all in the same line.
Wrap the lines.
While at it, use "FALSE" for "enforce_duid" argument, instead of "0".
It's a boolean.
