Re: [PATCH v8 3/6] wifi: mac80211: Use struct instead of macro for PERR frame

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

So I was just about to apply this set, but now I'm thinking about this:


> +struct ieee80211_mesh_hwmp_perr_dst {
> +	u8 flags;
> +	u8 addr[ETH_ALEN];
> +	__le32 sn;
> +	/* optional Destination External Address */
> +	u8 variable[];
> +} __packed;

This has a variable member, and the presence of the address in
'variable' depends on the flags (AE_F set there.)

> +struct ieee80211_mesh_hwmp_perr {
> +	u8 ttl;
> +	u8 number_of_dst;
> +	struct ieee80211_mesh_hwmp_perr_dst dsts[];
> +} __packed;

However this declares an array of these, and that doesn't really seem
right. It effectively puts non-variable fields (e.g. dsts[1].ttl) after
the variable field dsts[0].variable. I _think_ some compilers (versions)
will also (rightfully) complain about this.

It seems like this should just be

struct ... {
	u8 ttl;
	u8 number_of_dst;

	/* list of variably sized struct ieee80211_mesh_hwmp_perr_dst
*/
	u8 dsts[];
} __packed;

> +static inline u16
> +ieee80211_mesh_hwmp_perr_get_rcode(const u8 *ie, u8 dst_idx)
> +{
> +	struct ieee80211_mesh_hwmp_perr *perr_ie = (void *)ie;
> +	struct ieee80211_mesh_hwmp_perr_dst *dst =
> +		&perr_ie->dsts[dst_idx];

And especially this indexing doesn't seem like it could work - you have
to walk through all of them to see if each has the AE_F set and skip
sizeof() + optional ETH_ALEN.

> +
> +	return get_unaligned_le16(&dst->variable[
> +		(dst->flags & AE_F) ? ETH_ALEN : 0]);

This looks like the comment above should be

	/* optional Destination External Address, rcode */
	u8 variable[];
	
or so?

> +	target_rcode = ieee80211_mesh_hwmp_perr_get_rcode(perr_elem, 0);

but evidently, this doesn't really matter right now if only one
destination entry is ever read.

Still, please fix that, if only to avoid the compiler warnings I'm
imagining will happen.

johannes