[v2] Adding disassociate reason as property in dbus

On Wed, Jul 01, 2015 at 11:53:13AM -0500, Dan Williams wrote:
> It gives disassoc_bssid as string and  disassoc_reason as integer.
> This patch is updated according  to the suggestions given by Dan Williams.

This commit message needs cleanup.. In addition, calling this
"disassociation reason" is very confusing taking into account the
changes are using the ASSOC_REJECT event which has nothing to do with
disassociation; this event happens before the association is formed.

> Avichal: I took the liberty of fixing a few things:
...

Thanks! The version below fixes some more issues (a typo in a function
name, missing documentation, replace disassoc_bssid string with a
6-octet u8 array and print the string version only if and when needed).

I'm not comfortable committing this, though, taken into account the
conflict between the name of the D-Bus property and the internal
EVENT_ASSOC_REJECT event. Is this patch really trying to make the last
EVENT_ASSOC_REJECT event information available through D-Bus? If so, the
property should be renamed and the commit message and dbus.doxygen
should clearly state what this information is and especially what it is
not (it is _not_ showing the reason for disassociation from associated
state).


From: Avichal Agarwal <avichal.a@samsung.com>
Subject: [PATCH] D-Bus: Add last ASSOC_REJECT reason as a property

This gives the BSSID (as a string) and reason (as an integer) of the
last ASSOC_REJECT event from the driver.

Signed-off-by: Avichal Agarwal <avichal.a@samsung.com>
---
 doc/dbus.doxygen                        |  7 +++++-
 wpa_supplicant/dbus/dbus_new.c          |  8 ++++++
 wpa_supplicant/dbus/dbus_new.h          |  1 +
 wpa_supplicant/dbus/dbus_new_handlers.c | 44 +++++++++++++++++++++++++++++++++
 wpa_supplicant/dbus/dbus_new_handlers.h |  4 +++
 wpa_supplicant/events.c                 | 10 ++++++--
 wpa_supplicant/notify.c                 |  7 ++++++
 wpa_supplicant/notify.h                 |  1 +
 wpa_supplicant/wpa_supplicant_i.h       |  4 +++
 9 files changed, 83 insertions(+), 3 deletions(-)

On Tue, 2015-07-07 at 18:22 +0300, Jouni Malinen wrote:
> On Wed, Jul 01, 2015 at 11:53:13AM -0500, Dan Williams wrote:
> > It gives disassoc_bssid as string and  disassoc_reason as integer.
> > This patch is updated according  to the suggestions given by Dan Williams.
> 
> This commit message needs cleanup.. In addition, calling this
> "disassociation reason" is very confusing taking into account the
> changes are using the ASSOC_REJECT event which has nothing to do with
> disassociation; this event happens before the association is formed.
> 
> > Avichal: I took the liberty of fixing a few things:
> ...
> 
> Thanks! The version below fixes some more issues (a typo in a function
> name, missing documentation, replace disassoc_bssid string with a
> 6-octet u8 array and print the string version only if and when needed).
> 
> I'm not comfortable committing this, though, taken into account the
> conflict between the name of the D-Bus property and the internal
> EVENT_ASSOC_REJECT event. Is this patch really trying to make the last
> EVENT_ASSOC_REJECT event information available through D-Bus? If so, the
> property should be renamed and the commit message and dbus.doxygen
> should clearly state what this information is and especially what it is
> not (it is _not_ showing the reason for disassociation from associated
> state).

Would you like the patch more if it *also* showed the reason for a
disassociation from associated state?  I'd say there's a case for
both...

Dan

