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.
Add a helper function nm_device_parent_find_for_connection() to
unify implementations of setting the parent in update_connection().
There is some change in behavior, in particular for nm-device-vlan.c,
which no longer compares the link information from platform. But
update_connection() is anyway a questionable concept, only used
for external assumed connection (which itself, is questionable). Meaning,
update_connection() is a hack not science, and it's not at all clear
what the correct behavior is.
Also, note how vlan's implementation differs from all others. Why?
Should we always resort to also check the information from platform?
Either way, one of the two approaches should be used consistently and
nm_device_parent_find_for_connection() opts to not consult platform
cache.
Using '#ifdef' is generally error prone. It's better to always define
a define and check for it explicitly. This way, the compiler can issue
a warning if the define does not exist.
Also, note how meson would always define NM_MORE_LOGGING, possibly to
"0". That means, for meson, we unintentionally always enabled more
logging because the define was always present.
Fix that.
- have two variants of functions to set the SSID of an access point:
one that passes SSID as GBytes, and one that passes it as plain
data with length. Accepting a GBytes allows to share the immutable
GBytes instance.
- both functions now also support clearing the SSID. In
nm_wifi_ap_update_from_properties(), if the GVariant specifies
a "SSID", we always update the access point. We already support
chaging the SSID, so why not support changing it to *no* SSID
(hidden).
GBytes makes more sense, because it's immutable.
Also, since at other places we use GBytes, having
different types is combersome and requires needless
conversions.
Also:
- avoid nm_utils_escape_ssid() instead of _nm_utils_ssid_to_string().
We use nm_utils_escape_ssid() when we want to log the SSID. However, it
does not escape newlines, which is bad.
- also no longer use nm_utils_same_ssid(). Since it no longer
treated trailing NUL special, it is not different from
g_bytes_equal().
- also, don't use nm_utils_ssid_to_utf8() for logging anymore.
For logging, _nm_utils_ssid_escape_utf8safe() is better because
it is loss-less escaping which can be unambigously reverted.
nm_utils_same_ssid() has a comment
* Earlier versions of the Linux kernel added a NULL byte to the end of the
* SSID to enable easy printing of the SSID on the console or in a terminal,
* but this behavior was problematic (SSIDs are simply byte arrays, not strings)
* and thus was changed. This function compensates for that behavior at the
* cost of some compatibility with odd SSIDs that may legitimately have trailing
* NULLs, even though that is functionally pointless.
and the functionality was introduced by commit
ccb13f0bdd.
There was only place left that calls nm_utils_same_ssid().
I really don't think this is the right approach, nor is it clear
that this is still necessary. Also, it seems to only matter with
WEXT, and we should not have such an ugly hack in all cases.
Use GBytes instead of GBytesArray. GBytes is immutable and
can be shared.
It is also the type that we natively get from
nm_setting_wireless_get_ssid(). This way we avoid some
conversions.
