On 23-2-2016 15:20, Johannes Berg wrote:
> On Wed, 2016-02-17 at 11:47 +0100, Arend van Spriel wrote:
>>
>> + * @NL80211_ATTR_BSS_SELECT: nested attribute for driver supporting the
>> + * BSS selection feature. When used with %NL80211_CMD_GET_WIPHY it contains
>> + * NLA_FLAG type according &enum nl80211_bss_select_attr to indicate what
>> + * BSS selection behaviours are supported. When used with
>> %NL80211_CMD_CONNECT
>> + * it contains the behaviour and parameters for BSS selection to be done by
>> + * driver and/or firmware.
>
> Maybe we should be less specific here regarding the NLA_FLAG and say
> that it will contain attributes for each, indicating which behaviours
> are supported, and say there's behaviour-dependent data inside each of
> those attributes?
For the GET_WIPHY case there will be not behaviour dependent data.
> It seems entirely plausible that future behaviours would require their
> own sub-capabilities; perhaps that's even the case today for having an
> adjustment range for example, not that I think it's really necessary
> now.
At least it does not harm to take that into account.
>> @@ -3617,6 +3624,7 @@ enum nl80211_band {
>> NL80211_BAND_2GHZ,
>> NL80211_BAND_5GHZ,
>> NL80211_BAND_60GHZ,
>> + NUM_NL80211_BAND,
>
> You should use IEEE80211_NUM_BANDS and remove this.
Ok. missed that one.
>> +static bool is_band_valid(struct wiphy *wiphy, enum nl80211_band b)
>> +{
>> + return b < NUM_NL80211_BAND && wiphy->bands[b];
>> +}
>
> Here.
will change it.
>> +static int parse_bss_select(struct nlattr *nla, struct wiphy *wiphy,
>> + struct cfg80211_bss_selection
>> *bss_select)
>> +{
>> + struct nlattr *attr[NL80211_BSS_SELECT_ATTR_MAX + 1];
>> + struct nlattr *nest;
>> + int err;
>> + bool found = false;
>> + int i;
>> +
>> + /* only process one nested attribute */
>> + nest = nla_data(nla);
>> + if (!nla_ok(nest, nla_len(nest)))
>> + return -EBADF;
>
> This isn't quite clear to me. Shouldn't you do this by declaring it
> NLA_TESTED in the nl80211_policy?
Guess you mean NLA_NESTED.
> Actually, not really? you use nla_ok() on the inner (without having
> checked at all that it's even long enough btw, since you didn't do the
> policy change), but that would already be done by nla_parse() below?
Because ATTR_BSS_SELECT is used in CMD_GET_WIPHY and CMD_CONNECT the
length of the attribute is not a constant. Or do I only need to put
length in nl80211_policy for userspace-2-kernel direction, ie. for
ATTR_BSS_SELECT in CMD_CONNECT.
> What are you trying to do here?
ATTR_BSS_SELECT will have only a single nested attribute. So I looked at
nla_for_each_nested():
1233 /**
1234 * nla_for_each_attr - iterate over a stream of attributes
1235 * @pos: loop counter, set to current attribute
1236 * @head: head of attribute stream
1237 * @len: length of attribute stream
1238 * @rem: initialized to len, holds bytes currently remaining in stream
1239 */
1240 #define nla_for_each_attr(pos, head, len, rem) \
1241 for (pos = head, rem = len; \
1242 nla_ok(pos, rem); \
1243 pos = nla_next(pos, &(rem)))
1244
1245 /**
1246 * nla_for_each_nested - iterate over nested attributes
1247 * @pos: loop counter, set to current attribute
1248 * @nla: attribute containing the nested attributes
1249 * @rem: initialized to len, holds bytes currently remaining in stream
1250 */
1251 #define nla_for_each_nested(pos, nla, rem) \
1252 nla_for_each_attr(pos, nla_data(nla), nla_len(nla), rem)
So:
nla_for_each_nested(nest, nla, rem)
becomes:
for(nest = nla_data(nla), rem = nla_len(nla); nla_ok(nest, rem); ...)
As there is no need to iterate I just do the for loop initializer and
the loop condition using the if(nla_ok()).
> Regardless of that, EBADF seems like a bad error number.
Ok. Just use EINVAL here as well?
>> + err = nla_parse(attr, NL80211_BSS_SELECT_ATTR_MAX,
>> nla_data(nest),
>> + nla_len(nest), nl80211_bss_select_policy);
>> + if (err)
>> + return err;
>> +
>> + /* only one attribute may be given */
>> + for (i = 0; i <= NL80211_BSS_SELECT_ATTR_MAX; i++)
>> + if (attr[i]) {
>> + if (found)
>> + return -EINVAL;
>> + found = true;
>> + }
>
> I'd kinda prefer braces, but maybe that's just me. Surely not a
> blocking issue :)
I don't have strong opinion about it so I can change it.
Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
