On Fri, 2008-05-16 at 23:12 +0200, Johannes Berg wrote:
> > > > +static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)
> > >
> > > That seems fine too.
> > >
> > > > +static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 ftype)
> > >
> > > That, I think, is misnamed, it should be ieee80211_is_ftype()
> >
> > Well, I didn't expect this to be used directly that much, hence the
> > following three helpers...ftype_mgmt/ctl/data
>
> Right. Still, I think it should be named with _is_ in there.
Yes, I agree with you, that's what I meant to write.
>
> > > > +static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 stype)
> > > > +{
> > > > + return (hdr->frame_control & cpu_to_le16(stype)) != 0;
> > > > +}
> > >
> > > And that even seems implemented wrongly? stype is a 4-bit field, this
> > > doesn't make much sense to me.
> >
> > It was meant to be used to replace code like:
> >
> > fc & IEEE80211_STYPE_QOS_DATA
> >
> > ieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA)
> >
> > As most users of the stype bits test against a specific constant,
> > otherwise you could add a bunch of helpers, one for each stype
> > constant (similar to the ftype helpers I added. There were only
> > 3 in the ftype case, so I just added them all and dropped the second
> > parameter, in the stype case, there are more options, so I just
> > left one two-parameter helper.
>
> Well, yes, but that's semantically unclear because of the way these bits
> are used vs. how they are called. The subtype (stype) is a four-bit
> field in the frame control field which isn't actually defined to have
> "flags" or bits like that in it. Look at the table in IEEE 802.11-2007,
> it's at the beginning of clause 7 somewhere.
>
> Hence, hiding this simple AND behind a function that claims to check the
> subtype is confusing.
Ahhh, I see what you are getting at now, how about ieee80211_stype_mask?
That makes the & a little clearer and has better semantic meaning.
The other option is still to call it stype_mask, but change it to
return a __le16....so tests against != 0 still work and really
is just a wrapper around the byteswap of the constant and the &.
What do you think of that?
Harvey
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
