net/dns.c: Fix broken endian handling in dns command

The U-Boot dns command only worked in little-endian CPUs so far because it was based on an antique version of the TADNS source which was using a broken macro to read the shorts found in DNS reply messages by shifting the LSB from the message into the CPU's MSB of a short int and the MSB from the stream into the LSB part of the CPU's short int. So far, so twisted.

To correct the twisted bytes, the code used ntohs() as a byte-swapping function to swap the MSB back where it belongs and vice versa.

This works fine, except that ntohs() naturally does nothing on big-endian CPUs.

So on big-endian CPUs, the MSB from the message stayed in the LSB of the CPU and vice versa.

Ditch this brain-deadness by just shifting the MSB from the network byte stream of the reply message into the right (MSB) location of a short int and putting the LSB from the network byte stream as the lower byte of it, and we are done with reading the short from the network stream for both endianesses, no ntohs() or such!

Quoted 6 lines

Ditch this brain-deadness by just shifting the MSB from the network byte stream of the reply message into the right (MSB) location of a short int and putting the LSB from the network byte stream as the lower byte of it, and we are done with reading the short from the network stream for both endianesses, no ntohs() or such!

please use a standard macro instead of inventing yet another. we've got the rich Linux api which should cover every case you could possibly need.

	cpu_to_{l,b}e{16,32,64}(...)
	{l,b}e_to_cpu{16,32,63}(...)

see include/linux/byteorder/
-mike

Am 12.10.2011 23:48, schrieb Mike Frysinger:

Quoted 6 lines

Quoted 5 lines

Ditch this brain-deadness by just shifting the MSB from the network byte stream of the reply message into the right (MSB) location of a short int and putting the LSB from the network byte stream as the lower byte of it, and we are done with reading the short from the network stream for both endianesses, no ntohs() or such!

Quoted 8 lines

please use a standard macro instead of inventing yet another. we've got the rich Linux api which should cover every case you could possibly need.

	cpu_to_{l,b}e{16,32,64}(...)
	{l,b}e_to_cpu{16,32,63}(...)

see include/linux/byteorder/

No, of course I looked, but didn't find what is needed.

These are just conditional byte swaps in 16,32 or 64 bit, not more.

The code needs is a conversion from an uneven "const char *" to u16!

It ___is___ possible to use be_to_cpu16, but it needs ugly casting:

         if (&p[5] > e || be_to_cpu16(*(u16 *)(p+1)) != DNS_A_RECORD) {

To not be distracted by reading the ugly casting, one would have to put this into a macro again, but if you really like to see be_to_cpu16 so much, I can change the macro to:

#define netstring_to_cpu_short(p) be16_to_cpu(*(u16 *)(p))

This result doesn't make it more readable than what I already posted.

From: Bernhard Kaindl <[email protected]>

net/dns.c used endian conversion macros wrongly (shorts in reply were put swapped into CPU, and then ntohs() was used to swap it back, which broke on big-endian).

Fix this by using the correct linux conversion macro for reading a unaligned short in network byte order: get_unaligned_be16() Thanks to Mike Frysinger pointing at the best macro to use.

Tested on big and little endian qemu boards (mips and versatile)

Signed-off-by Bernhard Kaindl <[email protected]>

Cc Pieter Voorthuijsen <[email protected]>

Cc Robin Getz <[email protected]>

---
 net/dns.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

net/dns.c +8 -12

index b51d1bd..7a3f1f9 100644
@@ -25,6 +25,7 @@
 #include <common.h>
 #include <command.h>
 #include <net.h>
+#include <asm/unaligned.h>
 
 #include "dns.h"
 

@@ -109,7 +110,6 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 	int found, stop, dlen;
 	char IPStr[22];
 	IPaddr_t IPAddress;
-	short tmp;
 
 
 	debug("%s\n", __func__);

@@ -120,14 +120,14 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		debug("0x%p - 0x%.2x  0x%.2x  0x%.2x  0x%.2x\n",
 			pkt+i, pkt[i], pkt[i+1], pkt[i+2], pkt[i+3]);
 
-	/* We sent 1 query. We want to see more that 1 answer. */
+	/* We sent one query. We want to have a single answer: */
 	header = (struct header *) pkt;
 	if (ntohs(header->nqueries) != 1)
 		return;
 
 	/* Received 0 answers */
 	if (header->nanswers == 0) {
-		puts("DNS server returned no answers\n");
+		puts("DNS: host not found\n");
 		NetState = NETLOOP_SUCCESS;
 		return;
 	}

@@ -139,9 +139,8 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		continue;
 
 	/* We sent query class 1, query type 1 */
-	tmp = p[1] | (p[2] << 8);
-	if (&p[5] > e || ntohs(tmp) != DNS_A_RECORD) {
-		puts("DNS response was not A record\n");
+	if (&p[5] > e || get_unaligned_be16(p+1) != DNS_A_RECORD) {
+		puts("DNS: response was not an A record\n");
 		NetState = NETLOOP_SUCCESS;
 		return;
 	}

@@ -160,14 +159,12 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		}
 		debug("Name (Offset in header): %d\n", p[1]);
 
-		tmp = p[2] | (p[3] << 8);
-		type = ntohs(tmp);
+		type = get_unaligned_be16(p+2);
 		debug("type = %d\n", type);
 		if (type == DNS_CNAME_RECORD) {
 			/* CNAME answer. shift to the next section */
 			debug("Found canonical name\n");
-			tmp = p[10] | (p[11] << 8);
-			dlen = ntohs(tmp);
+			dlen = get_unaligned_be16(p+10);
 			debug("dlen = %d\n", dlen);
 			p += 12 + dlen;
 		} else if (type == DNS_A_RECORD) {

@@ -181,8 +178,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 
 	if (found && &p[12] < e) {
 
-		tmp = p[10] | (p[11] << 8);
-		dlen = ntohs(tmp);
+		dlen = get_unaligned_be16(p+10);
 		p += 12;
 		memcpy(&IPAddress, p, 4);
 

From: Bernhard Kaindl <[email protected]>

net/dns.c used endian conversion macros wrongly (shorts in reply were put swapped into CPU, and then ntohs() was used to swap it back, which broke on big-endian).

Fix this by using the correct linux conversion macro for reading a unaligned short in network byte order: get_unaligned_be16() Thanks to Mike Frysinger pointing at the best macro to use.

Tested on big and little endian qemu boards (mips and versatile)

Signed-off-by Bernhard Kaindl <[email protected]>

Cc Pieter Voorthuijsen <[email protected]>

Cc Robin Getz <[email protected]>

---
 net/dns.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)