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

Marco Dalla Torre
2013-09-17T14:01:02Z

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)?

>> >+
>> > #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

>
>> >+ 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...

>> >+{
>> >+ LEN_CHECK((size_t)buff_len - header_len,
>> >+ sizeof(struct tcphdr), "TCP");
>> >+ struct tcphdr *tcphdr;
> put all the declarations first and then perform the LEN_CHECK, like int he other
> functions.
>

Ok, it just didn't feel right to me to declare something even if you
might even get to use it, but I can understand that from a style
conformity point of view.

[...]

>> >+
>> >+ udphdr = (struct udphdr *)(packet_buff + header_len);
>> >+ printf("IP %s.%i > ",
>> >+ src_addr,
>> >+ ntohs(udphdr->source));
> does not fit on one line?

Yeah, it does now with the temporary variable, but when I did format it
before introducing *udphdr was still much longer for one line... oh
nevermind I'll fix it of course :)

>> >+
>> >+ switch (ntohs(udphdr->dest)) {
>> >+ case 67:
>> >+ LEN_CHECK(
>> >+ (size_t)buff_len - header_len - sizeof(struct udphdr),
>> >+ (size_t) 44,
>> >+ "DHCP");
> same alignment problem as before. If one argument is too long, feel free to use
> a temporary variable.

Ack.

[...]

>> >+ switch (icmphdr->icmp6_code) {
>> >+ case ICMP6_DST_UNREACH_NOPORT:
>> >+ tmp_iphdr = (struct ip6_hdr *)(((char *)icmphdr)
>> >+ + sizeof(struct icmp6_hdr));
>> >+
>> >+ tmp_udphdr = (struct udphdr *)
>> >+ ((char *)tmp_iphdr)
> there is no need for the parenthesis around the cast. and I'd suggest to cast to
> uint8_t or unsigned char. That is the 'recommended' type for this kind of pointer
> operations.

I'll keep it in mind, but I hope you don't mind if I sometimes happen to
use them to clear things up for my poor C-untrained eye :-D

>> >+ case ICMP6_ECHO_REQUEST:
>> >+ inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
>> >+ 40);
>> >+ printf("%s: ICMPv6 echo request, id: %d, seq: %d, length: %hu\n",
>> >+ nd_ipv6_addr, icmphdr->icmp6_id,
>> >+ icmphdr->icmp6_seq, iphdr->ip6_plen);
>> >+ break;
>> >+ case ICMP6_ECHO_REPLY:
>> >+ inet_ntop(AF_INET6, &(iphdr->ip6_dst),
>> >+ nd_ipv6_addr, 40);
>> >+ printf("%s: ICMPv6 echo reply, id: %d, seq: %d, length: %hu\n",
>> >+ nd_ipv6_addr, icmphdr->icmp6_id,
>> >+ icmphdr->icmp6_seq, iphdr->ip6_plen);
>> >+ break;
>> >+ case ICMP6_TIME_EXCEEDED:
>> >+ inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
>> >+ 40);
>> >+ printf("%s: ICMPv6 time exceeded in-transit, length %zu\n",
>> >+ nd_ipv6_addr,
>> >+ (size_t)buff_len - sizeof(struct icmp6_hdr));
>> >+ break;
>> >+ case ND_NEIGHBOR_SOLICIT:
>> >+ nd_neigh_sol = (struct nd_neighbor_solicit *)icmphdr;
>> >+
>> >+ inet_ntop(AF_INET6, &(nd_neigh_sol->nd_ns_target),
>> >+ nd_ipv6_addr, 40);
>> >+ printf("NEIGHBOR SOLICITATION, Request who-has %s",
>> >+ nd_ipv6_addr);
> Does (the real) tcpdump print "who-has" also in this case? I have not checked,
> but we try to reproduce the same output to avoid confusion and make it easier
> to read.
>

I wasn't aware of this "output equality" goal, I'll look into it.

>> >+
>> >+ inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr,
>> >+ 40);
>> >+ printf(" tell %s, length %zd\n", nd_ipv6_addr,
>> >+ buff_len);
>> >+ break;
>> >+ case ND_NEIGHBOR_ADVERT:
>> >+ nd_advert = (struct nd_neighbor_advert *)icmphdr;
>> >+ inet_ntop(AF_INET6, &(nd_advert->nd_na_target),
>> >+ nd_na_target, 40);
>> >+ inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
>> >+ 40);
>> >+ printf("NEIGHBOR ADVERTISEMENT, Reply to %s for target %s, length %zd\n",
> As above, check what tcpdump prints. I don't think it says "Reply" since there
> is no ARP Request/Reply in this case.

Yes, there is no ARP but the semantics in this case are pretty much the
same. But as before, I'll look into it.

>> > switch (iphdr->protocol) {
>> > case IPPROTO_ICMP:
>> >- LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct icmphdr), "ICMP");
>> >+ LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4),
>> >+ sizeof(struct icmphdr), "ICMP");
> This change is unrelated to this patch.

For this and the others unrelated CHANGES: ok, didn't feel wrong to fix
some offending formatting, but I'll revert those changes.
By the way: now I understand you! Fixing bad code formatting is like
drugs! :D

>
> Cheers,
>
> -- Antonio Quartulli
>

Thanks,
Marco