Re: [PATCH 13/16] batman-adv: add build check macros for packet member offset

[Antonio Quartulli <antonio-x4xJYDvStAgysxA8WJXlww@public.gmane.org>]

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

On Thu, Oct 10, 2013 at 09:37:32AM +0100, David Laight wrote:
> > Since we removed the __packed from most of the packets, we should
> > make sure that the offset generated by the compiler are correct for
> > sent/received data.
> ...
> > +	/* compile time checks for struct member offsets */
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_4addr_packet, src) != 10);
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_packet, dest) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_frag_packet, dest) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_tvlv_packet, dst) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_icmp_packet, dst) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_icmp_packet_rr, dst) != 4);
> 
> It is usually enough to check the size of the structures.

What if two fields are inverted by mistake in a way that the
size of the struct remains the same? The size check would not complain but the
code would not work anymore.

We use a "generic" struct to access the initial part of any packet.
Therefore these checks are to ensure that the information we are going to access
is really placed at that offset, whatever packet we have.
It was not possible to use a common inner struct and so we relied on this test
to be safe.

> Which is also best done in the .h file so it is validated
> in all the compilation environments that might be used.
> 

This does not really hurt at the moment because we placed them in main.c which
is a file that is always compiled. But thanks for the suggestion: putting them
in the .h file helps to remind the developer to add a new BUILD_ON_BUG when
creating a new packet type.


Thanks a lot.

Regards,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]