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

Antonio Quartulli
2013-09-17T15:38:54Z

[-- 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 :)