Re: [lwip-users] Problem With dns.c Using 32-Bit Compilers

[Jonathan Larmour]

Alain M. wrote:
> > It would be really handy, if, somehow, there could be a sys_arch style
> > hook or macro for protocol fragment encoding/decoding that would default
> > to structure overlaying, but let you override it with byte-by-byte
> > copying to a compiler friendly data structure if you need to.
>
> I have seen a comm packet long time ago that used macros with pointers, 
> and I used that for some small things. It is 100% portable and 
> performance would be hw dependent. Probably in chips like ARM7/9 it 
> would be bad, but on PCs and Cortex that have byte access it would be 
> better, no idea about 16 bits DSP. Also remember that 99% of protocol 
> header variables are contained in 32 bit words so it will not be too bad.
>
> the example would be:
> struct dns_answer {
>   /* DNS answer record starts with either a domain name or a pointer
>      to a name already present somewhere in the packet. */
>   u16_t type;
>   u16_t class;
>   u32_t ttl;
>   u16_t len;
> } PACK_STRUCT_STRUCT;
>
> char dns_answer[10];
> #define dns_answer_type  (*((u16_t*)dns_answer))
> #define dns_answer_class (*((u16_t*)&dns_answer[2]))
> #define dns_answer_ttl   (*((u32_t*)&dns_answer[4]))
> #define dns_answer_len   (*((u16_t*)&dns_answer[8]))
>
> or with pointers:
> #define dns_answer_type(p)  (*((u16_t*)p))
> #define dns_answer_class(p) (*((u16_t*)(p+2)))
>
> or some variations... (or fixes, I may have it wrong)

Unfortunately, the above is still not portable. That may well cause a 
mis-aligned access if dns_answer is not already aligned, and being char 
that isn't guaranteed.

An alternative could be bitfields - they can be dodgy territory for some 
compilers though. The really portable solution is to bitwise OR together 
the relevant bytes (using an endian-dependent macro), hoping that an 
optimising compiler can eliminate most of it. I expect that optimisation 
is beyond many/most though. A compromise might be to define offsets of 
members, and SMEMCPY from the correct location. An optimising compiler can 
more easily eliminate that and convert to an assignment where it knows it 
can. The situation for those using poor compilers could be bad though - 
function calls every time; and they may have been unaffected by the 
portability issue in the first place.


Anyway, would anyone arguing for improvement in this thread care to try 
the untested attached patch and confirm it fixes things for them in this 
particular case without breaking anything?

Thanks,

Jifl
--
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["The best things in life aren't things."]------      Opinions==mine

Index: core/dns.c
===================================================================
RCS file: /sources/lwip/lwip/src/core/dns.c,v
retrieving revision 1.22
diff -u -5 -p -r1.22 dns.c
--- core/dns.c  26 Jan 2008 16:11:39 -0000      1.22
+++ core/dns.c  28 Aug 2008 22:26:28 -0000
@@ -137,10 +137,11 @@ struct dns_hdr {
 } PACK_STRUCT_STRUCT;
 PACK_STRUCT_END
 #ifdef PACK_STRUCT_USE_INCLUDES
 #  include "arch/epstruct.h"
 #endif
+#define SIZEOF_DNS_HDR 12
 
 #ifdef PACK_STRUCT_USE_INCLUDES
 #  include "arch/bpstruct.h"
 #endif
 PACK_STRUCT_BEGIN
@@ -153,10 +154,11 @@ struct dns_query {
 } PACK_STRUCT_STRUCT;
 PACK_STRUCT_END
 #ifdef PACK_STRUCT_USE_INCLUDES
 #  include "arch/epstruct.h"
 #endif
+#define SIZEOF_DNS_QUERY 4
 
 #ifdef PACK_STRUCT_USE_INCLUDES
 #  include "arch/bpstruct.h"
 #endif
 PACK_STRUCT_BEGIN
@@ -171,10 +173,11 @@ struct dns_answer {
 } PACK_STRUCT_STRUCT;
 PACK_STRUCT_END
 #ifdef PACK_STRUCT_USE_INCLUDES
 #  include "arch/epstruct.h"
 #endif
