[PATCH] dbus: Make P2P group properties accessible individually

[Previte, ToddX A]

>-----Original Message-----
>From: hostap-bounces at lists.shmoo.com [mailto:hostap-bounces at lists.shmoo.com] On Behalf Of Jouni Malinen
>Sent: Saturday, February 25, 2012 1:18 AM
>To: hostap at 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