* [PATCH 2/4] auto-t: allow configurable DBus timeout/callbacks on connect{_bssid}
2025-05-28 14:27 [PATCH 1/4] station: treat netconfig failures as connection failures James Prestwood
@ 2025-05-28 14:27 ` James Prestwood
2025-05-28 14:27 ` [PATCH 3/4] auto-t: update serveral tests to work with netconfig refactor James Prestwood
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: James Prestwood @ 2025-05-28 14:27 UTC (permalink / raw)
To: iwd; +Cc: James Prestwood
Let the caller specify the method timeout if there is an expectation
that it could take a long time.
For the conventional connect call (not the "bssid" debug variant) let
them pass their own callback handlers. This is useful if we don't
want to wait for the connect call to finish, but later get some
indication that it did finish either successfully or not.
---
autotests/util/iwd.py | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index 9091807a..37eb4943 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -112,8 +112,8 @@ class AsyncOpAbstract(object):
self._is_completed = True
self._exception = _convert_dbus_ex(ex)
- def _wait_for_async_op(self):
- ctx.non_block_wait(lambda s: s._is_completed, 30, self, exception=None)
+ def _wait_for_async_op(self, timeout=50):
+ ctx.non_block_wait(lambda s: s._is_completed, timeout, self, exception=None)
self._is_completed = False
if self._exception is not None:
@@ -280,8 +280,15 @@ class StationDebug(IWDDBusAbstract):
def autoconnect(self):
return self._properties['AutoConnect']
- def connect_bssid(self, address):
- self._iface.ConnectBssid(dbus.ByteArray.fromhex(address.replace(':', '')))
+ def connect_bssid(self, address, wait=True):
+ self._iface.ConnectBssid(
+ dbus.ByteArray.fromhex(address.replace(':', '')),
+ reply_handler=self._success,
+ error_handler=self._failure
+ )
+
+ if wait:
+ self._wait_for_async_op()
def roam(self, address):
self._iface.Roam(dbus.ByteArray.fromhex(address.replace(':', '')))
@@ -870,8 +877,8 @@ class Device(IWDDBusAbstract):
def stop_adhoc(self):
self._prop_proxy.Set(IWD_DEVICE_INTERFACE, 'Mode', 'station')
- def connect_bssid(self, address):
- self._station_debug.connect_bssid(address)
+ def connect_bssid(self, address, wait=True):
+ self._station_debug.connect_bssid(address, wait=wait)
def roam(self, address):
self._station_debug.roam(address)
@@ -999,7 +1006,7 @@ class Network(IWDDBusAbstract):
def extended_service_set(self):
return self._properties['ExtendedServiceSet']
- def connect(self, wait=True):
+ def connect(self, wait=True, timeout=50, reply_handler=None, error_handler=None):
'''
Connect to the network. Request the device implied by the object
path to connect to specified network.
@@ -1014,12 +1021,19 @@ class Network(IWDDBusAbstract):
@rtype: void
'''
+ if not reply_handler:
+ reply_handler = self._success
+
+ if not error_handler:
+ error_handler = self._failure
+
self._iface.Connect(dbus_interface=self._iface_name,
- reply_handler=self._success,
- error_handler=self._failure)
+ reply_handler=reply_handler,
+ error_handler=error_handler,
+ timeout=timeout)
if wait:
- self._wait_for_async_op()
+ self._wait_for_async_op(timeout=timeout)
def __str__(self, prefix = ''):
return prefix + 'Network:\n' \
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] auto-t: update serveral tests to work with netconfig refactor
2025-05-28 14:27 [PATCH 1/4] station: treat netconfig failures as connection failures James Prestwood
2025-05-28 14:27 ` [PATCH 2/4] auto-t: allow configurable DBus timeout/callbacks on connect{_bssid} James Prestwood
@ 2025-05-28 14:27 ` James Prestwood
2025-05-28 14:40 ` Paul Menzel
2025-05-28 14:27 ` [PATCH 4/4] doc: add note about timeouts to Network.Connect() James Prestwood
2025-05-28 17:47 ` [PATCH 1/4] station: treat netconfig failures as connection failures Denis Kenzior
3 siblings, 1 reply; 7+ messages in thread
From: James Prestwood @ 2025-05-28 14:27 UTC (permalink / raw)
To: iwd; +Cc: James Prestwood
Since the method return to Connect() and ConnectBssid() come after
netconfig some tests needed to be updated since they were waiting
for the method return before continuing. For timeout-based tests
specifically this caused them to fail since before they expected
the return to come before the connection was actually completed.
---
autotests/testNetconfig/static_test.py | 15 +++-----
autotests/testNetconfig/timeout_test.py | 35 +++++++++++++------
.../testNetconfigRoam/netconfig_roam_test.py | 2 +-
3 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/autotests/testNetconfig/static_test.py b/autotests/testNetconfig/static_test.py
index 94307a8c..61037e0e 100644
--- a/autotests/testNetconfig/static_test.py
+++ b/autotests/testNetconfig/static_test.py
@@ -91,16 +91,11 @@ class Test(unittest.TestCase):
# using the same static config. The new client's ACD client should
# detect an IP conflict and not allow the device to reach the
# "connected" state although the DBus .Connect call will succeed.
- ordered_network.network_object.connect()
- self.assertEqual(dev2.state, iwd.DeviceState.connecting)
- try:
- # We should either stay in "connecting" indefinitely or move to
- # "disconnecting"
- condition = 'obj.state != DeviceState.connecting'
- iwd_ns0_1.wait_for_object_condition(dev2, condition, max_wait=21)
- self.assertEqual(dev2.state, iwd.DeviceState.disconnecting)
- except TimeoutError:
- dev2.disconnect()
+ with self.assertRaises(iwd.FailedEx):
+ ordered_network.network_object.connect(timeout=500)
+
+ condition = 'obj.state == DeviceState.disconnected'
+ iwd_ns0_1.wait_for_object_condition(dev2, condition, max_wait=21)
iwd_ns0_1.unregister_psk_agent(psk_agent_ns0_1)
del dev2
diff --git a/autotests/testNetconfig/timeout_test.py b/autotests/testNetconfig/timeout_test.py
index a15706e3..00b03df4 100644
--- a/autotests/testNetconfig/timeout_test.py
+++ b/autotests/testNetconfig/timeout_test.py
@@ -8,6 +8,8 @@ from iwd import PSKAgent
from iwd import NetworkType
class Test(unittest.TestCase):
+ def connect_failure(self, ex):
+ self.failure_triggered = True
def test_netconfig_timeout(self):
IWD.copy_to_storage('autoconnect.psk', name='ap-ns1.psk')
@@ -27,23 +29,34 @@ class Test(unittest.TestCase):
condition = 'not obj.connected'
wd.wait_for_object_condition(ordered_network.network_object, condition)
- ordered_network.network_object.connect()
+ self.failure_triggered = False
- condition = 'obj.state == DeviceState.connecting'
- wd.wait_for_object_condition(device, condition)
+ # Set our error handler here so we can check if it fails
+ ordered_network.network_object.connect(
+ wait=False,
+ timeout=1000,
+ error_handler=self.connect_failure
+ )
+
+ # IWD should attempt to try both BSS's with both failing netconfig.
+ # Then the autoconnect list should be exhausted, and IWD should
+ # transition to a disconnected state, then proceed to full autoconnect.
+ device.wait_for_event("netconfig-failed", timeout=1000)
+ device.wait_for_event("netconfig-failed", timeout=1000)
+ device.wait_for_event("disconnected")
- device.wait_for_event("connecting (netconfig)")
+ device.wait_for_event("autoconnect_full")
- # Netconfig should fail, and IWD should disconnect
- from_condition = 'obj.state == DeviceState.connecting'
- to_condition = 'obj.state == DeviceState.disconnecting'
- wd.wait_for_object_change(device, from_condition, to_condition, max_wait=60)
+ # The connect call should have failed
+ self.assertTrue(self.failure_triggered)
- # Autoconnect should then try again
- condition = 'obj.state == DeviceState.connecting'
+ condition = "obj.scanning"
+ wd.wait_for_object_condition(device, condition)
+ condition = "not obj.scanning"
wd.wait_for_object_condition(device, condition)
- device.wait_for_event("connecting (netconfig)")
+ # IWD should attempt to connect, but it will of course fail again.
+ device.wait_for_event("netconfig-failed", timeout=1000)
device.disconnect()
condition = 'obj.state == DeviceState.disconnected'
diff --git a/autotests/testNetconfigRoam/netconfig_roam_test.py b/autotests/testNetconfigRoam/netconfig_roam_test.py
index 63e5eabf..b7a7012f 100644
--- a/autotests/testNetconfigRoam/netconfig_roam_test.py
+++ b/autotests/testNetconfigRoam/netconfig_roam_test.py
@@ -19,7 +19,7 @@ class Test(unittest.TestCase):
device = wd.list_devices(1)[0]
device.get_ordered_network('TestFT', full_scan=True)
- device.connect_bssid(self.bss_hostapd[1].bssid)
+ device.connect_bssid(self.bss_hostapd[1].bssid, wait=False)
self.bss_hostapd[1].wait_for_event(f'AP-STA-CONNECTED {device.address}')
device.wait_for_event("connecting (netconfig)")
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/4] doc: add note about timeouts to Network.Connect()
2025-05-28 14:27 [PATCH 1/4] station: treat netconfig failures as connection failures James Prestwood
2025-05-28 14:27 ` [PATCH 2/4] auto-t: allow configurable DBus timeout/callbacks on connect{_bssid} James Prestwood
2025-05-28 14:27 ` [PATCH 3/4] auto-t: update serveral tests to work with netconfig refactor James Prestwood
@ 2025-05-28 14:27 ` James Prestwood
2025-05-28 17:47 ` [PATCH 1/4] station: treat netconfig failures as connection failures Denis Kenzior
3 siblings, 0 replies; 7+ messages in thread
From: James Prestwood @ 2025-05-28 14:27 UTC (permalink / raw)
To: iwd; +Cc: James Prestwood
Since netconfig is now part of the Connect() call from a DBus
perspective add a note indicating that this method has the potential
to take a very long time if there are issues with DHCP.
---
doc/network-api.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/doc/network-api.txt b/doc/network-api.txt
index 8bc6eea6..d96fcede 100644
--- a/doc/network-api.txt
+++ b/doc/network-api.txt
@@ -11,6 +11,12 @@ Methods void Connect()
the object path to connect to specified network.
Connecting to WEP networks is not supported.
+ Note: When [General].EnableNetworkConfiguration is set
+ to true a call to Connect() has the potential to take
+ a significant amount of time. Specifically if DHCP is
+ either slow, or is unable to complete. The timeout for
+ DHCP is roughly 30 seconds per BSS.
+
Possible errors: net.connman.iwd.Aborted
net.connman.iwd.Busy
net.connman.iwd.Failed
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/4] station: treat netconfig failures as connection failures
2025-05-28 14:27 [PATCH 1/4] station: treat netconfig failures as connection failures James Prestwood
` (2 preceding siblings ...)
2025-05-28 14:27 ` [PATCH 4/4] doc: add note about timeouts to Network.Connect() James Prestwood
@ 2025-05-28 17:47 ` Denis Kenzior
3 siblings, 0 replies; 7+ messages in thread
From: Denis Kenzior @ 2025-05-28 17:47 UTC (permalink / raw)
To: James Prestwood, iwd
Hi James,
On 5/28/25 9:27 AM, James Prestwood wrote:
> Currently the netconfig functionality is somewhat separated from
> IWD's "connection" process. Regardless if netconfig is enabled
> the method return from Connect() will happen after IWD has completed
> the 4-way handshake. If netconfig fails its shows externally as an
> IWD state change rather than part of the method return from
> Connect(). Overall this doesn't pose a significant problem since a
It looks like you're fixing this as part of this commit to send the DBus reply
after the netconfig has been completed. Can you separate this change into a
separate commit and document it for posterity? Not sure if NM relies on this
behavior (hopefully not), but we should have a Fixes tag as well.
Maybe 72e7d3ceb83d ("station: Handle NETCONFIG_EVENT_FAILED") ?
> netconfig failure essentially appears as if IWD became disconnected
> but more critical is that IWD will not iteratively try more BSS's if
> netconfig fails.
>
> For example a BSS may be misconfigured, or not able to communicate to
> the DHCP server. IWD could connect to it, and fail netconfig thereby
> restarting the autoconnect logic. IWD will then choose the "best" BSS
> which may be the one it just failed on. This would then repeat
> indefinitely or until a better BSS comes around which could be never.
>
> To improve this netconfig has been adopted into the IWD's BSS retry
> logic. If netconfig fails this will not result in IWD transitioning
> to a disconnected state, and instead the BSS will be network
> blacklisted and the next will be tried. Only once all BSS's have been
> tried will IWD go into a disconnected state and start autoconnect
> over.
> ---
> src/station.c | 104 +++++++++++++++++++++++++-------------------------
> 1 file changed, 51 insertions(+), 53 deletions(-)
>
> diff --git a/src/station.c b/src/station.c
> index 2b6a18f8..81c1e595 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -1795,6 +1795,13 @@ static void station_enter_state(struct station *station,
> periodic_scan_stop(station);
> break;
> case STATION_STATE_CONNECTED:
> + if (station->connect_pending) {
> + struct l_dbus_message *reply =
> + l_dbus_message_new_method_return(
> + station->connect_pending);
> + dbus_pending_reply(&station->connect_pending, reply);
> + }
> +
Looks like this part should be in a separate commit per above comment.
> l_dbus_object_add_interface(dbus,
> netdev_get_path(station->netdev),
> IWD_STATION_DIAGNOSTIC_INTERFACE,
> @@ -2214,6 +2221,26 @@ static void station_early_neighbor_report_cb(struct netdev *netdev, int err,
> &station->roam_freqs);
> }
>
> +static bool station_try_next_bss(struct station *station)
> +{
> + struct scan_bss *next;
> + int ret;
> +
> + next = network_bss_select(station->connected_network, false);
> +
> + if (!next)
> + return false;
> +
> + ret = __station_connect_network(station, station->connected_network,
> + next, station->state);
> + if (ret < 0)
> + return false;
> +
> + l_debug("Attempting to connect to next BSS "MAC, MAC_STR(next->addr));
> +
> + return true;
> +}
> +
> static bool station_can_fast_transition(struct station *station,
> struct handshake_state *hs,
> struct scan_bss *bss)
> @@ -2256,28 +2283,28 @@ static bool station_can_fast_transition(struct station *station,
> return true;
> }
>
> -static void station_disconnect_on_error_cb(struct netdev *netdev, bool success,
> - void *user_data)
> +static void station_disconnect_on_netconfig_failed(struct netdev *netdev,
> + bool success,
> + void *user_data)
> {
> struct station *station = user_data;
> - bool continue_autoconnect;
>
> - station_enter_state(station, STATION_STATE_DISCONNECTED);
> + if (station_try_next_bss(station))
> + return;
>
> - continue_autoconnect = station->state == STATION_STATE_CONNECTING_AUTO;
> + network_disconnected(station->connected_network);
Hmm, why is this needed? station_reset_connection_state() already invokes this.
>
> - if (continue_autoconnect) {
> - if (station_autoconnect_next(station) < 0) {
> - l_debug("Nothing left on autoconnect list");
> - station_enter_state(station,
> - STATION_STATE_AUTOCONNECT_FULL);
> - }
> + if (station->connect_pending) {
> + struct l_dbus_message *reply = dbus_error_failed(
> + station->connect_pending);
>
> - return;
> + dbus_pending_reply(&station->connect_pending, reply);
> }
>
> - if (station->autoconnect)
> - station_enter_state(station, STATION_STATE_AUTOCONNECT_QUICK);
> + station_reset_connection_state(station);
> +
> + station_enter_state(station, STATION_STATE_DISCONNECTED);
> + station_enter_state(station, STATION_STATE_AUTOCONNECT_FULL);
> }
>
> static void station_netconfig_event_handler(enum netconfig_event event,
> @@ -2287,26 +2314,24 @@ static void station_netconfig_event_handler(enum netconfig_event event,
>
> switch (event) {
> case NETCONFIG_EVENT_CONNECTED:
> + network_connected(station->connected_network);
> station_enter_state(station, STATION_STATE_CONNECTED);
> break;
> case NETCONFIG_EVENT_FAILED:
> - if (station->connect_pending) {
> - struct l_dbus_message *reply = dbus_error_failed(
> - station->connect_pending);
> + station_debug_event(station, "netconfig-failed");
>
> - dbus_pending_reply(&station->connect_pending, reply);
> - }
> + netconfig_reset(station->netconfig);
>
> if (station->state == STATION_STATE_NETCONFIG)
> network_connect_failed(station->connected_network,
> false);
>
> + network_blacklist_add(station->connected_network,
> + station->connected_bss);
> +
> netdev_disconnect(station->netdev,
> - station_disconnect_on_error_cb,
> + station_disconnect_on_netconfig_failed,
> station);
> - station_reset_connection_state(station);
> -
> - station_enter_state(station, STATION_STATE_DISCONNECTING);
> break;
> default:
> l_error("station: Unsupported netconfig event: %d.", event);
> @@ -3409,26 +3434,6 @@ static void station_event_channel_switched(struct station *station,
> network_bss_update(network, station->connected_bss);
> }
>
> -static bool station_try_next_bss(struct station *station)
> -{
> - struct scan_bss *next;
> - int ret;
> -
> - next = network_bss_select(station->connected_network, false);
> -
> - if (!next)
> - return false;
> -
> - ret = __station_connect_network(station, station->connected_network,
> - next, station->state);
> - if (ret < 0)
> - return false;
> -
> - l_debug("Attempting to connect to next BSS "MAC, MAC_STR(next->addr));
> -
> - return true;
> -}
> -
> static bool station_retry_owe_default_group(struct station *station)
> {
> /*
> @@ -3581,13 +3586,6 @@ static void station_connect_ok(struct station *station)
>
> l_debug("");
>
> - if (station->connect_pending) {
> - struct l_dbus_message *reply =
> - l_dbus_message_new_method_return(
> - station->connect_pending);
> - dbus_pending_reply(&station->connect_pending, reply);
> - }
> -
> /*
> * Get a neighbor report now so future roams can avoid waiting for
> * a report at that time
And this chunk belongs in a separate commit as well.
> @@ -3598,8 +3596,6 @@ static void station_connect_ok(struct station *station)
> l_warn("Could not request neighbor report");
> }
>
> - network_connected(station->connected_network);
> -
I'm not so sure about this part. network_connected() is mainly about syncing
the PSK to disk. Since we connected successfully, that should still happen,
regardless of whether netconfig succeeds or not.
> if (station->netconfig) {
> if (hs->fils_ip_req_ie && hs->fils_ip_resp_ie) {
> struct ie_fils_ip_addr_response_info info;
> @@ -3639,8 +3635,10 @@ static void station_connect_ok(struct station *station)
> return;
>
> station_enter_state(station, STATION_STATE_NETCONFIG);
> - } else
> + } else {
> + network_connected(station->connected_network);
> station_enter_state(station, STATION_STATE_CONNECTED);
> + }
> }
>
> static void station_connect_cb(struct netdev *netdev, enum netdev_result result,
Regards,
-Denis
^ permalink raw reply [flat|nested] 7+ messages in thread