+#define SIZEOF_DNS_ANSWER 10
 
 /** DNS table entry */
 struct dns_table_entry {
   u8_t  state;
   u8_t  numdns;
@@ -413,21 +416,21 @@ dns_send(u8_t numdns, const char* name, 
               (u16_t)(numdns), name));
   LWIP_ASSERT("dns server out of array", numdns < DNS_MAX_SERVERS);
   LWIP_ASSERT("dns server has no IP address set", dns_servers[numdns].addr != 
0);
 
   /* if here, we have either a new query or a retry on a previous query to 
process */
-  p = pbuf_alloc(PBUF_TRANSPORT, sizeof(struct dns_hdr) + DNS_MAX_NAME_LENGTH +
-                 sizeof(struct dns_query), PBUF_RAM);
+  p = pbuf_alloc(PBUF_TRANSPORT, SIZEOF_DNS_HDR + DNS_MAX_NAME_LENGTH +
+                 SIZEOF_DNS_QUERY, PBUF_RAM);
   if (p != NULL) {
     LWIP_ASSERT("pbuf must be in one piece", p->next == NULL);
     /* fill dns header */
     hdr = (struct dns_hdr*)p->payload;
-    memset(hdr, 0, sizeof(struct dns_hdr));
+    memset(hdr, 0, SIZEOF_DNS_HDR);
     hdr->id = htons(id);
     hdr->flags1 = DNS_FLAG1_RD;
     hdr->numquestions = htons(1);
-    query = (char*)hdr + sizeof(struct dns_hdr);
+    query = (char*)hdr + SIZEOF_DNS_HDR;
     pHostname = name;
     --pHostname;
 
     /* convert hostname into suitable query format. */
     do {
@@ -444,14 +447,14 @@ dns_send(u8_t numdns, const char* name, 
     *query++='\0';
 
     /* fill dns query */
     qry.type  = htons(DNS_RRTYPE_A);
     qry.class = htons(DNS_RRCLASS_IN);
-    MEMCPY( query, &qry, sizeof(struct dns_query));
+    MEMCPY( query, &qry, SIZEOF_DNS_QUERY);
 
     /* resize pbuf to the exact dns query */
-    pbuf_realloc(p, (query + sizeof(struct dns_query)) - 
((char*)(p->payload)));
+    pbuf_realloc(p, (query + SIZEOF_DNS_QUERY) - ((char*)(p->payload)));
 
     /* connect to the server for faster receiving */
     udp_connect(dns_pcb, &dns_servers[numdns], DNS_SERVER_PORT);
     /* send dns packet */
     err = udp_sendto(dns_pcb, p, &dns_servers[numdns], DNS_SERVER_PORT);
@@ -589,11 +592,11 @@ dns_recv(void *arg, struct udp_pcb *pcb,
     /* free pbuf and return */
     goto memerr1;
   }
 
   /* is the dns message big enough ? */
-  if (p->tot_len < (sizeof(struct dns_hdr) + sizeof(struct dns_query) + 
sizeof(struct dns_answer))) {
+  if (p->tot_len < (SIZEOF_DNS_HDR + SIZEOF_DNS_QUERY + SIZEOF_DNS_ANSWER)) {
     LWIP_DEBUGF(DNS_DEBUG, ("dns_recv: pbuf too small\n"));
     /* free pbuf and return */
     goto memerr1;
   }
 
@@ -630,45 +633,45 @@ dns_recv(void *arg, struct udp_pcb *pcb,
           goto responseerr;
         }
 
 #if DNS_DOES_NAME_CHECK
         /* Check if the name in the "question" part match with the name in the 
entry. */
-        if (dns_compare_name((unsigned char *)(pEntry->name), (unsigned char 
*)dns_payload + sizeof(struct dns_hdr)) != 0) {
+        if (dns_compare_name((unsigned char *)(pEntry->name), (unsigned char 
*)dns_payload + SIZEOF_DNS_HDR) != 0) {
           LWIP_DEBUGF(DNS_DEBUG, ("dns_recv: \"%s\": response not match to 
query\n", pEntry->name));
           /* call callback to indicate error, clean up memory and return */
           goto responseerr;
         }
 #endif /* DNS_DOES_NAME_CHECK */
 
         /* Skip the name in the "question" part */
-        pHostname = (char *) dns_parse_name((unsigned char *)dns_payload + 
sizeof(struct dns_hdr)) + sizeof(struct dns_query);
+        pHostname = (char *) dns_parse_name((unsigned char *)dns_payload + 
SIZEOF_DNS_HDR) + SIZEOF_DNS_QUERY;
 
         while(nanswers > 0) {
           /* skip answer resource record's host name */
           pHostname = (char *) dns_parse_name((unsigned char *)pHostname);
 
           /* Check for IP address type and Internet class. Others are 
discarded. */
-          MEMCPY(&ans, pHostname, sizeof(struct dns_answer));
+          MEMCPY(&ans, pHostname, SIZEOF_DNS_ANSWER);
           if((ntohs(ans.type) == DNS_RRTYPE_A) && (ntohs(ans.class) == 
DNS_RRCLASS_IN) && (ntohs(ans.len) == sizeof(struct ip_addr)) ) {
             /* read the answer resource record's TTL, and maximize it if 
needed */
             pEntry->ttl = ntohl(ans.ttl);
             if (pEntry->ttl > DNS_MAX_TTL) {
               pEntry->ttl = DNS_MAX_TTL;
             }
             /* read the IP address after answer resource record's header */
-            MEMCPY( &(pEntry->ipaddr), (pHostname+sizeof(struct dns_answer)), 
sizeof(struct ip_addr));
+            MEMCPY( &(pEntry->ipaddr), (pHostname+SIZEOF_DNS_ANSWER), 
sizeof(struct ip_addr));
             LWIP_DEBUGF(DNS_DEBUG, ("dns_recv: \"%s\": response = ", 
pEntry->name));
             ip_addr_debug_print(DNS_DEBUG, (&(pEntry->ipaddr)));
             LWIP_DEBUGF(DNS_DEBUG, ("\n"));
             /* call specified callback function if provided */
             if (pEntry->found) {
               (*pEntry->found)(pEntry->name, &pEntry->ipaddr, pEntry->arg);
             }
             /* deallocate memory and return */
             goto memerr2;
           } else {
-            pHostname = pHostname + sizeof(struct dns_answer) + htons(ans.len);
+            pHostname = pHostname + SIZEOF_DNS_ANSWER + htons(ans.len);
           }
           --nanswers;
         }
         LWIP_DEBUGF(DNS_DEBUG, ("dns_recv: \"%s\": error in response\n", 
pEntry->name));
         /* call callback to indicate error, clean up memory and return */