Andrew Lunn andrew@lunn.ch schrieb am 16.02.2011 07:08:11:
[Bild entfernt]
Re: [B.A.T.M.A.N.] [PATCH 02/13] batman-adv: Add explicit batman header structure
Andrew Lunn
an:
The list for a Better Approach To Mobile Ad-hoc Networking
16.02.2011 07:08
Kopie:
Linus L??ssing
On Tue, Feb 15, 2011 at 06:12:17PM +0100, Linus L??ssing wrote:
Just a minor style adjustment, to give people a hint which fields
should
not be reordered and need to be at the beginning of each packet with batman-adv's frame type.
Hi Linus
+struct batman_header {
- uint8_t packet_type;
- uint8_t version; /* batman version field */
- uint8_t ttl;
- uint8_t align;
+} __packed;
struct batman_packet {
- uint8_t packet_type;
- uint8_t version; /* batman version field */
- struct batman_header header;
- uint32_t seqno; uint8_t flags; /* 0x40: DIRECTLINK flag, 0x20 VIS_SERVER
flag... */
uint8_t tq;
- uint32_t seqno; uint8_t orig[6]; uint8_t prev_sender[6];
- uint8_t ttl; uint8_t num_hna; uint8_t gw_flags; /* flags related to gateway class */
- uint8_t align;
} __packed;
Two different ideas, both triggered by the align byte in header. This byte is a waste of space, it is not used, and it is probably not easy to find something which all packet types are going to need in the future.
Sure, anything that not all packet types have in common can't go inside of the batman_header. But are those three variables something we could agree on, being something that all packet types will need today and in the future? version + packet_type is something we'll always need I think, the TTL might be something to argue about, as packets that do not get routed would not necessarily need them (though it could be a "safety belt" in case of bugs, to really make sure packets only intended for one hop will not be routed).
- Did you try not having it. So long header is __packed, and all the
structures it is used in are __packed, i think the compiler will do the right thing. The downside is that alignment is not obvious. Most people will assume header is 4 bytes, not 3, and think the alignment of the packets is wrong.
- Did you consider using a union? It is a more invasive patch, but
the alignment is then clear. The downside is sizeof() no longer gives you the per packet type size, rather it gives you the size of the biggest member of the union. So this makes the change even more invasive and bug prone.
Hmm, no, actually haven't thought about that. So we'd actually put the batman packet types as a union inside of the batman-header structure, instead of putting the header structure at the front of each packet type. Don't know if I'd like that either, due to the reason you've mentioned.
I think i prefer 1), with appropriate comments to explain the alignment issue.
So I think I'd prefer the first option, that should work too :). Just didn't think that an extra Byte would matter that much, and had the readability / making the alignment a little more obvious in mind. (And who knows when we might need to increase the version number size already ;) )
Andrew
Cheers, Linus