| Message ID | 20240208010301.2630340-1-arowa@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | wpa_supplicant: dBus: Flush Sate Property Change | expand |
On Wed, Feb 07, 2024 at 05:02:40PM -0800, Arowa Suliman wrote: > The "State" property in wpa_supplicant helps track important WiFi > connection events (like when the security key is being updated). > Currently, updates to this property aren't always sent right away. This > delay can make it difficult to accurately track the different stages of > the connection process. > > This change ensures that any updates to the "State" property are sent > immediately. This will provide a more accurate and real-time view of the > WiFi connection process. This change breaks a couple of automated test cases: dbus_roam and dbus_remove_connected. It looks like at least the dbus_roam case is something that could result in breaking some deployed use cases due to modified behavior. The PropertiesChanged event that indicates State=completed could now go out without CurrentBSS being set. That seems undesired or at least would require more careful consideration of existing users of the interface and clear documentation of the potential risks in the commit message.
On Sun, Mar 3, 2024 at 11:38 AM Jouni Malinen <j@w1.fi> wrote: > > On Wed, Feb 07, 2024 at 05:02:40PM -0800, Arowa Suliman wrote: > > The "State" property in wpa_supplicant helps track important WiFi > > connection events (like when the security key is being updated). > > Currently, updates to this property aren't always sent right away. This > > delay can make it difficult to accurately track the different stages of > > the connection process. > > > > This change ensures that any updates to the "State" property are sent > > immediately. This will provide a more accurate and real-time view of the > > WiFi connection process. > > This change breaks a couple of automated test cases: dbus_roam and > dbus_remove_connected. It looks like at least the dbus_roam case is > something that could result in breaking some deployed use cases due to > modified behavior. The PropertiesChanged event that indicates > State=completed could now go out without CurrentBSS being set. That > seems undesired or at least would require more careful consideration of > existing users of the interface and clear documentation of the potential > risks in the commit message. I checked the failures in both tests and the reason for these failures is the assumption that the State and CurrentBSS properties are always sent together. After modifying the two tests to wait for these properties separately the tests are passing. Even without my change there is still a possibility that the properties could be sent separately. For example the state property changes right before WPA_DBUS_SEND_PROP_CHANGED_TIMEOUT and current BSS changes right after that.
Hi Jouni,
I ran the dbus_roam test 50 times, both with and without the State
flush modification. In both scenarios, the test consistently failed,
and the CurrentBSS property was sent prior to State=completed. Given
these results, is it fine to merge this change?
Logs from one of the runs without the change:
2024-07-15 14:26:02,729 DEBUG propertiesChanged:
dbus.Dictionary({dbus.String('State'): dbus.String('authenticating',
variant_level=1), dbus.String('CurrentAuthMode'):
dbus.String('WPA2-PSK+WPA-PSK', variant_level=1)},
signature=dbus.Signature('sv'))
2024-07-15 14:26:02,750 DEBUG propertiesChanged:
dbus.Dictionary({dbus.String('State'): dbus.String('associating',
variant_level=1), dbus.String('AuthStatusCode'): dbus.Int32(0,
variant_level=1)}, signature=dbus.Signature('sv'))
2024-07-15 14:26:02,791 DEBUG propertiesChanged:
dbus.Dictionary({dbus.String('State'): dbus.String('associated',
variant_level=1), dbus.String('CurrentBSS'):
dbus.ObjectPath('/fi/w1/wpa_supplicant1/Interfaces/0/BSSs/2',
variant_level=1), dbus.String('CurrentAuthMode'):
dbus.String('WPA2-PSK+WPA-PSK', variant_level=1)},
signature=dbus.Signature('sv'))
2024-07-15 14:26:02,850 DEBUG propertiesChanged:
dbus.Dictionary({dbus.String('State'): dbus.String('4way_handshake',
variant_level=1)}, signature=dbus.Signature('sv'))
2024-07-15 14:26:02,937 DEBUG propertiesChanged:
dbus.Dictionary({dbus.String('State'): dbus.String('completed',
variant_level=1), dbus.String('CurrentAuthMode'):
dbus.String('WPA2-PSK+WPA-PSK', variant_level=1)},
signature=dbus.Signature('sv'))
2024-07-15 14:26:02,940 ERROR Exception in handler for D-Bus signal:
Traceback (most recent call last):
File "/usr/lib/python3.8/site-packages/dbus/connection.py", line
232, in maybe_handle_message
self._handler(*args, **kwargs)
File "/usr/local/libexec/hostap/tests/hwsim/test_dbus.py", line
6070, in propertiesChanged
cur = properties["CurrentBSS"]
KeyError: 'CurrentBSS'
Thanks,
Arowa
On Mon, Mar 11, 2024 at 1:42 PM Arowa Suliman <arowa@chromium.org> wrote:
>
> On Sun, Mar 3, 2024 at 11:38 AM Jouni Malinen <j@w1.fi> wrote:
> >
> > On Wed, Feb 07, 2024 at 05:02:40PM -0800, Arowa Suliman wrote:
> > > The "State" property in wpa_supplicant helps track important WiFi
> > > connection events (like when the security key is being updated).
> > > Currently, updates to this property aren't always sent right away. This
> > > delay can make it difficult to accurately track the different stages of
> > > the connection process.
> > >
> > > This change ensures that any updates to the "State" property are sent
> > > immediately. This will provide a more accurate and real-time view of the
> > > WiFi connection process.
> >
> > This change breaks a couple of automated test cases: dbus_roam and
> > dbus_remove_connected. It looks like at least the dbus_roam case is
> > something that could result in breaking some deployed use cases due to
> > modified behavior. The PropertiesChanged event that indicates
> > State=completed could now go out without CurrentBSS being set. That
> > seems undesired or at least would require more careful consideration of
> > existing users of the interface and clear documentation of the potential
> > risks in the commit message.
>
> I checked the failures in both tests and the reason for these failures
> is the assumption that the State and CurrentBSS properties are always
> sent together. After modifying the two tests to wait for these
> properties separately the tests are passing. Even without my change
> there is still a possibility that the properties could be sent
> separately. For example the state property changes right before
> WPA_DBUS_SEND_PROP_CHANGED_TIMEOUT and current BSS changes right after
> that.
diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c index 00b38edf5..8db3407e9 100644 --- a/wpa_supplicant/dbus/dbus_new.c +++ b/wpa_supplicant/dbus/dbus_new.c @@ -2327,6 +2327,7 @@ void wpas_dbus_signal_prop_changed(struct wpa_supplicant *wpa_s, break; case WPAS_DBUS_PROP_STATE: prop = "State"; + flush = TRUE; break; case WPAS_DBUS_PROP_CURRENT_BSS: prop = "CurrentBSS";
The "State" property in wpa_supplicant helps track important WiFi connection events (like when the security key is being updated). Currently, updates to this property aren't always sent right away. This delay can make it difficult to accurately track the different stages of the connection process. This change ensures that any updates to the "State" property are sent immediately. This will provide a more accurate and real-time view of the WiFi connection process. Signed-off-by: Arowa Suliman <arowa@chromium.org> --- wpa_supplicant/dbus/dbus_new.c | 1 + 1 file changed, 1 insertion(+)