Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Record route for ICMP messages

[Marek Lindner <lindner_marek@yahoo.de>]

On Saturday 13 February 2010 02:35:19 Daniel Seither wrote:
> The standard layer 3 ping utility can use the record route option of IP
> to collect route data for sent ping messages (ping -R). This patch
> introduces comparable functionality for batman-adv ICMP messages.
> 
> The patch modifies the batman ICMP packet format such that up to 17 MAC
> addresses can be recorded (sufficient for up to 8 hops per direction).
> batctl is extended to recognize the -R option for the ping subcommand.
> The output should be the same as for the standard iputils ping program.
> For this, the destination host is printed two times.

This is a very cool patch! I know quite some people will be happy to see this 
functionality. :-)


> This patch could be improved by dynamically growing the packet when a
> MAC address is to be added to the recorded route instead of statically
> allocating a buffer of fixed length.

I don't think it will be necessary to change the packet size dynamically. The 
gain will be rather small compared to the overhead it creates. However, it 
would make sense to specify 2 different icmp structs and only send the large 
packet when RR is really needed. 

For example:
struct icmp_packet {
	uint8_t  packet_type;
	uint8_t  version;  /* batman version field */
	uint8_t  msg_type; /* see ICMP message types above */
	uint8_t  ttl;
	uint8_t  dst[6];
	uint8_t  orig[6];
	uint16_t seqno;
	uint8_t  uid;
} __attribute__((packed));

struct icmp_packet_rr {
	uint8_t  packet_type;
	uint8_t  version;  /* batman version field */
	uint8_t  msg_type; /* see ICMP message types above */
	uint8_t  ttl;
	uint8_t  dst[6];
	uint8_t  orig[6];
	uint16_t seqno;
	uint8_t  uid;
        uint8_t  rr_cur;
	uint8_t  rr[BAT_RR_LEN];
} __attribute__((packed));

What do you think?


> +	/* add record route information if not full */
> +	if (icmp_packet->rr_cur && icmp_packet->rr_cur < BAT_RR_LEN / ETH_ALEN) {
> +		memcpy(&(icmp_packet->rr[icmp_packet->rr_cur * ETH_ALEN]),
> ethhdr->h_dest, ETH_ALEN);
> +		icmp_packet->rr_cur++;
> +	}

It would be better to check for the actual packet size rather than the 
BAT_RR_LEN define. Some node might have sent us an icmp packet which had a 
different size and then we crash here (or worse).

Regards,
Marek