On Fri, Oct 8, 2010 at 2:06 PM, Ben Greear <greearb@xxxxxxxxxxxxxxx> wrote:
> On 10/08/2010 11:03 AM, Bob Copeland wrote:
>>
>> On Fri, Oct 8, 2010 at 12:43 PM,<greearb@xxxxxxxxxxxxxxx> wrote:
>>>
>>> @@ -558,6 +577,7 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc,
>>> struct ieee80211_vif *vif)
>>> memset(&iter_data.mask, 0xff, ETH_ALEN);
>>> iter_data.found_active = false;
>>> iter_data.need_set_hw_addr = true;
>>> + iter_data.opmode = NL80211_IFTYPE_UNSPECIFIED;
>>>
>>> if (vif)
>>> ath_vif_iter(&iter_data, vif->addr, vif);
>>> @@ -567,6 +587,11 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc,
>>> struct ieee80211_vif *vif)
>>> &iter_data);
>>> memcpy(sc->bssidmask, iter_data.mask, ETH_ALEN);
>>>
>>> + if (update_opmode&& sc->opmode != iter_data.opmode) {
>>> + sc->opmode = iter_data.opmode;
>>> + ath_do_set_opmode(sc);
>>> + }
>>> +
>>
>> Should we really couple updating bssid mask and configuring
>> the opmode? Generally, I dislike adding boolean flags to
>> functions because it's hard to figure out from the callsite
>> what is happening (you have to go back to the prototype), and
>> it usually indicates that the abstraction is a little broken.
>
> Well, we need to do the iteration over all VIFS to figure out what to set
> this
> too. Seems doing one iteration v/s doing two is worth the
> extra flag?
I admit I haven't looked at the context, so I'm not sure how ugly it
is, but you can often do this kind of thing by inverting the loop:
update_vif_data()
{
for all vifs:
compute new bssid & opmode
update_bssid(new_bssid)
update_opmode(neW_opmode)
}
and call that instead (even in the single vif case).
--
Bob Copeland %% www.bobcopeland.com
--
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
