Message ID | 1318454600-18349-1-git-send-email-bernhard.kaindl@gmx.net |
---|---|
State | Changes Requested |
Headers | show |
On Wednesday 12 October 2011 17:23:20 Bernhard Kaindl wrote: > 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! 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: >> >> 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 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. I'd prefer the already sent patch. Best Regards, Bernhard
On Friday 14 October 2011 03:44:13 Bernhard Kaindl wrote: > Am 12.10.2011 23:48, schrieb Mike Frysinger: > >> 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/ > > 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. sounds like you want get_unaligned_be16() -mike
diff --git a/net/dns.c b/net/dns.c index b51d1bd..7034183 100644 --- a/net/dns.c +++ b/net/dns.c @@ -28,6 +28,9 @@ #include "dns.h" +/* p is a char *, we shift the MSB up in our CPU endianness -> no ntohs used! */ +#define netbytes2hs(p) ((p)[0] << 8 | (p)[1]) + char *NetDNSResolve; /* The host to resolve */ char *NetDNSenvvar; /* The envvar to store the answer in */ @@ -109,7 +112,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 +122,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 +141,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 || netbytes2hs(p+1) != DNS_A_RECORD) { + puts("DNS: response not an A record\n"); NetState = NETLOOP_SUCCESS; return; } @@ -160,14 +161,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 = netbytes2hs(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 = netbytes2hs(p+10); debug("dlen = %d\n", dlen); p += 12 + dlen; } else if (type == DNS_A_RECORD) { @@ -181,8 +180,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 = netbytes2hs(p+10); p += 12; memcpy(&IPAddress, p, 4);