Re: [B.A.T.M.A.N.] [PATCH] batman-adv: generalize batman-adv icmp packet handling

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Tuesday 15 October 2013 21:20:23 Antonio Quartulli wrote:
> > Besides, if we make everything generic batadv_socket_packet->icmp_packet
> > should not be hard-coded to batadv_icmp_packet_rr but the largest
> > available
> > ICMP packet type ?
> 
> or we dynamically allocate a buffer of size 'len'? In this way we don't need
> to change icmp_packet each time (hopefully not so many but still..)  the
> "largest available ICMP packet type" changes.

Allocating a dynamic buffer does not solve the underlying issue. At some point 
we would want to check the packet size - either through sizeof(socket_packet-
>icmp_packet) or a macro or whatever. 
Take a look at batadv_max_header_len() for an idea how to address the matter.


> > Wouldn't it be better to dump unkown icmp types for us instead of copying
> > everything to user space ?
> 
> dump == drop ? in that case I agree. Delivering unknown packets to batctl
> may also be dangerous.

Yes, that is what I was talking about.


> > Same is true for batadv_socket_write(). We should use the icmp header and
> > not assume icmp echo.
> 
> do you mean using the ICMP header to understand what packet it is and then
> behave accordingly? Also in this case I agree with you :)

Yap.


> Moving to a packet type based "check" was also part of the original idea of
> this generalisation (if I remember correctly), but not really needed when
> looking at avoiding further compatibility breakage (this should be the
> reason why this "feature" is not part of this patch).

I agree - it is not strictly needed but would fit since the rest of the patch 
works into the same direction.

Cheers,
Marek
 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]