On Tue, Aug 30, 2005 at 03:07:33PM +0200, Jirka Bohac wrote:
> Hi,
>
> > +/* wrappers of WE functions that don't check for errors */
> > +static inline int _iwe_stream_add_point(struct translate_scan *data,
> >
> > You may wonder why the original API of iwe_stream_add_point(()
> > is the way it is rather than the way you expect. I'll explain :
> > working closely with people doing embedded systems, one of my prime
> > concern has always been footprint and code simplicity, and this
> > explain those choices.
>
> ...
>
> >
> > Obviously, in your case it would be slightly different, so you
> > could do something like this :
> > ------------------------------------------------------
> > res = ieee80211_networks_foreach(ieee, ieee80211_translate_scan, &data);
> > if (res > 0) res = 0;
> > if (data.stop - data.start <= IW_EV_ADDR_LEN)
> > res = -E2BIG;
> > wrqu->data.length = data.start - extra;
> > ------------------------------------------------------
> > Then, obviously, you would be able to drop all the checks in
> > _iwe_stream_add_event(). Would you mind to try that and see how it
> > affect footprint and code readability ?
>
> You're suggesting this check:
> if (data.stop - data.start <= IW_EV_ADDR_LEN)
>
> Think of this sequence: add(8b); add(10b); add(8b) -- adding the
> ten bytes could silently do nothing, but subsequently adding
> eight bytes might succeed
>
> This would only work in cases where the largest event put into
> the stream would not exceed IW_EV_ADDR_LEN bytes; This is not the
> case. Think of the rates that are passed as a freeform string
> in an IWEVCUSTOM event (which is bad anyway).
>
> Even if we used the size of the largest possible event in the
> check above, I don't like the idea of lying to the userspace -
> returning -E2BIG even if there is enough space in the buffer.
> Let alone how error-prone the resulting code would be - anyone
> adding another event to the scan results would need
> to understand this dirty hack...
>
> I think the wrapper really makes the code cleaner and more
> readable.
Good point. I prefer to add that directly in the upstream
functions, to avoid wrapper of wrapper and make the code more
efficient, so I'll prepare a patch for Wireless Extensions ASAP.
Thinking about it, what about this alternate form that allow
to keep the overflow check offline :
---------------------------------------------------------
struct translate_scan {
char *start;
char *stop;
int count;
int error;
};
static inline int _iwe_stream_add_point(struct translate_scan *data,
struct iw_event *iwe, char *extra)
{
char *new_start;
new_start = iwe_stream_add_point(data->start, data->stop, iwe, extra);
if (unlikely(new_start == data->start))
data->error = -E2BIG;
data->start = new_start;
return 0;
}
---------------------------------------------------------
Tell me, and I'll work on it.
> > This brings us to my second point. You don't seem to support
> > arbitrary size scan buffer (WE-17). That would be a trivial
> > modification of your code, and allow to remove the arbitrary limit on
> > IW_SCAN_MAX_DATA.
>
> Good point, thanks. Will be fixed.
Technically, this is not a fix, but a new feature ;-)
> Regards,
>
> --
> Jirka Bohac <[EMAIL PROTECTED]>
Jean
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
