[PATCH v3] hostap: Support ht-cap over-rides.

[Ben Greear]

On 12/18/2011 10:49 AM, Jouni Malinen wrote:
> On Tue, Nov 22, 2011 at 12:37:40PM -0800, greearb at candelatech.com wrote:
>> This allows ht-capabilities over-rides on kernels that
>> support these features.
>
>> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
>> @@ -26,6 +26,7 @@
>> +#include "common/ieee802_11_defs.h"
>
> I would prefer to not do this. driver.h tries to live mostly on its own
> without pulling in many of the header files. Some driver wrappers live
> outside the main hostap.git repository and the less dependencies there
> are between them and some header files I consider more internal to the
> core hostapd/wpa_supplicant, the better in general.
>
>> @@ -516,6 +517,21 @@ struct wpa_driver_associate_params {
>> +#ifdef CONFIG_HT_OVERRIDES
>> +	struct ieee80211_ht_capabilities htcaps;
>> +	struct ieee80211_ht_capabilities htcaps_mask;
>> +#endif
>
> i.e., these could be just const u8 *htcaps, *htcaps_mask and I would
> drop the #ifdef from this case to keep driver.h API more independent of
> the build configuration.

That just adds more void* pointers effectively, and that just
makes it easier to introduce bugs that the compiler could otherwise
easily catch.

I added all the ifdef stuff to make the code smaller per your previous
comments.  Making them pointers instead of members adds some extra coding
overhead to deal with properly freeing memory, but if you really want
them to be pointers, I will make the changes.

>> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
>> index 7943595..9b8df69 100644
>> --- a/src/drivers/driver_nl80211.c
>> +++ b/src/drivers/driver_nl80211.c
>> @@ -5617,6 +5617,16 @@ skip_auth_type:
>> +#ifdef CONFIG_HT_OVERRIDES
>> +	if (params->disable_ht)
>> +		NLA_PUT_FLAG(msg, NL80211_ATTR_DISABLE_HT);
>> +
>> +	NLA_PUT(msg, NL80211_ATTR_HT_CAPABILITY, sizeof(params->htcaps),
>> +		&params->htcaps);
>> +	NLA_PUT(msg, NL80211_ATTR_HT_CAPABILITY_MASK, sizeof(params->htcaps_mask),
>> +		&params->htcaps_mask);
>> +#endif
>
> I would drop the ifdef once params->htcaps != NULL can be used to see
> whether the value is set. Obviously, same for
> wpa_driver_nl80211_associate().


>> diff --git a/src/drivers/nl80211_copy.h b/src/drivers/nl80211_copy.h
>
> No need to include nl80211_copy.h changes in submissions. I'll sync this
> directly with wireless-testing.git anyway.
>
>> diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c
>> index b446a3f..35f446d 100644
>> --- a/wpa_supplicant/config.c
>> +++ b/wpa_supplicant/config.c
>> @@ -1864,6 +1875,14 @@ void wpa_config_set_network_defaults(struct wpa_ssid *ssid)
>> +#ifdef CONFIG_HT_OVERRIDES
>> +	ssid->ht_mcs = strdup("");
>
> Why? Wouldn't NULL work here?

Probably..but makes the parsing slightly harder if you have to check
for null as well as empty string.  I can change it to null if you want.

I won't be able to work on this until first of January, but will
post a new patch then that attempts to address your comments.

Thanks,
Ben

-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com