diff mbox

dbus: Make P2P group properties accessible individually

Message ID 20120225091754.GD2344@w1.fi
State Superseded
Headers show

Commit Message

Jouni Malinen Feb. 25, 2012, 9:17 a.m. UTC 
On Wed, Feb 22, 2012 at 04:05:36PM -0800, Angie Chinchilla wrote:
> Group properties are now accessible individually. The function to retrieve
> the dictionary containing the group properties is removed in favor of the
> individual functions. The group member properties are removed as well as
> they erroneously retrieved the group properties via the old function.

This patch was not completely consistent, so I did not apply it yet. I
fixed things that I noticed while reviewing the patch, but was not
completely sure whether that resulted with a version to matches with
what you tried to do here. Could you please verify whether the patch
below is correct?

> diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
> @@ -3126,10 +3126,37 @@ static const struct wpa_dbus_property_desc wpas_dbus_p2p_group_properties[] = {
> -	{ "Properties",
> -	  WPAS_DBUS_NEW_IFACE_P2P_GROUP, "a{sv}",
> -	  wpas_dbus_getter_p2p_group_properties,
> -	  wpas_dbus_setter_p2p_group_properties

This removes the only user of wpas_dbus_setter_p2p_group_properties(),
but that function was not removed. Was that on purpose?

> +	{ "Group", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "s",
> +	  wpas_dbus_getter_p2p_group,

wpas_dbus_getter_p2p_group() returns DBUS_TYPE_OBJECT_PATH, not a
string. In addition, this function was used in another interface. Was
this meant to be "o" here or not included at all in this interface?

> diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> -	if (!wpa_s->current_ssid)
> -		return FALSE;
> +	if (role == WPAS_P2P_ROLE_CLIENT)
> +		p_bssid = wpa_s->current_ssid->bssid;

This type of validation on wpa_s->current_ssid (or go_params or
ap_iface) != NULL were removed. Why? I added them back in the patch
below to make sure wpa_supplicant does not segfault on NULL pointer
dereference should the handler get called at inconvenient time. I'm not
sure whether this could happen in practice, but it looks safer to check
explicitly.


This is the version I had at the end of the review:

Comments

Previte, ToddX A Feb. 29, 2012, 10:50 p.m. UTC | #1 
>-----Original Message-----
>From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Jouni Malinen
>Sent: Saturday, February 25, 2012 1:18 AM
>To: hostap@lists.shmoo.com
>Subject: Re: [PATCH] dbus: Make P2P group properties accessible individually
>
>On Wed, Feb 22, 2012 at 04:05:36PM -0800, Angie Chinchilla wrote:
>> Group properties are now accessible individually. The function to retrieve
>> the dictionary containing the group properties is removed in favor of the
>> individual functions. The group member properties are removed as well as
>> they erroneously retrieved the group properties via the old function.
>
>This patch was not completely consistent, so I did not apply it yet. I
>fixed things that I noticed while reviewing the patch, but was not
>completely sure whether that resulted with a version to matches with
>what you tried to do here. Could you please verify whether the patch
>below is correct?

There were a couple issues with the patch below, so I regenerated it based on the current hostap tree with your changes included. That patch has been submitted to the list for review.

>> diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
>> @@ -3126,10 +3126,37 @@ static const struct wpa_dbus_property_desc wpas_dbus_p2p_group_properties[] = {
>> -	{ "Properties",
>> -	  WPAS_DBUS_NEW_IFACE_P2P_GROUP, "a{sv}",
>> -	  wpas_dbus_getter_p2p_group_properties,
>> -	  wpas_dbus_setter_p2p_group_properties
>
>This removes the only user of wpas_dbus_setter_p2p_group_properties(),
>but that function was not removed. Was that on purpose?

No, it should not have been removed. It should have been renamed to wpas_dbus_setter_p2p_group_vendor_extensions(). We had some internal churn in generating this patch and it looks like a few things got lost in the shuffle.

>> +	{ "Group", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "s",
>> +	  wpas_dbus_getter_p2p_group,
>
>wpas_dbus_getter_p2p_group() returns DBUS_TYPE_OBJECT_PATH, not a
>string. In addition, this function was used in another interface. Was
>this meant to be "o" here or not included at all in this interface?

It should be 'o'. Another change that was lost in the churn.

>> diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
>> -	if (!wpa_s->current_ssid)
>> -		return FALSE;
>> +	if (role == WPAS_P2P_ROLE_CLIENT)
>> +		p_bssid = wpa_s->current_ssid->bssid;
>
>This type of validation on wpa_s->current_ssid (or go_params or
>ap_iface) != NULL were removed. Why? I added them back in the patch
>below to make sure wpa_supplicant does not segfault on NULL pointer
>dereference should the handler get called at inconvenient time. I'm not
>sure whether this could happen in practice, but it looks safer to check
>explicitly.

Fair enough. I removed it in favor of the check for the current role, but it seems perfectly reasonable to have the extra safety net there. 


-T
diff mbox

Patch

diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
index a240649..bc44fcc 100644
--- a/wpa_supplicant/dbus/dbus_new.c
+++ b/wpa_supplicant/dbus/dbus_new.c
@@ -3126,10 +3126,37 @@  static const struct wpa_dbus_property_desc wpas_dbus_p2p_group_properties[] = {
 	  wpas_dbus_getter_p2p_group_members,
 	  NULL
 	},
-	{ "Properties",
-	  WPAS_DBUS_NEW_IFACE_P2P_GROUP, "a{sv}",
-	  wpas_dbus_getter_p2p_group_properties,
-	  wpas_dbus_setter_p2p_group_properties
+	{ "Group", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "o",
+	  wpas_dbus_getter_p2p_group,
+	  NULL
+	},
+	{ "Role", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "s",
+	  wpas_dbus_getter_p2p_role,
+	  NULL
+	},
+	{ "SSID", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "ay",
+	  wpas_dbus_getter_p2p_group_ssid,
+	  NULL
+	},
+	{ "BSSID", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "ay",
+	  wpas_dbus_getter_p2p_group_bssid,
+	  NULL
+	},
+	{ "Frequency", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "q",
+	  wpas_dbus_getter_p2p_group_frequency,
+	  NULL
+	},
+	{ "Passphrase", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "s",
+	  wpas_dbus_getter_p2p_group_passphrase,
+	  NULL
+	},
+	{ "PSK", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "ay",
+	  wpas_dbus_getter_p2p_group_psk,
+	  NULL
+	},
+	{ "WPSVendorExtensions", WPAS_DBUS_NEW_IFACE_P2P_GROUP, "aay",
+	  wpas_dbus_getter_p2p_group_vendor_ext,
+	  NULL
 	},
 	{ NULL, NULL, NULL, NULL, NULL }
 };
@@ -3251,10 +3278,6 @@  void wpas_dbus_unregister_p2p_group(struct wpa_supplicant *wpa_s,
 
 static const struct wpa_dbus_property_desc
 wpas_dbus_p2p_groupmember_properties[] = {
-	{ "Properties", WPAS_DBUS_NEW_IFACE_P2P_GROUPMEMBER, "a{sv}",
-	  wpas_dbus_getter_p2p_group_properties,
-	  NULL
-	},
 	{ NULL, NULL, NULL, NULL, NULL }
 };
 
diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
index dbf61bd..825fd3c 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
@@ -1798,9 +1798,11 @@  dbus_bool_t wpas_dbus_getter_p2p_group_members(DBusMessageIter *iter,
 	const u8 *addr;
 	dbus_bool_t success = FALSE;
 
-	/* Ensure we are a GO */
-	if (wpa_s->wpa_state != WPA_COMPLETED)
-		return FALSE;
+	/* Verify correct role for this property */
+	if (wpas_get_p2p_role(wpa_s) != WPAS_P2P_ROLE_GO) {
+		return wpas_dbus_simple_array_property_getter(
+			iter, DBUS_TYPE_OBJECT_PATH, NULL, 0, error);
+	}
 
 	ssid = wpa_s->conf->ssid;
 	/* At present WPAS P2P_GO mode only applicable for p2p_go */
@@ -1848,166 +1850,140 @@  out_of_memory:
 }
 
 
-dbus_bool_t wpas_dbus_getter_p2p_group_properties(DBusMessageIter *iter,
-						  DBusError *error,
-						  void *user_data)
+dbus_bool_t wpas_dbus_getter_p2p_group_ssid(DBusMessageIter *iter,
+					    DBusError *error, void *user_data)
 {
 	struct wpa_supplicant *wpa_s = user_data;
-	DBusMessageIter variant_iter, dict_iter;
-	struct hostapd_data *hapd = NULL;
-	const struct wpabuf *vendor_ext[MAX_WPS_VENDOR_EXTENSIONS];
-	int num_vendor_ext = 0;
-	int i;
-	u8 role = wpas_get_p2p_role(wpa_s);
-	u16 op_freq = 0;
-	u8 *p_bssid = NULL;
-	char *role_name = NULL;
-
-	if (!wpa_s->current_ssid)
+	if (wpa_s->current_ssid == NULL)
 		return FALSE;
+	return wpas_dbus_simple_array_property_getter(
+		iter, DBUS_TYPE_BYTE, wpa_s->current_ssid->ssid,
+		wpa_s->current_ssid->ssid_len, error);
+}
 
-	/* Check current role and adjust information accordingly */
-	switch (role) {
-	case WPAS_P2P_ROLE_CLIENT:
-		/* go_params is only valid for a client */
-		if (wpa_s->go_params) {
-			op_freq = wpa_s->go_params->freq;
-			p_bssid = wpa_s->current_ssid->bssid;
-			role_name = "client";
-		} else
+
+dbus_bool_t wpas_dbus_getter_p2p_group_bssid(DBusMessageIter *iter,
+					     DBusError *error,
+					     void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	u8 role = wpas_get_p2p_role(wpa_s);
+	u8 *p_bssid;
+
+	if (role == WPAS_P2P_ROLE_CLIENT) {
+		if (wpa_s->current_ssid == NULL)
 			return FALSE;
-		break;
-	case WPAS_P2P_ROLE_GO:
-		/* ap_iface is only valid for a GO */
-		if (wpa_s->ap_iface) {
-			hapd = wpa_s->ap_iface->bss[0];
-			p_bssid = hapd->own_addr;
-			op_freq = wpa_s->ap_iface->freq;
-			role_name = "GO";
-		} else
+		p_bssid = wpa_s->current_ssid->bssid;
+	} else {
+		if (wpa_s->ap_iface == NULL)
 			return FALSE;
-		break;
-	default:
-		/* Error condition; this should NEVER occur */
-		return FALSE;
+		p_bssid = wpa_s->ap_iface->bss[0]->own_addr;
 	}
 
-	if (!dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
-					      "a{sv}", &variant_iter) ||
-	    !wpa_dbus_dict_open_write(&variant_iter, &dict_iter))
-		goto err_no_mem;
-	/* Provide the SSID */
-	if (!wpa_dbus_dict_append_byte_array(
-		    &dict_iter, "SSID",
-		    (const char *) wpa_s->current_ssid->ssid,
-		    wpa_s->current_ssid->ssid_len))
-		goto err_no_mem;
-	/* Provide the BSSID */
-	if (p_bssid &&
-	    !wpa_dbus_dict_append_byte_array(&dict_iter, "BSSID",
-					     (const char *) p_bssid, ETH_ALEN))
-		goto err_no_mem;
-	/* Provide the role within the group */
-	if (role_name &&
-	    !wpa_dbus_dict_append_string(&dict_iter, "Role", role_name))
-		goto err_no_mem;
-	/* Provide the operational frequency */
-	if (!wpa_dbus_dict_append_uint16(&dict_iter, "Frequency", op_freq))
-		goto err_no_mem;
+	return wpas_dbus_simple_array_property_getter(iter, DBUS_TYPE_BYTE,
+						      p_bssid, ETH_ALEN,
+						      error);
+}
 
-	/* Additional information for group owners */
-	if (role == WPAS_P2P_ROLE_GO) {
-		/* Provide the passphrase */
-		if (!wpa_dbus_dict_append_string(&dict_iter, "Passphrase",
-					wpa_s->current_ssid->passphrase))
-			goto err_no_mem;
-		/* Parse WPS Vendor Extensions sent in Beacon/Probe Response */
-		for (i = 0; hapd && i < MAX_WPS_VENDOR_EXTENSIONS; i++) {
-			if (hapd->conf->wps_vendor_ext[i] == NULL)
-				continue;
-			vendor_ext[num_vendor_ext++] =
-				hapd->conf->wps_vendor_ext[i];
-		}
-		if (!wpa_dbus_dict_append_wpabuf_array(&dict_iter,
-					"WPSVendorExtensions",
-					vendor_ext, num_vendor_ext))
-			goto err_no_mem;
-	} else {
-		/* If not a GO, provide the PSK */
-		if (!wpa_dbus_dict_append_byte_array(
-			    &dict_iter, "PSK",
-			    (const char *) wpa_s->current_ssid->psk, 32))
-			goto err_no_mem;
-	}
 
-	if (!wpa_dbus_dict_close_write(&variant_iter, &dict_iter) ||
-	    !dbus_message_iter_close_container(iter, &variant_iter))
-		goto err_no_mem;
+dbus_bool_t wpas_dbus_getter_p2p_group_frequency(DBusMessageIter *iter,
+						 DBusError *error,
+						 void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	u16 op_freq;
+	u8 role = wpas_get_p2p_role(wpa_s);
 
-	return TRUE;
+	if (role == WPAS_P2P_ROLE_CLIENT) {
+		if (wpa_s->go_params == NULL)
+			return FALSE;
+		op_freq = wpa_s->go_params->freq;
+	} else {
+		if (wpa_s->ap_iface == NULL)
+			return FALSE;
+		op_freq = wpa_s->ap_iface->freq;
+	}
 
-err_no_mem:
-	dbus_set_error_const(error, DBUS_ERROR_NO_MEMORY, "no memory");
-	return FALSE;
+	return wpas_dbus_simple_property_getter(iter, DBUS_TYPE_UINT16,
+						&op_freq, error);
 }
 
 
-dbus_bool_t wpas_dbus_setter_p2p_group_properties(DBusMessageIter *iter,
+dbus_bool_t wpas_dbus_getter_p2p_group_passphrase(DBusMessageIter *iter,
 						  DBusError *error,
 						  void *user_data)
 {
 	struct wpa_supplicant *wpa_s = user_data;
-	DBusMessageIter variant_iter, iter_dict;
-	struct wpa_dbus_dict_entry entry = { .type = DBUS_TYPE_STRING };
-	unsigned int i;
-	struct hostapd_data *hapd = NULL;
+	u8 role = wpas_get_p2p_role(wpa_s);
+	char *p_pass = NULL;
 
-	if (wpas_get_p2p_role(wpa_s) == WPAS_P2P_ROLE_GO &&
-	    wpa_s->ap_iface != NULL)
-		hapd = wpa_s->ap_iface->bss[0];
-	else
-		return FALSE;
+	/* Verify correct role for this property */
+	if (role == WPAS_P2P_ROLE_GO) {
+		if (wpa_s->current_ssid == NULL)
+			return FALSE;
+		p_pass = wpa_s->current_ssid->passphrase;
+	} else
+		p_pass = "";
 
-	dbus_message_iter_recurse(iter, &variant_iter);
-	if (!wpa_dbus_dict_open_read(&variant_iter, &iter_dict, error))
-		return FALSE;
+	return wpas_dbus_simple_property_getter(iter, DBUS_TYPE_STRING,
+						&p_pass, error);
 
-	while (wpa_dbus_dict_has_dict_entry(&iter_dict)) {
-		if (!wpa_dbus_dict_get_entry(&iter_dict, &entry)) {
-			dbus_set_error_const(error, DBUS_ERROR_INVALID_ARGS,
-					     "invalid message format");
+}
+
+dbus_bool_t wpas_dbus_getter_p2p_group_psk(DBusMessageIter *iter,
+					   DBusError *error, void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	u8 role = wpas_get_p2p_role(wpa_s);
+	u8 *p_psk = NULL;
+	u8 psk_len = 0;
+
+	/* Verify correct role for this property */
+	if (role == WPAS_P2P_ROLE_CLIENT) {
+		if (wpa_s->current_ssid == NULL)
 			return FALSE;
-		}
+		p_psk = wpa_s->current_ssid->psk;
+		psk_len = 32;
+	}
 
-		if (os_strcmp(entry.key, "WPSVendorExtensions") == 0) {
-			if (entry.type != DBUS_TYPE_ARRAY ||
-			    entry.array_type != WPAS_DBUS_TYPE_BINARRAY ||
-			    entry.array_len > MAX_WPS_VENDOR_EXTENSIONS)
-				goto error;
+	return wpas_dbus_simple_array_property_getter(iter, DBUS_TYPE_BYTE,
+						      &p_psk, psk_len, error);
+}
 
-			for (i = 0; i < MAX_WPS_VENDOR_EXTENSIONS; i++) {
-				if (i < entry.array_len) {
-					hapd->conf->wps_vendor_ext[i] =
-						entry.binarray_value[i];
-					entry.binarray_value[i] = NULL;
-				} else
-					hapd->conf->wps_vendor_ext[i] = NULL;
-			}
 
-			hostapd_update_wps(hapd);
-		} else
-			goto error;
+dbus_bool_t wpas_dbus_getter_p2p_group_vendor_ext(DBusMessageIter *iter,
+						  DBusError *error,
+						  void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	struct hostapd_data *hapd;
+	struct wpabuf *vendor_ext[MAX_WPS_VENDOR_EXTENSIONS];
+	int num_vendor_ext = 0;
+	int i;
 
-		wpa_dbus_dict_entry_clear(&entry);
-	}
+	/* Verify correct role for this property */
+	if (wpas_get_p2p_role(wpa_s) == WPAS_P2P_ROLE_GO) {
+		if (wpa_s->ap_iface == NULL)
+			return FALSE;
+		hapd = wpa_s->ap_iface->bss[0];
 
-	return TRUE;
+		/* Parse WPS Vendor Extensions sent in Beacon/Probe Response */
+		for (i = 0; i < MAX_WPS_VENDOR_EXTENSIONS; i++) {
+			if (hapd->conf->wps_vendor_ext[i] == NULL)
+				vendor_ext[i] = NULL;
+			else {
+				vendor_ext[num_vendor_ext++] =
+					hapd->conf->wps_vendor_ext[i];
+			}
+		}
+	}
 
-error:
-	wpa_dbus_dict_entry_clear(&entry);
-	dbus_set_error_const(error, DBUS_ERROR_INVALID_ARGS,
-			     "invalid message format");
-	return FALSE;
+	/* Return vendor extensions or no data */
+	return wpas_dbus_simple_array_array_property_getter(iter,
+							    DBUS_TYPE_BYTE,
+							    vendor_ext,
+							    num_vendor_ext,
+							    error);
 }
 
 
diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.h b/wpa_supplicant/dbus/dbus_new_handlers_p2p.h
index 293eb6b..ffc0112 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers_p2p.h
+++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.h
@@ -161,11 +161,27 @@  dbus_bool_t wpas_dbus_getter_p2p_group_members(DBusMessageIter *iter,
 					       DBusError *error,
 					       void *user_data);
 
-dbus_bool_t wpas_dbus_getter_p2p_group_properties(DBusMessageIter *iter,
+dbus_bool_t wpas_dbus_getter_p2p_group_ssid(DBusMessageIter *iter,
+					    DBusError *error,
+					    void *user_data);
+
+dbus_bool_t wpas_dbus_getter_p2p_group_bssid(DBusMessageIter *iter,
+					     DBusError *error,
+					     void *user_data);
+
+dbus_bool_t wpas_dbus_getter_p2p_group_frequency(DBusMessageIter *iter,
+						 DBusError *error,
+						 void *user_data);
+
+dbus_bool_t wpas_dbus_getter_p2p_group_passphrase(DBusMessageIter *iter,
 						  DBusError *error,
 						  void *user_data);
 
-dbus_bool_t wpas_dbus_setter_p2p_group_properties(DBusMessageIter *iter,
+dbus_bool_t wpas_dbus_getter_p2p_group_psk(DBusMessageIter *iter,
+					   DBusError *error,
+					   void *user_data);
+
+dbus_bool_t wpas_dbus_getter_p2p_group_vendor_ext(DBusMessageIter *iter,
 						  DBusError *error,
 						  void *user_data);