Re: [PATCH Try#9 3/4] cfg80211: Radiotap parser

[Michael Wu <flamingice@sourmilk.net>]

[-- Attachment #1: Type: text/plain, Size: 5653 bytes --]

On Monday 11 June 2007 08:21, andy@warmcat.com 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@warmcat.com>
> + */
> +
> +#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 #2: Type: application/pgp-signature, Size: 189 bytes --]