Hi Johannes,
On Thu, Nov 3, 2011 at 9:55 AM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>> + if (!ie1 && !ie2)
>> + return 0;
> I don't think it's valid to leave out the SSID IE, so I'd rather not
> have that test here. It might interact badly with other uses (mesh?)
>
ie1 and ie2 could not NULL at the same time, otherwise the bss
would be found in the first search attempt. However, I would prefer
to have some water-proof check here. WARN_ON? BUG_ON?
>> + for (i = 0; i < ielen; ++i)
> ++i here is a bit atypical code style.
>
Ok.
>> + if (ie2[i + 2])
>> + return -1;
> I don't understand this loop. Why check that ie2 is zeroed, but check
> only to the minlen of ie1, ie2?
>
Check that ie2 is zeroed is a check for hidden SSID.
Check only to minlent to be in-line with cmp_ies().
>> + return ie2[1] - ie1[1];
> I'd error out much earlier if the length is different instead of
> comparing to the minlen first.
>
Key comparator must use same algorithm in any rb-tree search function
(order is important), otherwise ordering of items in the tree is broken
and search gives incorrect results.
This code uses same order as cmp_ies() does.
>
>> +static int
>> +merge_hidden_ies(struct cfg80211_internal_bss *res,
>> + struct cfg80211_internal_bss *hidden)
> There seems little use for the return value.
>
Agree. int -> void
>> @@ -494,6 +578,13 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
>> spin_lock_bh(&dev->bss_lock);
>>
>> found = rb_find_bss(dev, res);
>> + if (!found) {
>> + struct cfg80211_internal_bss *hidden;
>> +
>> + hidden = rb_find_hidden_bss(dev, res);
>> + if (hidden)
>> + merge_hidden_ies(res, hidden);
>> + }
> I was going to complain that I don't understand why this works -- but
> then I think now I do. If you move the code down into the else branch
> it'll be much easier to follow.
>
Ok.
Thank you for review!
With best regards,
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