> 
> From: Avichal Agarwal <avichal.a@samsung.com>
> Subject: [PATCH] D-Bus: Add last ASSOC_REJECT reason as a property
> 
> This gives the BSSID (as a string) and reason (as an integer) of the
> last ASSOC_REJECT event from the driver.
> 
> Signed-off-by: Avichal Agarwal <avichal.a@samsung.com>
> ---
>  doc/dbus.doxygen                        |  7 +++++-
>  wpa_supplicant/dbus/dbus_new.c          |  8 ++++++
>  wpa_supplicant/dbus/dbus_new.h          |  1 +
>  wpa_supplicant/dbus/dbus_new_handlers.c | 44 +++++++++++++++++++++++++++++++++
>  wpa_supplicant/dbus/dbus_new_handlers.h |  4 +++
>  wpa_supplicant/events.c                 | 10 ++++++--
>  wpa_supplicant/notify.c                 |  7 ++++++
>  wpa_supplicant/notify.h                 |  1 +
>  wpa_supplicant/wpa_supplicant_i.h       |  4 +++
>  9 files changed, 83 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/dbus.doxygen b/doc/dbus.doxygen
> index 3a08ad7..236a9d9 100644
> --- a/doc/dbus.doxygen
> +++ b/doc/dbus.doxygen
> @@ -673,6 +673,11 @@ fi.w1.wpa_supplicant1.CreateInterface.
>  	<h3>DisconnectReason - i - (read)</h3>
>  	<p>The most recent IEEE 802.11 reason code for disconnect. Negative value indicates locally generated disconnection.</p>
>        </li>
> +
> +      <li>
> +	<h3>DisassociateReason - (is) - (read)</h3>
> +	<p>The most recent disassociation reason and BSSID.</p>
> +      </li>
>      </ul>
>  
>  \subsection dbus_interface_signals Signals
> @@ -792,7 +797,7 @@ fi.w1.wpa_supplicant1.CreateInterface.
>  	<h4>Arguments</h4>
>  	<dl>
>  	  <dt>a{sv} : properties</dt>
> -	  <dd>A dictionary with pairs of properties names which have changed and theirs new values. Possible dictionary keys are: "ApScan", "Scanning", "State", "CurrentBSS", "CurrentNetwork"</dd>
> +	  <dd>A dictionary with pairs of properties names which have changed and theirs new values. Possible dictionary keys are: "ApScan", "Scanning", "State", "CurrentBSS", "CurrentNetwork", "DisassociateReason"</dd>
>  	</dl>
>        </li>
>  
> diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
> index 1959ea7..76bdf42 100644
> --- a/wpa_supplicant/dbus/dbus_new.c
> +++ b/wpa_supplicant/dbus/dbus_new.c
> @@ -1904,6 +1904,10 @@ void wpas_dbus_signal_prop_changed(struct wpa_supplicant *wpa_s,
>  		prop = "DisconnectReason";
>  		flush = TRUE;
>  		break;
> +	case WPAS_DBUS_PROP_DISASSOCIATE_REASON:
> +		prop = "DisassociateReason";
> +		flush = TRUE;
> +		break;
>  	default:
>  		wpa_printf(MSG_ERROR, "dbus: %s: Unknown Property value %d",
>  			   __func__, property);
> @@ -3020,6 +3024,10 @@ static const struct wpa_dbus_property_desc wpas_dbus_interface_properties[] = {
>  	  wpas_dbus_getter_disconnect_reason,
>  	  NULL
>  	},
> +	{ "DisassociateReason", WPAS_DBUS_NEW_IFACE_INTERFACE, "(is)",
> +	  wpas_dbus_getter_disassociate_reason,
> +	  NULL
> +	},
>  	{ NULL, NULL, NULL, NULL, NULL }
>  };
>  
> diff --git a/wpa_supplicant/dbus/dbus_new.h b/wpa_supplicant/dbus/dbus_new.h
> index 7503348..febbdf0 100644
> --- a/wpa_supplicant/dbus/dbus_new.h
> +++ b/wpa_supplicant/dbus/dbus_new.h
> @@ -29,6 +29,7 @@ enum wpas_dbus_prop {
>  	WPAS_DBUS_PROP_CURRENT_AUTH_MODE,
>  	WPAS_DBUS_PROP_BSSS,
>  	WPAS_DBUS_PROP_DISCONNECT_REASON,
> +	WPAS_DBUS_PROP_DISASSOCIATE_REASON,
>  };
>  
>  enum wpas_dbus_bss_prop {
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
> index 1c04e92..7c1c97a 100644
> --- a/wpa_supplicant/dbus/dbus_new_handlers.c
> +++ b/wpa_supplicant/dbus/dbus_new_handlers.c
> @@ -2797,6 +2797,50 @@ dbus_bool_t wpas_dbus_getter_disconnect_reason(DBusMessageIter *iter,
>  
> 
>  /**
> + * wpas_dbus_getter_disassociate_reason - Get bssid(string) with status code(int) for ASSOC_REJECT
> + * @iter: Pointer to incoming dbus message iter
> + * @error: Location to store error on failure
> + * @user_data: Function specific data
> + * Returns: TRUE on success, FALSE on failure
> + *
> + * Getter for "DisassociateReason" property.
> + */
> +dbus_bool_t wpas_dbus_getter_disassociate_reason(DBusMessageIter *iter,
> +						 DBusError *error,
> +						 void *user_data)
> +{
> +	struct wpa_supplicant *wpa_s = user_data;
> +	DBusMessageIter variant_iter;
> +	DBusMessageIter struct_iter;
> +	char bssid[20];
> +
> +	if (wpa_s->disassoc_reason)
> +		os_snprintf(bssid, sizeof(bssid), MACSTR,
> +			    MAC2STR(wpa_s->disassoc_bssid));
> +	else
> +		bssid[0] = '\0';
> +
> +	if (!dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
> +					      "(is)", &variant_iter) ||
> +	    !dbus_message_iter_open_container(&variant_iter, DBUS_TYPE_STRUCT,
> +					      NULL, &struct_iter) ||
> +	    !dbus_message_iter_append_basic(&struct_iter, DBUS_TYPE_INT32,
> +					    &wpa_s->disassoc_reason) ||
> +	    !dbus_message_iter_append_basic(&struct_iter, DBUS_TYPE_STRING,
> +					    &bssid) ||
> +	    !dbus_message_iter_close_container(&variant_iter, &struct_iter) ||
> +	    !dbus_message_iter_close_container(iter, &variant_iter)) {
> +		dbus_set_error(error, DBUS_ERROR_FAILED,
> +			       "%s: error constructing reply variant",
> +			       __func__);
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
> +
> +/**
>   * wpas_dbus_getter_bss_expire_age - Get BSS entry expiration age
>   * @iter: Pointer to incoming dbus message iter
>   * @error: Location to store error on failure
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.h b/wpa_supplicant/dbus/dbus_new_handlers.h
> index 50f72ec..9b6fbd7 100644
> --- a/wpa_supplicant/dbus/dbus_new_handlers.h
> +++ b/wpa_supplicant/dbus/dbus_new_handlers.h
> @@ -173,6 +173,10 @@ dbus_bool_t wpas_dbus_getter_disconnect_reason(DBusMessageIter *iter,
>  					       DBusError *error,
>  					       void *user_data);
>  
> +dbus_bool_t wpas_dbus_getter_disassociate_reason(DBusMessageIter *iter,
> +						 DBusError *error,
> +						 void *user_data);
> +
>  dbus_bool_t wpas_dbus_getter_bss_expire_age(DBusMessageIter *iter,
>  					    DBusError *error, void *user_data);
>  
> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> index aa171ef..f171722 100644
> --- a/wpa_supplicant/events.c
> +++ b/wpa_supplicant/events.c
> @@ -3328,15 +3328,21 @@ void wpa_supplicant_event(void *ctx, enum wpa_event_type event,
>  		break;
>  #endif /* CONFIG_IBSS_RSN */
>  	case EVENT_ASSOC_REJECT:
> -		if (data->assoc_reject.bssid)
> +		wpa_s->disassoc_reason = data->assoc_reject.status_code;
> +		os_memset(wpa_s->disassoc_bssid, 0, ETH_ALEN);
> +		if (data->assoc_reject.bssid) {
> +			os_memcpy(wpa_s->disassoc_bssid,
> +				  data->assoc_reject.bssid, ETH_ALEN);
>  			wpa_msg(wpa_s, MSG_INFO, WPA_EVENT_ASSOC_REJECT
>  				"bssid=" MACSTR	" status_code=%u",
>  				MAC2STR(data->assoc_reject.bssid),
>  				data->assoc_reject.status_code);
> -		else
> +		} else {
>  			wpa_msg(wpa_s, MSG_INFO, WPA_EVENT_ASSOC_REJECT
>  				"status_code=%u",
>  				data->assoc_reject.status_code);
> +		}
> +		wpas_notify_disassociate_reason(wpa_s);
>  		if (wpa_s->drv_flags & WPA_DRIVER_FLAGS_SME)
>  			sme_event_assoc_reject(wpa_s, data);
>  		else {
> diff --git a/wpa_supplicant/notify.c b/wpa_supplicant/notify.c
> index 822db74..7fbb62a 100644
> --- a/wpa_supplicant/notify.c
> +++ b/wpa_supplicant/notify.c
> @@ -117,6 +117,13 @@ void wpas_notify_disconnect_reason(struct wpa_supplicant *wpa_s)
>  }
>  
> 
> +void wpas_notify_disassociate_reason(struct wpa_supplicant *wpa_s)
> +{
> +	wpas_dbus_signal_prop_changed(wpa_s,
> +				      WPAS_DBUS_PROP_DISASSOCIATE_REASON);
> +}
> +
> +
>  void wpas_notify_network_changed(struct wpa_supplicant *wpa_s)
>  {
>  	if (wpa_s->p2p_mgmt)
> diff --git a/wpa_supplicant/notify.h b/wpa_supplicant/notify.h
> index 1aeec47..3912e67 100644
> --- a/wpa_supplicant/notify.h
> +++ b/wpa_supplicant/notify.h
> @@ -23,6 +23,7 @@ void wpas_notify_state_changed(struct wpa_supplicant *wpa_s,
>  			       enum wpa_states new_state,
>  			       enum wpa_states old_state);
>  void wpas_notify_disconnect_reason(struct wpa_supplicant *wpa_s);
> +void wpas_notify_disassociate_reason(struct wpa_supplicant *wpa_s);
>  void wpas_notify_network_changed(struct wpa_supplicant *wpa_s);
>  void wpas_notify_ap_scan_changed(struct wpa_supplicant *wpa_s);
>  void wpas_notify_bssid_changed(struct wpa_supplicant *wpa_s);
> diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h
> index dd5b245..4c3f0cf 100644
> --- a/wpa_supplicant/wpa_supplicant_i.h
> +++ b/wpa_supplicant/wpa_supplicant_i.h
> @@ -912,6 +912,10 @@ struct wpa_supplicant {
>  	/* WLAN_REASON_* reason codes. Negative if locally generated. */
>  	int disconnect_reason;
>  
> +	/* ASSOC_REJECT status code and bssid */
> +	u8 disassoc_bssid[ETH_ALEN];
> +	int disassoc_reason;
> +
>  	struct ext_password_data *ext_pw;
>  
>  	struct wpabuf *last_gas_resp, *prev_gas_resp;
> -- 
> 1.9.1
>

On Tue, Jul 07, 2015 at 10:45:55AM -0500, Dan Williams wrote:
> Would you like the patch more if it *also* showed the reason for a
> disassociation from associated state?  I'd say there's a case for
> both...

I don't know what the use case is for this new property change signal
(which, I'd assume, is the goal here.. getting out a D-Bus signal on
some specific events related to connection status) and as such,
including the disassociation case does not make any difference to me as
long as there are conflicts between the documentation and
implementation. If the commit message (and the D-Bus property name) were
to accurately describe what thisbeing done and what it is useful for, I
would have no issues with either of these cases.

Please keep in mind that disassociation happens only if there was an
association and the association reject event indicates that the attempt
to associate was rejected. These are very different cases from the view
point of association/disassociation management frames.