On Thu, 10 Sep 2020 at 02:47, Denis Kenzior <denkenz(a)gmail.com> wrote:
> Oh ok, yes, the original code contained an overflow bug, once I
> understood it I didn't like it either ;-) Hence the fix.
Ok, I'm not following now. Are you saying there was a possibility of an empty
(not NULL) set somehow? If so, shouldn't this have been caught in ap_start
somewhere? In the end, a NULL or empty set would produce an invalid IE.
Assuming a set is valid, then I don't see anything wrong with the current code?
Currently we add either 3 or 8 numbers to the set, but we had a bug there.
Looking at this code closely I totally hate how this is done. You
going to be calling l_uintset_contains a few million times
I was trying to understand whether you referred to the old code (which
was fine except if ap->rates was NULL where l_uintset_contains was
called in an endless loop...) or the new code (which is fine either
way). I guess you must have referred to the old code in the ap->rates
== NULL scenario... in that case I guess I also dislike that code
because of the corner case overflow bug but I can't see anything wrong
with it otherwise. And both versions should be fine once we fix the
Hence the fix ;-)
Or am I missing something?
i.e. what is wrong with
if (L_WARN_ON(!ap->rates || l_uintset_isempty(ap->rates))
.. proceed as before.
Since ap->rates shouldn't need to ever change after ap_start, I'd
rather not add explicit checks everywhere.
>> when a static 0..127 would have been enough.
> For an invalid set, 0..127 is still wasteful ;-)
Of course, but at least you don't have to think about the cast and the
arithmetic and go 'what in the world is that doing'...
But really, l_uintset_foreach seems to be the better candidate here.
>>> Granted we're iterating over values that don't correspond to
>>> rates, we could store indexes into the array of known rates to save
>>> some iterations.
>> Maybe consider just using wiphy_get_supported_rates() instead.
> Right, but we're giving the user (of the ap.h API) some control over
> which rates to support so the plan is to eventually call it but during
> ap_start only.
You're giving a no_cck hint, not control over the rates, right? So the rates
should still come from wiphy_get_supported_rates().
To be specific I think the final set should be a result of multiple
calls to wiphy_get_supported_rates and the no_cck_rates setting.
If you want to stuff these
into the uintset or just carry the no_cck hint into the IE builders is a
different question. I would think the latter would be easier.
Not sure about that, but I didn't mean to refactor this code, only fix
a few one-liners. The refactoring if needed can happen in an
independent commit without functionality changes.