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

[Marco Dalla Torre <marco.dallato@gmail.com>]

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