From patchwork Sat Feb 25 09:17:54 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jouni Malinen X-Patchwork-Id: 143040 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from maxx.maxx.shmoo.com (maxx.shmoo.com [205.134.188.171]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "maxx.shmoo.com", Issuer "CA Cert Signing Authority" (not verified)) by ozlabs.org (Postfix) with ESMTPS id E5568B6FA4 for ; Sat, 25 Feb 2012 20:18:12 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 7854A9D211; Sat, 25 Feb 2012 04:18:10 -0500 (EST) X-Virus-Scanned: amavisd-new at maxx.shmoo.com Received: from maxx.maxx.shmoo.com ([127.0.0.1]) by localhost (maxx.shmoo.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id DfAE34P8pjC4; Sat, 25 Feb 2012 04:18:10 -0500 (EST) Received: from maxx.shmoo.com (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id E68149D207; Sat, 25 Feb 2012 04:18:04 -0500 (EST) X-Original-To: mailman-post+hostap@maxx.shmoo.com Delivered-To: mailman-post+hostap@maxx.shmoo.com Received: from localhost (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 154A09D207 for ; Sat, 25 Feb 2012 04:18:03 -0500 (EST) X-Virus-Scanned: amavisd-new at maxx.shmoo.com Received: from maxx.maxx.shmoo.com ([127.0.0.1]) by localhost (maxx.shmoo.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id B4VcumUdND6J for ; Sat, 25 Feb 2012 04:17:57 -0500 (EST) Received: from jmaline2.user.openhosting.com (kvm.w1.fi [128.177.28.162]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by maxx.maxx.shmoo.com (Postfix) with ESMTPS id DE7B39D200 for ; Sat, 25 Feb 2012 04:17:57 -0500 (EST) Received: from jm (a88-112-110-150.elisa-laajakaista.fi [88.112.110.150]) (authenticated bits=0) by jmaline2.user.openhosting.com (8.13.8/8.13.8) with ESMTP id q1P9CJdk012374 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Sat, 25 Feb 2012 04:12:20 -0500 Received: by jm (sSMTP sendmail emulation); Sat, 25 Feb 2012 11:17:54 +0200 Date: Sat, 25 Feb 2012 11:17:54 +0200 From: Jouni Malinen To: hostap@lists.shmoo.com Subject: Re: [PATCH] dbus: Make P2P group properties accessible individually Message-ID: <20120225091754.GD2344@w1.fi> Mail-Followup-To: hostap@lists.shmoo.com References: <1329955536-17057-1-git-send-email-angie.v.chinchilla@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1329955536-17057-1-git-send-email-angie.v.chinchilla@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: hostap@lists.shmoo.com X-Mailman-Version: 2.1.9 Precedence: list List-Id: HostAP Project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: hostap-bounces@lists.shmoo.com Errors-To: hostap-bounces@lists.shmoo.com 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: 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);