Hi,
I don't want to disturb the good work you guys are doing, but
I had a few comments on your patch regarding the WE APIs.
In particular :
----------------------------------------------
+struct translate_scan {
+ char *start;
+ char *stop;
+ int count;
+};
+
+/* wrappers of WE functions that don't check for errors */
+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))
+ return -E2BIG;
+ data->start = new_start;
+ return 0;
+}
----------------------------------------------
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.
Having a return in the middle of a function adds footprint and
potential bugs, especially that those error paths are seldom
tested. In such function, you typically hold locks and have allocated
memory, and when exiting you need to make sure things are proper.
Thereofore the approach there was to ignore errors when buried
deep into the translation and only check that at a higher level. Think
Exception in C++.
For example, the code in the Airo driver :
-----------------------------------------------------------
/* Read and parse all entries */
while((!rc) && (BSSList.index != 0xffff)) {
/* Translate to WE format this entry */
current_ev = airo_translate_scan(dev, current_ev,
extra + dwrq->length,
&BSSList);
/* Check if there is space for one more entry */
if((extra + dwrq->length - current_ev) <= IW_EV_ADDR_LEN) {
/* Ask user space to try again with a bigger buffer */
return -E2BIG;
}
/* Read next entry */
rc = PC4500_readrid(ai, RID_BSSLISTNEXT,
&BSSList, sizeof(BSSList), 1);
}
-----------------------------------------------------------
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 ?
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.
The final code would look like :
----------------------------------------------------------
struct translate_scan data = {
.start = extra,
.stop = extra + wrqu->data.length,
.count = 0
};
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;
else
wrqu->data.length = data.start - extra;
----------------------------------------------------------
Basically, you use the buffer length given by
wrqu->data.length and you don't mess up with wrqu->data.length if you
return -E2BIG. Actually, if you were able to approximate how much
buffer you need, you can put that in wrqu->data.length.
If you have more questions, feel free to contact me...
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
