On Wed, 2011-11-02 at 19:19 +0100, Dmitry Tarnyagin wrote:
> +static int cmp_hidden_bss(struct cfg80211_bss *a,
> + struct cfg80211_bss *b)
> +{
> + const u8 *ie1;
> + const u8 *ie2;
> + size_t ielen;
> + int i;
> + int r;
> +
> + r = cmp_bss_core(a, b);
> + if (r)
> + return r;
> +
> + ie1 = cfg80211_find_ie(WLAN_EID_SSID,
> + a->information_elements,
> + a->len_information_elements);
> + ie2 = cfg80211_find_ie(WLAN_EID_SSID,
> + b->information_elements,
> + b->len_information_elements);
> + 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?)
> + if (!ie1 || !ie2)
> + return -1;
> +
> + ielen = min(ie1[1], ie2[1]);
> + for (i = 0; i < ielen; ++i)
++i here is a bit atypical code style.
> + 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?
> + return ie2[1] - ie1[1];
I'd error out much earlier if the length is different instead of
comparing to the minlen first.
> +static int
> +merge_hidden_ies(struct cfg80211_internal_bss *res,
> + struct cfg80211_internal_bss *hidden)
There seems little use for the return value.
> +{
> + if (unlikely(res->pub.beacon_ies))
> + return -EALREADY;
> + if (WARN_ON(!hidden->pub.beacon_ies))
> + return -EINVAL;
> +
> + res->pub.beacon_ies = kmalloc(hidden->pub.len_beacon_ies, GFP_ATOMIC);
> + if (unlikely(!res->pub.beacon_ies))
> + return -ENOMEM;
> +
> + res->beacon_ies_allocated = true;
> + res->pub.len_beacon_ies = hidden->pub.len_beacon_ies;
> + memcpy(res->pub.beacon_ies, hidden->pub.beacon_ies,
> + res->pub.len_beacon_ies);
> +
> + return 0;
> +}
> +
> +static struct cfg80211_internal_bss *
> cfg80211_bss_update(struct cfg80211_registered_device *dev,
> struct cfg80211_internal_bss *res)
> {
> @@ -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.
johannes
--
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
