Hi Tomasz,
On Tue, Jan 11, 2011 at 03:53:54PM +0200, Tomasz Bursztyka wrote:
> static void merge_network(GSupplicantNetwork *network)
> {
> GString *str;
> @@ -930,6 +939,11 @@ static void add_bss_to_network(struct g_supplicant_bss
> *bss)
> memcpy(network->ssid, bss->ssid, bss->ssid_len);
> network->signal = bss->signal;
>
> + network->wps = FALSE;
> + if ((bss->keymgmt & G_SUPPLICANT_KEYMGMT_WPS) ==
> + G_SUPPLICANT_KEYMGMT_WPS)
You probably only need to check for:
if (bss->keymgmt & G_SUPPLICANT_KEYMGMT_WPS) being != 0
> + network->wps = TRUE;
> +
> network->bss_table = g_hash_table_new_full(g_str_hash, g_str_equal,
> NULL, remove_bss);
>
> @@ -1024,6 +1038,75 @@ static void bss_wpa(const char *key, DBusMessageIter
> *iter,
>
> }
>
> +static unsigned int get_tlv(unsigned char *ie, unsigned int ie_size,
> +unsigned int type)
Indentation please.
So I suppose this is already done in wpa_supplicant but the outcome (as in
proper keymgmt setting) is not exported ?
> +{
> + unsigned int len = 0;
> +
> + while (len + 4 < ie_size) {
> + unsigned int hi = ie[len];
> + unsigned int lo = ie[len+1];
len + 1 and not len+1.
> + unsigned int tmp_type = (hi << 8) + lo;
> + unsigned int v_len = 0;
> +
> + hi = ie[len+2];
> + lo = ie[len+3];
Ditto.
> + v_len = (hi << 8) + lo;
> +
> + if (tmp_type == type) {
> + unsigned int ret_value = 0;
> + unsigned char *value = (unsigned char *)&ret_value;
> +
> + SUPPLICANT_DBG("IE: match type 0x%x", type);
> +
> + if (v_len > sizeof(unsigned int) ||
> + len+4+v_len > ie_size)
Ditto.
> + break;
> +
> + memcpy(value, ie+len+4, v_len);
Ditto.
> + SUPPLICANT_DBG("returning 0x%x", ret_value);
> + return ret_value;
> + }
> +
> + len += v_len + 4;
> + }
> +
> + SUPPLICANT_DBG("returning 0");
> + return 0;
> +}
I think the plan is to have wpa_supplicant letting us know about WPS key
management, and then being able to get rid of the above routine.
Meanwhile, I would still appreciate if you could replace all the magic
constants here by meaningful defines. This will help us understanding,
debugging and maintaining this piece of code.
> +static void bss_process_ies(DBusMessageIter *iter, void *user_data)
> +{
> + struct g_supplicant_bss *bss = user_data;
> + const unsigned char WPS_OUI[] = { 0x00, 0x50, 0xf2, 0x04 };
> + unsigned char *ie, *ie_end;
> + DBusMessageIter array;
> + int ie_len;
> +
> +#define WPS_VERSION_TLV 0x104A
> +#define WPS_STATE_TLV 0x1044
> +#define WPS_VERSION 0x10
> +
> + dbus_message_iter_recurse(iter, &array);
> + dbus_message_iter_get_fixed_array(&array, &ie, &ie_len);
> +
> + if (ie == NULL || ie_len < 2)
> + return;
> +
> + for (ie_end = ie+ie_len; ie+ie[1]+1 <= ie_end; ie += ie[1]+2) {
> + if (ie[0] == 221 && ie[1] >= 6 &&
> + memcmp(ie+2, WPS_OUI, sizeof(WPS_OUI)) == 0) {
For code style consistency, I'd rather see a :
if (ie[0] != 221 || ie[1] >= 6 ||
memcmp(ie + 2, WPS_OUI, sizeof(WPS_OUI)) != 0)
continue;
> + SUPPLICANT_DBG("IE: match WPS_OUI");
> +
> + if (get_tlv(&ie[6], ie[1],
> + WPS_VERSION_TLV) == WPS_VERSION &&
> + get_tlv(&ie[6], ie[1],
> + WPS_STATE_TLV) != 0)
I would feel more confortable with a check_wps() routine that would just go
through the IEs once and return true if the BSSID supports WPS. No big deal
though, just something nice to have if you find time for it.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
