Re: [PATCH 3/4] supplicant: only signal ScanDone if the scan really is done (or failed)

[Dan Williams <dcbw redhat com>]

On Thu, 2014-09-04 at 22:04 -0400, Mathieu Trudel-Lapierre wrote:
> It's counter-intuitive to signal a ScanDone when the request to Scan succeeds
> with the supplicant, because we'll get one ScanDone very early one when
> the scan begins, which will trigger the usual mechanisms, and again once
> the scan really is finished.
>
> It does still make sense however to signal when the request fails.
>
> Signed-off-by: Mathieu Trudel-Lapierre <mathieu trudel-lapierre canonical com>
> ---
>  src/supplicant-manager/nm-supplicant-interface.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/supplicant-manager/nm-supplicant-interface.c 
> b/src/supplicant-manager/nm-supplicant-interface.c
> index 77924b3..d495213 100644
> --- a/src/supplicant-manager/nm-supplicant-interface.c
> +++ b/src/supplicant-manager/nm-supplicant-interface.c
> @@ -1176,10 +1176,11 @@ scan_request_cb (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data)
>       GError *err = NULL;
>  
>       nm_call_store_remove (priv->other_pcalls, proxy, call_id);
> -     if (!dbus_g_proxy_end_call (proxy, call_id, &err, G_TYPE_INVALID))
> +     if (!dbus_g_proxy_end_call (proxy, call_id, &err, G_TYPE_INVALID)) {
>               nm_log_warn (LOGD_SUPPLICANT, "Could not get scan request result: %s", err->message);
> +             g_signal_emit (self, signals[SCAN_DONE], 0, FALSE);
> +     }
>  
> -     g_signal_emit (self, signals[SCAN_DONE], 0, err ? FALSE : TRUE);
>       g_clear_error (&err);
>  }

I dug more into the supplicant code, and both the WEXT and nl80211
drivers implement a scan timeout, which should emit an scan event if the
driver never replies.  So I think this patch is actually sufficient, and
we can rely on the supplicant to indicate when a scan has finished.  If
that never happens for some reason, we should fix the supplicant.  For
extra credit we could do another timeout in NM itself, though that seems
somewhat overkill.

So contrary to my earlier statements on IRC, I think this patch is fine.

Dan