On Monday 11 June 2007 08:21, andy@xxxxxxxxxxx wrote:
> +struct ieee80211_radiotap_iterator {
> + struct ieee80211_radiotap_header *rtheader;
> + int max_length;
> + int this_arg_index;
> + u8 * this_arg;
^
Eliminate the space here between * and this_arg.
> +
> + int arg_index;
> + u8 * arg;
And here. ^
> + __le32 *next_bitmap;
> + u32 bitmap_shifter;
> +};
> +
> +extern int ieee80211_radiotap_iterator_init(
> + struct ieee80211_radiotap_iterator * iterator,
Here. ^
> + struct ieee80211_radiotap_header * radiotap_header,
You get the idea. ^
> Index: wireless-dev/net/wireless/radiotap.c
> ===================================================================
> --- /dev/null
> +++ wireless-dev/net/wireless/radiotap.c
> @@ -0,0 +1,256 @@
> +/*
> + * Radiotap parser
> + *
> + * Copyright 2007 Andy Green <andy@xxxxxxxxxxx>
> + */
> +
> +#include <linux/if.h>
Don't think anything from here is used.
> +#include <linux/err.h>
> +#include <linux/mutex.h>
We don't touch any mutexes in this file..
> +#include <linux/device.h>
Nor do we care about devices.
> +#include <net/genetlink.h>
Or netlink.
> +#include <net/cfg80211.h>
> +#include <net/wireless.h>
Or wiphys.
> +#include "nl80211.h"
> +#include "core.h"
nl80211.h and core.h seem unnecessary too.
All these includes might be pulling some other header that you do need,
however. (linux/kernel.h probably covers it..)
> +int ieee80211_radiotap_iterator_init(
> + struct ieee80211_radiotap_iterator * iterator,
> + struct ieee80211_radiotap_header * radiotap_header,
> + int max_length)
> +{
> + /* Linux only supports version 0 radiotap format */
> +
I think the code would look better without a blank line following the comment.
> + if (radiotap_header->it_version)
> + return -EINVAL;
> +
> + /* sanity check for allowed length and radiotap length field */
> +
> + if (max_length < (le16_to_cpu(radiotap_header->it_len)))
Unnecessary parenthesis. ^ ^
> + return -EINVAL;
> +
> + iterator->rtheader = radiotap_header;
> + iterator->max_length = le16_to_cpu(radiotap_header->it_len);
> + iterator->arg_index = 0;
> + iterator->bitmap_shifter = le32_to_cpu(radiotap_header->it_present);
> + iterator->arg = ((u8 *)radiotap_header) +
Ditto. ^ ^
> + sizeof(struct ieee80211_radiotap_header);
I usually like to use sizeof on a variable (eg., sizeof(*radiotap_header))
since it's shorter and doesn't need to be changed if the variable type
changes.
> + iterator->this_arg = 0;
> +
> + /* find payload start allowing for extended bitmap(s) */
> +
> + if (unlikely(iterator->bitmap_shifter &
> + IEEE80211_RADIOTAP_PRESENT_EXTEND_MASK)) {
+--------^
Indenting this a bit more to the right should look better.
> + while (le32_to_cpu(*((u32 *)iterator->arg)) &
> + IEEE80211_RADIOTAP_PRESENT_EXTEND_MASK) {
+--------------^
Ditto.
> + iterator->arg += sizeof(u32);
> +
> + /*
> + * check for insanity where the present bitmaps
> + * keep claiming to extend up to or even beyond the
> + * stated radiotap header length
> + */
> +
> + if ((((int)iterator->arg) - ((int)iterator->rtheader)) >
> + iterator->max_length)
For pointer arithmetic, you should use unsigned long. The parenthesis are a
little too paranoid here too.
> + if ((((int)iterator->arg)-((int)iterator->rtheader)) &
unsigned long for pointer arithmetic and reduce parenthesis, as before. Seems
to be a lot of this, so I won't mention it again.
> + ((rt_sizes[iterator->arg_index] >> 4) - 1))
> + iterator->arg_index +=
> + (rt_sizes[iterator->arg_index] >> 4) -
> + ((((int)iterator->arg) -
> + ((int)iterator->rtheader)) &
> + ((rt_sizes[iterator->arg_index] >> 4) - 1));
> +
> + /*
> + * this is what we will return to user, but we need to
> + * move on first so next call has something fresh to test
> + */
> +
> + iterator->this_arg_index = iterator->arg_index;
> + iterator->this_arg = iterator->arg;
> + hit = 1;
> +
> + /* internally move on the size of this arg */
> +
> + iterator->arg += rt_sizes[iterator->arg_index] & 0x0f;
> +
> + /*
> + * check for insanity where we are given a bitmap that
> + * claims to have more arg content than the length of the
> + * radiotap section. We will normally end up equalling this
> + * max_length on the last arg, never exceeding it.
> + */
> +
> + if ((((int)iterator->arg) - ((int)iterator->rtheader)) >
> + iterator->max_length)
> + return -EINVAL;
> +
> + next_entry:
> +
A blank line after a label doesn't look right to me..
> + iterator->arg_index++;
> + if (unlikely((iterator->arg_index & 31) == 0)) {
> + /* completed current u32 bitmap */
> + if (iterator->bitmap_shifter & 1) {
> + /* b31 was set, there is more */
> + /* move to next u32 bitmap */
> + iterator->bitmap_shifter = le32_to_cpu(
> + *iterator->next_bitmap);
Move "le32_to_cpu(" down to the next line.
> + iterator->next_bitmap++;
> + } else {
> + /* no more bitmaps: end */
> + iterator->arg_index = sizeof(rt_sizes);
> + }
> + } else { /* just try the next bit */
> + iterator->bitmap_shifter >>= 1;
> + }
> +
> + /* if we found a valid arg earlier, return it now */
> +
> + if (hit)
> + return iterator->this_arg_index;
> +
Another unnecessary blank line. ;)
Thanks,
-Michael Wu
Attachment:
pgpbtT64GTuaH.pgp
Description: PGP signature
