Re: [B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Antonio Quartulli <antonio@meshcoding.com>]

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

On Tue, Sep 17, 2013 at 04:01:02PM +0200, Marco Dalla Torre wrote:
> Hi,
> 
> On 09/17/13 14:31, Antonio Quartulli wrote:
> > On Tue, Sep 17, 2013 at 02:08:14PM +0200, Marco Dalla Torre wrote:
> >> >@@ -47,6 +49,8 @@
> >> >  #define ETH_P_BATMAN	0x4305
> >> >  #endif /* ETH_P_BATMAN */
> >> >
> >> >+#define IPV6_MIN_MTU	1280
> > I have it in /usr/include/linux/ipv6.h don't you?
> > If it has been introduced in a recent version of the headers, please add an
> > #ifndef to avoid defining the symbol twice.
> >
> 
> I'm aware of ipv6.h, in fact this is exactly where the definition I used 
> comes from. Unfortunately including it in tcpdump.c generates all sort 
> of namespace clashes with the other include statements, and I felt that 
> doing namespace housecleaning was beyond the scope of the patch...
> I'm not sure of what you mean with the #ifndef, since that statement is 
> already present in the file.
> Can you suggest a solution (or explain better what you meant if that one 
> is the solution)?

oh ok.

my ideas was to do something like:

#ifndef IPV6_MIN_MTU
#define IPV6_MIN_MTU     1280


but given your problem I think there is something else we should care about...I
will check into this namespace issue.

> 
> >> >+
> >> >  #define LEN_CHECK(buff_len, check_len, desc) \
> >> >  if ((size_t)(buff_len) < (check_len)) { \
> >> >  	fprintf(stderr, "Warning - dropping received %s packet as it is smaller than expected (%zu): %zu\n", \
> >> >@@ -188,11 +192,210 @@ static void dump_arp(unsigned char *packet_buff, ssize_t buff_len,
> >> >  	}
> >> >  }
> >> >
> >> >+static void parse_tcp(
> > Why don't you call this function dump_tcp like all the other dump_* ?
> 
> Because, I wasn't aware of this rule. Now I know! :P

They are all over in the same file :-P

> 
> >
> >> >+	unsigned char *packet_buff,
> >> >+	ssize_t buff_len,
> >> >+	size_t header_len,
> >> >+	char *src_addr,
> >> >+	char *dst_addr)
> > Did we discuss this "freestyle" alignment in the last RFC?
> >
> 
> For this and all the other styling problems like that: yes sorry when I 
> wrote this patch some time ago I wasn't aware of these formatting rules. 
> And I clearly failed correcting all while reviewing it...

I guessed so :)

Cheers,

-- 
Antonio Quartulli

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