> > +/*
> > + * This is the new wireless configuration interface.
>
> I don't think new makes a lot of sense here, it's hopefully the
> standad one soon.
Heh. I suppose it'll need a better description anyway.
> > + *
> > + * Copyright 2006 Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> > + */
> > +
> > +#include "core.h"
> > +#include <linux/if.h>
> > +#include <linux/module.h>
> > +#include <linux/err.h>
> > +#include <net/genetlink.h>
> > +#include <net/cfg80211.h>
> > +#include <linux/mutex.h>
> > +#include <linux/list.h>
>
> This include order seems odd. We normally include local headers
> last and net/ after linux/
Oh well. I often just stick in what I need in the order I needed it ;)
I'll change it.
> > +MODULE_AUTHOR("Johannes Berg");
> > +MODULE_LICENSE("GPL");
>
> Can you please add a MODULE_DESCRIPTION aswell?
Sure. Then again, didn't you just ask for "why can this be a module at
all" too? :)
> > +
> > +/* RCU might be appropriate here since we usually
> > + * only read the list, and that can happen quite
> > + * often because we need to do it for each command */
> > +LIST_HEAD(cfg80211_drv_list);
> > +DEFINE_MUTEX(cfg80211_drv_mutex);
>
> Any reason these are non-static? They aren't actually used outside
> of this file, and in general having non-static lists isn't very nice,
> we prefer having proper accessor functions.
They'll be used in nl80211 which wasn't part of this patchset. Having
accessor functions for lists is nice as long as it's add/remove, but I
don't like having iterator functions with callbacks and all that much.
And in fact, a list_for_each() is the only thing done with this list...
I'd prefer keeping it this way.
> > --- /dev/null
> > +++ b/net/wireless/wext-compat.c
> > @@ -0,0 +1,25 @@
> > +/* NOT YET */
> > +
>
> huh? this file isn't added to the build process and only contains
> a (non-standard formatted) comment. No point in adding it in this
> patch.
Heh, yeah, it's replaced right away in the next one. I just had wanted
to document the thoughts that went into that.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
