[PATCH] Expose connected stations on DBus.

[Jouni Malinen]

My first response was a bit too long for this mailing list, so dropping
the attached patch.. It is available from here:
http://w1.fi/p/0001-D-Bus-Expose-connected-stations-in-AP-P2P-GO-mode.patch


On Wed, Dec 17, 2014 at 08:58:44PM -0500, Mathieu Trudel-Lapierre wrote:
> Make it possible to list connected stations in AP mode over DBus, along
> with some of their properties.

Could you please also provide a matching update to the D-Bus interface
documentation in doc/dbus.doxygen?

As far as the changes are concerned, they did not seem to be on top of
the current development snapshot. Nothing too difficult there, so I
rebased this and did some small coding style cleanup to make it easier
to review. The attached patch shows the version I reviewed and tested
with.

The most critical issue here seems to be in the change to the existing
StaAuthorized signal. Adding a new parameter to it breaks existing users
of this event, so that does not look like an acceptable modification.
wpas_dbus_interface_signals[] would have needed to be updated to match
the addition of the properties argument for this signal. If this new
station properties information is needed in authorization signal, it
looks like a new signal would need to be defined for this purpose in
order to not break existing users.

This patch breaks build without CONFIG_AP=y, i.e., that needs to be
cleaned up with ifdef CONFIG_AP and/or static inline wrappers etc. to
make sure all build configurations continue to work.


Some more comments below regarding the changes:

> Index: b/wpa_supplicant/dbus/dbus_new.c
>  static void wpas_dbus_signal_sta(struct wpa_supplicant *wpa_s,

> @@ -895,19 +900,33 @@ static void wpas_dbus_signal_sta(struct
>  	msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> -				      WPAS_DBUS_NEW_IFACE_INTERFACE, sig_name);
> +				      WPAS_DBUS_NEW_IFACE_INTERFACE,
> +				      sig_name);

Why? Looks unrelated to the added functionality and not really
consistent with the coding style in wpa_supplicant.

> +	dbus_message_iter_init_append(msg, &iter);
> +	if (!dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH,
> +					    &path))
> +		goto err;
> +
> +	if (properties) {
> +		if (!wpa_dbus_get_object_properties(iface, path,
> +						    WPAS_DBUS_NEW_IFACE_STA,
> +						    &iter))
> +			goto err;
> +	}

This is the part that breaks backwards compatibility.

> +static const struct wpa_dbus_signal_desc wpas_dbus_sta_signals[] = {
> +	/* Deprecated: use org.freedesktop.DBus.Properties.PropertiesChanged */
> +	{ "PropertiesChanged", WPAS_DBUS_NEW_IFACE_STA,

Why would we add something new that is already documented as deprecated?

> Index: b/wpa_supplicant/dbus/dbus_new_handlers.c

> +dbus_bool_t wpas_dbus_getter_sta_flags(DBusMessageIter *iter, DBusError *error,
> +				       void *user_data)

> +	struct sta_info *sta;

> +	return wpas_dbus_simple_property_getter(iter, DBUS_TYPE_UINT32,
> +						&sta->flags,
> +						error);

sta->flags is a bitfield of the WLAN_STA_* values. These are internal to
hostapd/wpa_supplicant implementation and are subject to change whenever
needed. As such, it is not appropriate to expose those values as-is
through a D-Bus interface that is supposed to remain stable. If some of
these flags are really needed to be exposed, those specific flags
should be mapped into something new defined for the D-Bus interface in a
way that can be maintained for long period of time.

> + * Getter for "RxPackets" property.
> + */
> +dbus_bool_t wpas_dbus_getter_sta_rx_packets(DBusMessageIter *iter, DBusError *error,

> +        if (hostapd_drv_read_sta_data(hapd, &data, sta->addr) < 0)

Fetching these values separately for each TX/RX statistics property
looks a bit inefficient since I'd assume GetAll() would be used to fetch
these most of the time (or a signal providing these in a group). Anyway,
I guess this can be acceptable if these is no convenient means for using
a single fetch for "grouped" use.

-- 
Jouni Malinen                                            PGP id EFC895FA