Re: [PATCH] remove AP always on device disconnect

[Dan Williams <dcbw redhat com>]

On Wed, 2011-10-12 at 10:11 +0200, Ludwig Nussel wrote:
> Dan Williams wrote:
> > On Tue, 2011-10-11 at 16:51 +0200, Ludwig Nussel wrote:
> >> Dan Williams wrote:
> >>> On Fri, 2011-10-07 at 15:21 +0200, Ludwig Nussel wrote:
> >>>> This avoids immediate reconnect after link timeout to an AP that may no longer exist (down/out of range). This also avoids needless prompting for a password for the no longer existing AP.
> >>>
> >>> This could work, but I think we need to restrict the AP removal to cases
> >>> where the supplicant times out after we've already been successfully
> >>> connected, which is basically the case you're talking about.  For
> >>> example, in the case of an explicit disconnect (via the menu item or
> >>> D-Bus call) we'd remove a perfectly good AP and might not find it again
> >>> until the next scan happened a few minutes after the disconnect.
> >>
> >> Yes. Is there a way to find out whether the request was triggered by
> >> an event from wpa_supplicant or by the user?
> >>
> >>> Maybe instead of doing it during deactivate, remove the AP in
> >>> supplicant_connection_timeout_cb() right before moving to the
> >>> DISCONNECTED state?
> >>
> >> Doesn't that callback trigger too late? Ie after NM has already
> >> prompted for a new password?
> > 
> > That function *should* be the place where disconnects after successful
> > connection are handled.  But yeah, the code is wrong in
> > supplicant_iface_state_cb() (probably from a6f7dfff233de118) so that
> > case doesn't get hit correctly.  I'll fix that, but in the end, here's
> > what should happen:
> 
> I think it's been that way before and intentionally so. To handle the
> first case you listed below.
> 
> > 1) disconnect during connection process: if we can tell that the
> > password is wrong (need help from wpa_supplicant here) ask for new
> > secrets; otherwise if we've connected to this SSID before (ie, timestamp
> >> 0) then don't ask for secrets, fail with an error code that the
> > secrets might be wrong.  If we haven't connected successfully before,
> > then possibly ask for secrets.
> 
> Sounds good. That's where supplicant_iface_state_cb() is involved I think.
> 
> > 2) disconnect after successful connect: start a timer and wait for the
> > supplicant to reassociate; it that doesn't happen, then when the timer
> > fires, deactivate the device and mark the connection as "invalid" for a
> > short period of time (at least until new scan results come in)
> 
> Yes. That's what I wanted to achieve, ie prevent reconnect to a no longer
> existing AP. From my limited experiments I think this case happens when
> the AP gets switched off, goes out of range or kicks the client out.
> That's where link_timeout_cb() fires.
> 
> Besides the AP vanishing from the list on explicit user disconnect do you
> see any other bad side effect of my patch? The deadline for openSUSE
> 12.1 is near. So even if the patch is not acceptable for upstream I'd
> like to add it to our package anyways and accept that minor drawback to
> avoid users whining about the PK popups.

The problem I had was that it unconditionally removes the AP from the
list on deactivate, which can happen for any number of reasons, only one
of which is loss of sync with the AP.  Any of the 802.11 Reason codes
(802.11-2007 section 7.3.1.7) are reasons why this could happen; so do
we really want to remove the AP from the internal list and wait for the
next scan when the disconnect happened due to inactivity, invalid PSK,
etc.

Removing the AP from link_timeout_cb() is one way to restrict the cases
where the AP would be removed.  Even better would be to get better
status codes from the supplicant (or kernel via nl80211!) and only
remove the AP from the list when the driver reported generic disconnect,
or reason codes 3 or 8, for example.

Dan