Nothing changes practically, as the NMDevice still starts this with
AF_UNSPEC. That is going to change in the following commit.
The ugly part:
priv->concheck_x[0] in few places. I believe we shouldn't be using union
aliasing here, and instead of indexing the v4/v6 arrays by a boolean it
should be an enum. I'm not fixing it here, but I eventually plan to if
this gets an ACK.
This allows us to use the correct DNS server for the particular interface
independent of what the system resolver is configured to use.
The ugly part:
Unfortunately, it is not all that easy. The libc's libresolv.so API does
not provide means for influencing neither interface nor name servers used
for DNS resolving.
Curl can also be compiled with c-ares resolver backend that does provide
the necessary functionality, but it requires and extra library and the
Linux distributions don't seem to enable it. (Fedora doesn't, which is a
good sign we don't have an option of relying on it.)
systemd-resolved does provide everything we need. If we take care to
keep its congfiguration up to date, we can use it to do the resolving on
a particular interface with that interface's DNS configuration. Great!
There's one more problem: Curl doesn't provide callbacks for resolving
host names. It doesn't, however, allow us to pass in the pre-resolved
hostnames in form of an CURLOPT_RESOLVE(3) option. This means we have to
parse the host name out of the URL ourselves. Fair enough I guess...
Before:
"manager: check_if_startup_complete returns FALSE because of eth0"
Now:
"manager: startup complete is waiting for device 'eth0' (autoactivate)"
Also, the logging line is now more a human readable sentence, but still
follows the same pattern as later
"manager: startup complete"
Meaning: grepping for "startup complete" becomes more helpful because
one first finds the reasons why startup-complete is not yet reached,
followed by the moment when it is reached.
src/devices/nm-acd-manager.c:419:31: error: variable 'info' is uninitialized
when used here [-Werror,-Wuninitialized]
nm_utils_inet4_ntop (info->address, NULL),
Also make sure the secrets request callback only send a reply to IWD and
the Connect method return callback executes the device state change to
"disconnected".
state_changed (called when IWD signalled device state change) was
supposed to not change NM device state on connect success or failure and
instead wait for the DBus Connect() method callback but it would
actually still call nm_device_emit_recheck_auto_activate on failure so
refactor state_changed and network_connect_cb to make sure
the state change and nm_device_emit_recheck_auto_activate are only
called from network_connect_cb.
This fixes a race where during a switch from one network to another NM
would immediately start a new activation after state_changed and
network_connect_cb would then handle the Connect failure and mark the
new activation as failed.
When creating the mirror 802.1x connections for IWD 802.1x profiles
set the NM_SETTING_SECRET_FLAG_NOT_SAVED flag on the secrets that
may at some point be requested from our agent. The saved secrets could
not be used anyway because of our use of
NM_SECRET_AGENT_GET_SECRETS_FLAG_REQUEST_NEW in
nm_device_iwd_agent_query. But also try to respect whatever secret
caching policy has been configured in the IWD profile for those secrets,
IWD would be responsible for storing them if it was allowed in the
profile.
In two places stop using g_dbus_proxy_new_* to create whole new
interface proxy objects for net.connman.iwd.Network as this will
normally have a huge overhead compared to asking the ObjectManager
client that we already have in NMIwdManager for those proxies.
dbus-monitor shows that for each network path returned by
GetOrderedNetworks () -- and we call it every 10 or 20 seconds and may
get many dozens of networks back -- gdbus would do the following each
time:
org.freedesktop.DBus.AddMatch("member=PropertiesChanged")
org.freedesktop.DBus.StartServiceByName("net.connman.iwd")
org.freedesktop.DBus.GetNameOwner("net.connman.iwd")
org.freedesktop.DBus.Properties.GetAll("net.connman.iwd.Network")
org.freedesktop.DBus.RemoveMatch("member=PropertiesChanged")
Using these unormalized was wrong all along, but by chance didn't hit
paths that needed normalized connections. This may change if we
actually write in memory connections to /run with the keyfile plugin,
because that one wants them normalized.
This also saves some work, because normalization does boring things for
us, such as adding default ipv4/ipv6/proxy settings everywhere.
On networked boot we need to somehow communicate this to the early boot
machinery. Sadly, no DBus there and we're running in configure-and-quit
mode.
Abusing the state file for this sounds almost reasonable and is
reasonably straightforward thing to do.
libcurl does not allow removing easy-handles from within a curl
callback.
That was already partly avoided for one handle alone. That is, when
a handle completed inside a libcurl callback, it would only invoke the
callback, but not yet delete it. However, that is not enough, because
from within a callback another handle can be cancelled, leading to
the removal of (the other) handle and a crash:
==24572== at 0x40319AB: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24572== by 0x52DDAE5: Curl_close (url.c:392)
==24572== by 0x52EC02C: curl_easy_cleanup (easy.c:825)
==24572== by 0x5FDCD2: cb_data_free (nm-connectivity.c:215)
==24572== by 0x5FF6DE: nm_connectivity_check_cancel (nm-connectivity.c:585)
==24572== by 0x55F7F9: concheck_handle_complete (nm-device.c:2601)
==24572== by 0x574C12: concheck_cb (nm-device.c:2725)
==24572== by 0x5FD887: cb_data_invoke_callback (nm-connectivity.c:167)
==24572== by 0x5FD959: easy_header_cb (nm-connectivity.c:435)
==24572== by 0x52D73CB: chop_write (sendf.c:612)
==24572== by 0x52D73CB: Curl_client_write (sendf.c:668)
==24572== by 0x52D54ED: Curl_http_readwrite_headers (http.c:3904)
==24572== by 0x52E9EA7: readwrite_data (transfer.c:548)
==24572== by 0x52E9EA7: Curl_readwrite (transfer.c:1161)
==24572== by 0x52F4193: multi_runsingle (multi.c:1915)
==24572== by 0x52F5531: multi_socket (multi.c:2607)
==24572== by 0x52F5804: curl_multi_socket_action (multi.c:2771)
Fix that, by never invoking any callbacks when we are inside a libcurl
callback. Instead, the handle is marked for completion and queued. Later,
we complete all queue handles separately.
While at it, drop the @error argument from NMConnectivityCheckCallback.
It was only used to signal cancellation. Let's instead signal that via
status NM_CONNECTIVITY_CANCELLED.
https://bugzilla.gnome.org/show_bug.cgi?id=797136https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1792745https://bugzilla.opensuse.org/show_bug.cgi?id=1107197https://github.com/NetworkManager/NetworkManager/pull/207
Fixes: d8a31794c8
As we accept addr_family %AF_UNSPEC to detect the address family,
we also need to return it. Just returning the binary address without
the address family makes no sense.
Note that NMSettingEthtool and NMSettingMatch don't have such
functions either.
We have API
nm_connection_get_setting (NMConnection *, GType)
nm_connection_get_setting_by_name (NMConnection *, const char *)
which can be used generically, meaning: the requested setting type
is an argument to the function. That is generally more useful and
flexible.
Don't add API which duplicates existing functionality and is (arguably)
inferiour. Drop it now. This is an ABI/API break for the current development
cycle where the 1.14.0 API is still unstable. Indeed it's already after
1.14-rc1, which is ugly. But it's also unlikely that somebody already uses
this API/ABI and is badly impacted by this change.
Note that nm_connection_get_setting() and nm_connection_get_setting_by_name()
are slightly inconvenient in C still, because they usually require a cast.
We should fix that by changing the return type to "void *". Such
a change may be possibly any time without breaking API/ABI (almost, it'd
be an API change when taking a function pointer without casting).
(cherry picked from commit a10156f516)
The Station.GetOrderedNetworks dbus method's return type has changed in
IWD commit 0a42f63d42be903a46c595693884772c1c84d39f as the last incompatible
API change before IWD 0.8 (docs change was made earlier in
0453308134a3aadb6a2ec6a78ea642e19427704c) so that network names and
types are no longer included in the reply. Expect this new reply
signature although still handle the old signature if we're using the
Device interface for IWD <= 0.7 compatibility.
It may be good idea to eventually pass the object manager instance from
nm-iwd-manager.c to nm-device-iwd.c to avoid using g_dbus_proxy_new_sync
and g_dbus_proxy_new_for_bus_sync in act_stage2_config, which possibly
generates a lot of DBus property queries.
https://github.com/NetworkManager/NetworkManager/pull/197
(cherry picked from commit 32506c8788)
Later we want to fully support wireguard devices. Also,
possibly activating a generic profile in a wireguard device
would make sense.
Anyway, for the moment, just prevent that from happening
by explicitly marking the device as unmanaged.
(cherry picked from commit e3bd482329)
Currently, NMDeviceWireguard does neither set connection_type_check_compatible
nor implement check_connection_compatible. That means, it appears to be compatible
with every connection profile, which is obviously wrong.
Allow devices not to implement check_connection_compatible() and avoid the issue
by rejecting profiles by default.
(cherry picked from commit baa0008313)
NM_IN_SET will only compare string pointers and isn't useful for
checking if nm_setting_wireless_get_mode (s_wifi) is infrastructure.
Fixes: 570e1fa75b
Make sure we free our IWD agent objects whenever we're freeing the
IWD Object Manager. We're registering those objects on the same DBus
connection as the Object Manager so that they're visible to IWD, and
our only reference to that connection is through priv->object_manager
so even though the connection isn't changing when we free the object
manager and create a new one, we still need to free the agent object.
We could maybe keep a reference to the connection, but I'm not sure
there's any warranty that it doesn't get closed. We could also use
nm_dbus_manager_get_connection (nm_dbus_manager_get ()) and only
register and free the agent once, since it happens to be the same
connection but it'd perhaps be a hack to rely on this.
- since commit d17d26887c, a
NMSettingsConnection no longer "is-a" NMConnection. Instead,
we must call nm_settings_connection_get_connection() to obtain
the NMConnection instance. Adjust this in mirror_8021x_connection()
- don't leak "ssid" in mirror_8021x_connection()
- move deletion of the mirror-connection to known_network_data_free().
Previously, we must have made sure that every g_hash_table_remove()
and g_hash_table_insert()(!!) first deletes the mirror connection.
Likewise, in got_object_manager() when we call g_hash_table_remove_all(),
delete created mirror connections.
- rework interface_added() to make it robust against calling
interface_added() more than once without removing the interface
in between. Essentially, this just means that we first look into
"priv->known_networks" to see whether the @id is already tracked.
And if so, delete an existing mirror-connection as necessary.
Calling g_hash_table_insert() with a key which is already hashed
will destroy the *new* key. Since @id is used below, that would
be use after free.
Fixes: d635caf940551f8f5b52683b8379a1f81c58f8fc
IWD's mechanism for connecting to EAP networks requires a network config
file to be present in IWD's storage. NM and its clients however won't
allow a connection to be attempted until a valid NMConnection is created
on the NM side for the network. To avoid duplicating the settings from
the IWD-side profiles in NM, automatically create NMSettingConnections
for EAP networks preconfigured on the IWD side, unless a matching
connection already exists. These connections will use the "external"
EAP method to mean their EAP settings can't be modified through NM, also
they won't be valid for devices configured to use the wpa_supplicant
backend unfortunately.
Those nm-generated connections can be modified by NM users (makes sense
for settings not related to the wifi authentication) in which case they
get saved as normal profiles and will not be recreated as nm-generated
connections on the next run.
I want to additionally handle deleting connections from NM clients so
that they're also forgotten by IWD, in a later patch.
Make this function public. I'm not sure if at this point it makes
much sense to add a new file for iwd-specific utilities.
While there add a way for the function to return error if security
type can't be mapped to an IWD-supported security type.
The known networks hash table is indexed by the (ssid, security) tuple
for fast lookups both on DBus signals related to an IWD known network
and local NMConnection signals such as on removal.
There's no need anymore for NMIwdManager to know when a network has been
connected to, InterfaceAdded signals are now emitted when a network is
saved as a Known Network.
Before 0.5 IWD has changed the known networks API to expose separate
objects for each known network and dropped the KnownNetworks
manager-like interface so stop using that interface. Following
patches will add tracking of the known networks through
ObjectManager.
Instead of passing the return values to g_variant_get_string or
g_variant_boolean and then checking the return value of that call,
add wrappers that first check's whether the variant is non-NULL
and of the right type.
g_variant_get_string doesn't allow a NULL parameter and will also never
return NULL according to the docs.
For the State property we assume a state "unknown" and emit a warning
if the property can't be read, "unknown" is also a string in IWD itself
which would be returned if something went really wrong. In any case
this shouldn't happen.
[thaller@redhat.com: fix missing initialization of nm_auto() variable
interfaces.]
NMConnection is an interface, which is implemented by the types
NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
NMRemoteConnection (libnm).
NMSettingsConnection does a lot of things already:
1) it "is-a" NMDBusObject and exports the API of a connection profile
on D-Bus
2) it interacts with NMSettings and contains functionality
for tracking the profiles.
3) it is the base-class of types like NMSKeyfileConnection and
NMIfcfgConnection. These handle how the profile is persisted
on disk.
4) it implements NMConnection interface, to itself track the
settings of the profile.
3) and 4) would be better implemented via delegation than inheritance.
Address 4) and don't let NMSettingsConnection implemente the NMConnection
interface. Instead, a settings-connection references now a NMSimpleConnection
instance, to which it delegates for keeping the actual profiles.
Advantages:
- by delegating, there is a clearer separation of what
NMSettingsConnection does. For example, in C we often required
casts from NMSettingsConnection to NMConnection. NMConnection
is a very trivial object with very little logic. When we have
a NMConnection instance at hand, it's good to know that it is
*only* that simple instead of also being an entire
NMSettingsConnection instance.
The main purpose of this patch is to simplify the code by separating
the NMConnection from the NMSettingsConnection. We should generally
be aware whether we handle a NMSettingsConnection or a trivial
NMConnection instance. Now, because NMSettingsConnection no longer
"is-a" NMConnection, this distinction is apparent.
- NMConnection is implemented as an interface and we create
NMSimpleConnection instances whenever we need a real instance.
In GLib, interfaces have a performance overhead, that we needlessly
pay all the time. With this change, we no longer require
NMConnection to be an interface. Thus, in the future we could compile
a version of libnm-core for the daemon, where NMConnection is not an
interface but a GObject implementation akin to NMSimpleConnection.
- In the previous implementation, we cannot treat NMConnection immutable
and copy-on-write.
For example, when NMDevice needs a snapshot of the activated
profile as applied-connection, all it can do is clone the entire
NMSettingsConnection as a NMSimpleConnection.
Likewise, when we get a NMConnection instance and want to keep
a reference to it, we cannot do that, because we never know
who also references and modifies the instance.
By separating NMSettingsConnection we could in the future have
NMConnection immutable and copy-on-write, to avoid all unnecessary
clones.
The limit of trying up to 10000 was arbitrary. In practice, we are not expected
that we need that many searches. If that would be the case (and we would have
10000 conflicting connections that take all the names), then we anyway would
need to refactor the code not to scale with O(n^2).
Replace the arbitrary limit with an even larger one. The new limit is so
large that in practice it's impossible to reach it.
