tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7017, Implement DNS client

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #7017: Implement DNS client

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Tue 19 Jun 2007 12:37:35 PM UTC
   
 
Category:  None Should Start On:  Mon 18 Jun 2007 10:00:00 PM UTC
Should be Finished on:  Tue 18 Dec 2007 05:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  jim_pettinato Percent Complete:  100%
Open/Closed:  Closed Planned Release:  1.3.0
Effort:  0.00

Jump to the original submission

Fri 14 Dec 2007 09:03:50 PM UTC, comment #100: 

I note some possible improvements for a future task:

- return multiple ip addresses
- return other RR types (MX, SRV...)
- implement a "dynamic dns update" message

Jim, no objections to close this task ?

Frédéric Bernon <fbernon>
Group Member
Thu 13 Dec 2007 11:07:53 PM UTC, comment #99: 

Done, but could need more tests (but I think it's good).


Frédéric Bernon <fbernon>
Group Member
Thu 13 Dec 2007 07:11:38 AM UTC, comment #98: 

Fine for me!

Simon Goldschmidt <goldsimon>
Group administrator
Wed 12 Dec 2007 11:11:44 PM UTC, comment #97: 

DNS_ERR_MEM => ERR_MEM
DNS_QUERY_INVALID => ERR_VAL
DNS_QUERY_QUEUED => ERR_INPROGRESS
DNS_COMPLETE => ERR_OK

ERR_INPROGRESS or any other name which could be reuse in another module...

Frédéric Bernon <fbernon>
Group Member
Wed 12 Dec 2007 07:24:41 PM UTC, comment #96: 

I moved DNS_MSG_SIZE in opt.h, and add a comment, like jifl suggests it.

Frédéric Bernon <fbernon>
Group Member
Wed 12 Dec 2007 06:32:47 PM UTC, comment #95: 


> In fact, it more for RFC compliance. Of course, it could be see
> like an option, but I don't think it is.


Well, as with many other lightweight aspects of lwIP, full RFC compliance is a goal rather than compulsory, even if it affects interoperability - and often even then (turning off IP fragmentation/reassembly for example).

512 is the maximum size a server can send, but on a private network, or for some applications, people will know they won't ever be receiving that much. Or they'd rather have DNS that worked virtually all the time, than DNS which can't work because it doesn't fit :).

512 bytes is a lot for people like me who are using lwIP on targets with 32K RAM. (Of course I can modify my sources, but I'm assuming I won't be the only one).

Jonathan Larmour <jifl>
Group Member
Wed 12 Dec 2007 06:03:45 PM UTC, comment #94: 


>Each of the dns.h/dns.c ones surrounded by #ifndefs presumably?


yes

>I think DNS_MSG_SIZE should go in opt.h as well as it has a direct effect on memory use.


In fact, it more for RFC compliance. Of course, it could be see like an option, but I don't think it is.

>Aside from that, I just remember we wanted to get rid of the enum dns_result and change dns_gethostbyname to returning err_t, adding an err_t define for DNS_COMPLETE??


yes, no, could be better. I will read what we said in previous comments (never than 100 here !!!)

For the options, I move them like this :

opt.h:
DNS_TABLE_SIZE (ram usage)
DNS_MAX_NAME_LENGTH (ram usage)
DNS_MAX_SERVERS (ram usage)
DNS_DOES_NAME_CHECK (behavior)
DNS_USES_STATIC_BUF (ram usage)

dns.h:
DNS_TMR_INTERVAL (tcpip_thread used, but it's not configurable)
DNS_RRxxx (for avoid to move them again in future uses)

dns.c:
DNS_SERVER_ADDRESS (default value, most of time, replace by user value)
DNS_SERVER_PORT (should be rare to change it)
DNS_MAX_RETRIES (I didn't know, so, I leave it here)
DNS_MAX_TTL (should be rare to change it, RFC compliant)
DNS_MSG_SIZE (it's not really an option, but more a RFC constant)

Of course, feel free to tell me if I'm wrong.



Frédéric Bernon <fbernon>
Group Member
Tue 11 Dec 2007 10:19:27 PM UTC, comment #93: 

I think DNS_MSG_SIZE should go in opt.h as well as it has a direct effect on memory use.

Each of the dns.h/dns.c ones surrounded by #ifndefs presumably?

Jonathan Larmour <jifl>
Group Member
Tue 11 Dec 2007 08:03:54 PM UTC, comment #92: 

My suggestions:
DNS_MAX_SERVERS -> opt.h
DNS_MAX_TTL -> dns.c
DNS_TMR_INTERVAL -> leave in dns.h
DNS_MSG_SIZE -> dns.c
"Resource Records" defines -> dns.c (or leave in dns.h if you need them externally)


Aside from that, I just remember we wanted to get rid of the enum dns_result and change dns_gethostbyname to returning err_t, adding an err_t define for DNS_COMPLETE??

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Dec 2007 07:58:26 PM UTC, comment #91: 

Yes, it's done. The last thing to do is to choose where to place DNS options (see comment #61).

Frédéric Bernon <fbernon>
Group Member
Tue 11 Dec 2007 07:47:46 PM UTC, comment #90: 

Re comment #76:

is this task done now that the "per-thread/static" option is included?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 05 Dec 2007 12:16:44 AM UTC, comment #89: 

The patch looks fine to me.


> Having something like a "hosts" file could be interresting, but
> this is the "hardcoded" side of your patch which cause me a
> "problem". A "dynamic" solution should be better (to let the
> user to complete the hosts table).


That is true, although I thought this may be a simple way to do something nearly as good.

It would not be difficult to change the LWIP_HOSTS_LOOKUP define to instead be LWIP_HOSTS_MAP_SIZE, to give the number of table entries. Add a couple of simple functions to add/remove and that would be it.

But you are right to leave this for after 1.3. I will create a new task.


Jonathan Larmour <jifl>
Group Member
Tue 04 Dec 2007 10:35:45 PM UTC, comment #88: 

Here the patch for static or per-thread variables set for "gethostbyname". Is it ok for you?

Except that, I think that other suggestions can be delayed to post-1.3.0 (MX and other types/class request, static host table...). So, the last point which options to move in opt.h (or at another place). But it's just my point of view.


(file #14560)

Frédéric Bernon <fbernon>
Group Member
Tue 04 Dec 2007 10:13:53 PM UTC, comment #87: 


>It's analogous to /etc/hosts on Unix (or the 'hosts' file on windows, although I can never remember where it lives).


yes, of course, I understood

>It can be used for fixed addresses that won't change and are important (some sort of main server). This can be a) to save delay and network traffic by having to look them up repeatedly; but more importantly, b) to reduce the chance of problems when there are temporary DNS lookup failures, at least for contacting these important servers.


Having something like a "hosts" file could be interresting, but this is the "hardcoded" side of your patch which cause me a "problem". A "dynamic" solution should be better (to let the user to complete the hosts table).


Frédéric Bernon <fbernon>
Group Member
Tue 04 Dec 2007 12:09:39 PM UTC, comment #86: 

Re LWIP_MAKE_ADDRWORD - a helper macro would be useful and easy to provide. From David's comment, it looks like it would want to be:

#define LWIP_MAKE_ADDRWORD(a,b,c,d) htonl(((u32_t)a<<24)|((u32_t)b<<16)|(c<<8)|(d<<0))

Frederic wrote:

> To be honest, except "localhost", I don't see any real cases
> where a hardcoded name could be useful. Can you tell me some
> samples for such uses?


It's analogous to /etc/hosts on Unix (or the 'hosts' file on windows, although I can never remember where it lives).

It can be used for fixed addresses that won't change and are important (some sort of main server). This can be a) to save delay and network traffic by having to look them up repeatedly; but more importantly, b) to reduce the chance of problems when there are temporary DNS lookup failures, at least for contacting these important servers.

The other use is to allow use of friendly names for something that won't ever be (or won't be guaranteed to be) in the DNS server being used. For example a private name, but in general you want to use a normal DNS server which only resolves public addresses.

Jonathan Larmour <jifl>
Group Member
Mon 03 Dec 2007 09:19:39 PM UTC, comment #85: 

Just a comment regarding the proposed LWIP_MAKE_ADDRWORD macro or equivalent expression: it would be handy if this was written in a way which allows it to be used on a compiler for a 16-bit processor without having to modify core LWIP source code.

If sizeof(int) == 2 then the << 16 and << 24 operations will produce a zero result. It is necessary to force a 32-bit shift operation, e.g. by typecasting the first and second elements to a 32-bit unsigned integer type before doing the shift.

This may be an issue that should be left for people doing their own ports, but the C standard only requires sizeof(int) to be at least 16 bits, and I'd argue that LWIP should be written to be compliant with standard C rather than assuming the now more common environment with 32-bit integers.

David Empson <dempson>
Mon 03 Dec 2007 06:08:30 PM UTC, comment #84: 

To be honest, except "localhost", I don't see any real cases where a hardcoded name could be useful. Can you tell me some samples for such uses?

Frédéric Bernon <fbernon>
Group Member
Mon 03 Dec 2007 04:55:17 PM UTC, comment #83: 

I like that thought! Maybe we could add a macro like

#define LWIP_MAKE_ADDRWORD(a,b,c,d) htonl((a<<24)|(b<<16)|(c<<8)|(d<<0))

or even

#define LWIP_HOST_MAP_ENTRY(name, a,b,c,d) { name, LWIP_MAKE_ADDRWORD(a,b,c,d) }

so that the code the user has to change (#define LWIP_HOSTS_MAP) gets more readable?

Simon Goldschmidt <goldsimon>
Group administrator
Sun 02 Dec 2007 10:35:18 PM UTC, comment #82: 

Strictly it's incorrect: the name localhost is not special, and the configured DNS server should map it to 127.0.0.1 anyway. In practice it makes sense though :).

But once we consider this, it doesn't take much to take one little extra step to provide a general hard-coded hosts lookup functionality, configured by the user. For example in the following, the user can enable LWIP_HOSTS_LOOKUP in lwipopts.h, and then define their own host mappings, e.g.:

#define LWIP_HOSTS_MAP \
  { "server1", htonl((192<<24)|(168<<16)|(1<<8)|(1<<0)) }, \
  { "server2", htonl((192<<24)|(168<<16)|(1<<8)|(2<<0)) }

And here's the code for dns.c:

#if LWIP_HOSTS_LOOKUP
struct hosts_map_s {
  const char *name;
  u32_t addr;
};

struct hosts_map_s hosts_map[] = {
#if LWIP_HAVE_LOOPIF
   { "localhost", INADDR_LOOPBACK }
#endif
#ifdef LWIP_HOSTS_MAP
   ,
   LWIP_HOSTS_MAP
#endif
};

In dns_gethostbyname:

#if LWIP_HOSTS_LOOKUP
{
  u16_t i;
  for (i=0;i<(sizeof(hosts_map)/sizeof(hosts_map[0]));i++) {
    if (!strcmp(hostname, host_maps[i].name)) {
      addr->addr=hosts_maps[i].addr;
      return DNS_COMPLETE;
    }
  }
}
#endif


It's just a thought, take it or leave it.

Jonathan Larmour <jifl>
Group Member
Sat 01 Dec 2007 01:04:45 PM UTC, comment #81: 

That's OK I think. Even if someone changes the address of the loopif, localhost should translate to 127.0.0.1, shouldn't it?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 01 Dec 2007 10:27:17 AM UTC, comment #80: 

Are you agree to add in dns_gethostbyname this code :

#if LWIP_HAVE_LOOPIF
  if (strcmp(hostname,"localhost")==0) {
    addr->addr = INADDR_LOOPBACK;
    return DNS_COMPLETE;
  }
#endif /* LWIP_HAVE_LOOPIF */


Of course, it seems possible to loop_netif's ip address to something different than INADDR_LOOPBACK, but is it very legal? Comments?

Frédéric Bernon <fbernon>
Group Member
Fri 30 Nov 2007 03:24:41 PM UTC, comment #79: 

For what it's worth I agree with Frédéric.

Jonathan Larmour <jifl>
Group Member
Fri 30 Nov 2007 02:30:11 PM UTC, comment #78: 


>I would like to move the creation of the udp pcb dns uses from dns_init to a function called the first time dns_gethostbyname so the pcb isn't created until it is really used.


I have a real preference that all permanent services were created at the product startup, and not when at the time it's need. Since once it's created, it is not free, so, there is no reason to change the allocation place to my point of view. And if a user doesn't need it, he can always disable it with LWIP_DNS=0. Current behavior is more determinist.

Frédéric Bernon <fbernon>
Group Member
Fri 30 Nov 2007 02:17:23 PM UTC, comment #77: 

I would like to move the creation of the udp pcb dns uses from dns_init to a function called the first time dns_gethostbyname so the pcb isn't created until it is really used.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Nov 2007 09:20:07 AM UTC, comment #76: 

I hope (again) find time this weekend for a "per-thread/static" option.

Frédéric Bernon <fbernon>
Group Member
Thu 29 Nov 2007 07:53:23 AM UTC, comment #75: 

DNS seems pretty complete. Can we close this or is there something left?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 22 Nov 2007 06:51:15 PM UTC, comment #74: 

re comment #41:

> It seems there is the same problem with DHCP: there is no check
> that the full message fit in one pbuf. So, it's possible to have
> a memory overflow...


FYI: I just checked that (didn't read the DHCP code at all until now) and DHCP always uses mem_malloc to copy the message!

(There is some room for perfection though, as the code uses its own pbuf to memory copy routine...)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 22 Nov 2007 01:17:37 PM UTC, comment #73: 

I think you may as well keep the other RR type defines in, since I think being able to query those is important and we should do it. But I also agree it can wait until after 1.3.0. For example, I'm not sure how Frederic proposes it would work would be right - not every lookup should recurse all the way to an IP address.

Anyway, there doesn't seem much point removing something we know we'll need in the future.


Jonathan Larmour <jifl>
Group Member
Thu 22 Nov 2007 08:26:16 AM UTC, comment #72: 


>I think that would be a good change as we get rid of th DNS_RESULT enum.


Ok

>Hmm, shouldn't the user be able to override it, just like errno so he can use an external errno that is already implemented (e.g. in a c-library)?


If errno can already exists on the OS (used for file IO by example), this is not the same case for h_errno, which is, if I'm not wrong, "ip stack" specific (netdb.h).

>Finally, I don't really know where to put the options... But to query a MX or another type: can't that be solved differently? (I really don't know about DNS and how that works) but dns.h is kind of big like it is now... Anyway, if you need those defines just leave them there.


So, is there any other comments for that? Jim ?

Frédéric Bernon <fbernon>
Group Member
Thu 22 Nov 2007 08:09:38 AM UTC, comment #71: 


> just post it in a patch record?


I'll do  that. Maybe someone can use it...

> I thought to add a err_t values mainly for the "enqueued request" case.


I think that would be a good change as we get rid of th DNS_RESULT enum.

> Since we have LWIP_DNS option, we can also add "int h_errno;" at the beginning of the file, and use it?


Hmm, shouldn't the user be able to override it, just like errno so he can use an external errno that is already implemented (e.g. in a c-library)?

Finally, I don't really know where to put the options... But to query a MX or another type: can't that be solved differently? (I really don't know about DNS and how that works) but dns.h is kind of big like it is now... Anyway, if you need those defines just leave them there.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Nov 2007 07:41:46 PM UTC, comment #70: 


>I rather was concerned about adding code that noone needs/maintains..


So, to my point of view, this is not a need, but if this is code you don't want to maintain, so, perhaps just post it in a patch record?

>Do you mean to add more err_t values and include them in err_to_errno_table? I think that would be a good thing to do, but we'd have to talk about the return value of dns_gethostbyname: it would be ERR_OK when the result was cached, I guess (ERR_MEM/ERR_VAL/ERR_ARG is straightforward, also). But what would it be for an enqueued request? I think it should be ERR_OK also, since this is OK, but then we can't distinguish it from the other ERR_OK... any thoughts?


I thought to add a err_t values mainly for the "enqueued request" case.

>Honestly, I also read that (h_errno instead of errno), but since we don't have a h_errno, I just modified errno... Not really portable, but who cares for an exact error message ;-)


Since we have LWIP_DNS option, we can also add "int h_errno;"  at the beginning of the file, and use it?

Else, about comment #61 and options places?





Frédéric Bernon <fbernon>
Group Member
Wed 21 Nov 2007 06:32:01 PM UTC, comment #69: 


> As long the linker can drop the code if the application doesn't use it, it's not a problem to add it.


I rather was concerned about adding code that noone needs/maintains...

re comment #64:

Do you mean to add more err_t values and include them in err_to_errno_table? I think that would be a good thing to do, but we'd have to talk about the return value of dns_gethostbyname: it would be ERR_OK when the result was cached, I guess (ERR_MEM/ERR_VAL/ERR_ARG is straightforward, also). But what would it be for an enqueued request? I think it should be ERR_OK also, since this is OK, but then we can't distinguish it from the other ERR_OK... any thoughts?

re comment #65:

Honestly, I also read that (h_errno instead of errno), but since we don't have a h_errno, I just modified errno... Not really portable, but who cares for an exact error message ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Nov 2007 06:16:15 PM UTC, comment #68: 

As long the linker can drop the code if the application doesn't use it, it's not a problem to add it.

About comments #64/#65 ?

Frédéric Bernon <fbernon>
Group Member
Wed 21 Nov 2007 06:03:36 PM UTC, comment #67: 

Does anyone care for a thread-safe implementation of getaddrinfo/freeaddrinfo (the official successor of gethostbyname, which is not thread-safe)?

I've already done it here but I don't know if we really need it in the core, given the lw in our name...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 20 Nov 2007 10:00:32 PM UTC, comment #66: 

I hope do my last changes on DNS (for a static/per-thread set of variables for hostent) this weekend.

Frédéric Bernon <fbernon>
Group Member
Tue 20 Nov 2007 09:58:25 PM UTC, comment #65: 

Perhaps I'm wrong but it seems it's "h_errno", and not "errno" we should set on error :

See "ERRORS" in http://www.opengroup.org/onlinepubs/007908799/xns/endhostent.html


Frédéric Bernon <fbernon>
Group Member
Tue 20 Nov 2007 09:48:02 PM UTC, comment #64: 


>compacted dns_recv using some (ugly) gotos


I do a minor change for that (I think it's a less "ugly"). Except that other changes are good for me.

About sockets.c error handling, perhaps it should be better to extend err_to_errno_table and err.h (and avoid the specific DNS_RESULT) ?

Frédéric Bernon <fbernon>
Group Member
Tue 20 Nov 2007 08:32:04 PM UTC, comment #63: 

The new code works fine!

I did some changes too (see commit log), mainly function documentation, coding style and some attempts to make the code smaller, hope you don't mind!

What remains is a check that the numerous size-defines (dns table size, number of dns servers) fit into the u8_t which is used to index them (I added a precompiler check for that, maybe that's already enough?).

Also, dns_seqno (currently an u8_t) can overflow when more than 256 queries are done. But since the only side-effect is that a new entry gets overwritten, it's not that important...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 20 Nov 2007 09:17:01 AM UTC, comment #62: 


>And about the tests: I used a tcpip_callback to directly use dns_gethostbyname. This way, you can even test the order in which the callbacks come (and printfs don't get mixed). I'll test again tonight.


Oh, When I said "mixed", it wasn't the printf output, but the hostent values (for printf, I use my own printf in my test - a "oprintf" function which use a critical section inside - but I don't copy paste here to be more readable, so, use netconn_gethostbyname is not a problem, and enable to test more layers).





Frédéric Bernon <fbernon>
Group Member
Tue 20 Nov 2007 08:57:56 AM UTC, comment #61: 


>Pretty much, yes! Although I wouldn't give DNS_USES_STATIC_BUF three values (0/1/2). I think the option to use the stack isn't really necessary...


Since cases 0 & 1 are the same, and if Jim prefer use the stack, I don't see any problem to propose a solution for every point of view.

>About the dns defines: why are some of those moved from dns.c to dns.h, are they used anywhere outside dns.c???


DNS_MAX_SERVERS is in dns.h because if you want to know what it the upper limit to call dns_getserver or dns_setserver, you have to see this define.

DNS_MAX_TTL is not well place, but I wanted to move options (in opt.h, dns.h or dns.c) at the end of the task.

DNS_TMR_INTERVAL is here to be see by tcpip_thread (or nosys case in test.c).

DNS_MSG_SIZE was here to be checked in init.c, but now, it could be moved in dns.c.

DNS_RRTYPE_ & DNS_RRCLASS_ are here because I want to be able to query a MX or another type. I have test it with a local patch (add 2 parameters to dns_gethostbyname, 2 fields in entry, and a change in dns_recv to resend a query when we got MX name to resolve it in IP address). But it should stay an option (and for post 1.3.0 I think).

So, to my point of view, I would place :

in opt.h:
- DNS_TABLE_SIZE (ram usage should be visible)
- DNS_MAX_NAME_LENGTH (ram usage should be visible)
- DNS_USES_STATIC_BUF (ram usage should be visible)
- DNS_DOES_NAME_CHECK (in the same spirit than DHCP_DOES_ARP_CHECK)

in dns.h:
- DNS_MAX_SERVERS (2 is an usual value, not really need to change it)
- DNS_TMR_INTERVAL (should be visible)
- DNS_RRTYPE_xxx (will avoid to move them again for future change)
- DNS_RRCLASS_xxx (will avoid to move them again for future change)

in dns.c:
- DNS_SERVER_ADDRESS (I think this default DNS server should work for most of users)
- DNS_SERVER_PORT (should not be change, but...)
- DNS_MSG_SIZE (only use locally now)
- DNS_MAX_RETRIES (only use locally, but could be move to opt.h)
- DNS_FLAG1_xxx (only use locally)
- DNS_STATE_xxx (only use locally)

Frédéric Bernon <fbernon>
Group Member
Tue 20 Nov 2007 07:24:15 AM UTC, comment #60: 


> Ok, it's check in, hoping it is what you expected


Pretty much, yes! Although I wouldn't give DNS_USES_STATIC_BUF three values (0/1/2). I think the option to use the stack isn't really necessary...

About the dns defines: why are some of those moved from dns.c to dns.h, are they used anywhere outside dns.c???

And about the tests: I used a tcpip_callback to directly use dns_gethostbyname. This way, you can even test the order in which the callbacks come (and printfs don't get mixed). I'll test again tonight.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Nov 2007 10:50:32 PM UTC, comment #59: 

Ok, it's check in, hoping it is what you expected.

"Implement DNS_DOES_NAME_CHECK option (check if name received match the name query), implement DNS_USES_STATIC_BUF (the place where copy dns payload to parse the response), return an error if there is no place for a new query, and fix some minor problems."

I not yet move DNS options in opt.h, I think we should be agree that some of them should stay in dns.c (but which ones?).

To check the behavior, I do this code :

static void
dns_thread(void *arg)
{ struct ip_addr addr;
  err_t err;
  if ((err = netconn_gethostbyname((char*)(arg), &(addr))) == ERR_OK) {
    printf("netconn_gethostbyname(%s)==", (char*)(arg));
    ip_addr_debug_print(LWIP_DBG_ON, (&addr));
    printf("\n");
  } else {
    printf("netconn_gethostbyname(%s)==%i\n", (char*)(arg), (int)(err));
  }
}

...

{
  sys_thread_new("dns_thread", dns_thread, (void*)("yoda.hymatom.fr"),   DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO);
  sys_thread_new("dns_thread", dns_thread, (void*)("host1.hymatom.fr"), DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO);
  sys_thread_new("dns_thread", dns_thread, (void*)("host2.hymatom.fr"), DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO);
  sys_thread_new("dns_thread", dns_thread, (void*)("host3.hymatom.fr"),   DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO);
  sys_thread_new("dns_thread", dns_thread, (void*)("host4.hymatom.fr"),   DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO);
  sys_thread_new("dns_thread", dns_thread, (void*)("host5.hymatom.fr"), DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO);
}

And I got good results (two of them returns -1, ERR_MEM, since table size is 4). Others give good results.

Last, I also test lwip_gethostbyname, but results are mixed due to the static set of variables. inet_ntoa should also be avoid (same problem of course).

Frédéric Bernon <fbernon>
Group Member
Mon 19 Nov 2007 09:16:21 PM UTC, comment #58: 


>> I'd rather return ERR_MEM ...
> I will do


Good. I'll update the sequential API to return EAGAIN once you're done.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Nov 2007 08:37:05 PM UTC, comment #57: 


Returning ERR_MEM if the table is full is fine with me; in practicality I think most lwIP systems will have a fixed number of hosts to look up anyway, and intelligent dimensioning of the table will preclude any problems like this. For example, I'll never have more than 3 host names to resolve.. an SMTP server, a POP3 server, and a network printer.

Jim Pettinato <jim_pettinato>
Group Member
Mon 19 Nov 2007 08:30:45 PM UTC, comment #56: 


>Thanks. But one question must be discussed, I think: do we abort outstanding request when the table is full? I'd rather return ERR_MEM so we could try again. This retry could even be handled by the sequential api functions...


I will do

Frédéric Bernon <fbernon>
Group Member
Mon 19 Nov 2007 08:27:19 PM UTC, comment #55: 


> The client has changed significantly since the initial port from uIP


Oh, OK. I must say I didn't really follow this discussion until the point where I started to implement the sequential api functions...

I thought it was taken from uIP like that (and wondered how it could ever have worked, HAHA!)

> I'm already working on most of these problems.

and

> it's lwIP code now... so it's as much yours as


Thanks. But one question must be discussed, I think: do we abort outstanding request when the table is full? I'd rather return ERR_MEM so we could try again. This retry could even be handled by the sequential api functions...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Nov 2007 08:06:33 PM UTC, comment #54: 

I'm already working on most of these problems. I think most will be check in this evening. So, please, wait until "tomorrow" (I'm on IRC if you want to chat)...

Frédéric Bernon <fbernon>
Group Member
Mon 19 Nov 2007 08:00:10 PM UTC, comment #53: 


> - Every call to dns_gethostbyname results in sending
> queries for ALL entries in dns_table resulting in the
> same request being sent many times.


The client has changed significantly since the initial port from uIP, so any stress testing done is likely not valid anyway... as an example, the original client did not call _check_entries() in the _gethostbyname() function... that was added since. I plan on integrating and testing (and appealing for testing help from others) once we've stabilized the module a bit more.

> When a response is received, the name of this response
> is not checked against the name in the dns_table entry,
> thus giving wrong results when reusing entries (because
> a previous entry was aborted that had the same transaction
> ID, used as the table index)


I believe there is a comment in there that suggests we should address this?

And Simon, it's lwIP code now... so it's as much yours as mine or Frederic's... feel free to correct things you find!


Jim Pettinato <jim_pettinato>
Group Member
Mon 19 Nov 2007 07:42:03 PM UTC, comment #52: 


> The global variable is not a problem


That's right, of course! So implementing an option for global vs. mem_malloc shouldn't be a big problem.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Nov 2007 07:34:21 PM UTC, comment #51: 

Uhhm, hust a question, but has anyone really stress-tested this DNS client? I just did and discovered some issues. Since this is not really my code, I'm posting them here (although I'm happy to change the code if you let me ;-)

- When a response is received, the name of this response is not checked against the name in the dns_table entry, thus giving wrong results when reusing entries (because a previous entry was aborted that had the same transaction ID, used as the table index)

-> Possible solutions:
 1. don't abort pending request until timed out
 2. don't use transaction ID directly as index, but use it as a unique id and search for that ID in the array (of course slower than the current implementation but still faster than comparing the name)

- Every call to dns_gethostbyname results in sending queries for ALL entries in dns_table resulting in the same request being sent many times.

-> Don't use dns_check_entries() in dns_gethostbyname()

- struct dns_query and function dns_query() have the same name - leads to warnings with many compilers

- (This one comes from reading the code) If dns_query() doesn't find an empty entry, it reuses the oldest entry, even if that is still waiting for a response (while others might be completed)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Nov 2007 07:00:51 PM UTC, comment #50: 


>A global variable also won't work as multiple threads would access it at the same time, so what's left is mem_malloc, I suppose...


The global variable is not a problem (except the memory use) since dns is like the other lwIP "core" functions, only one thread should access it in the same time (and it's use only inside dns_recv).

About mem_malloc, I prefer having a static buffer. Or an option to let the port decide to use a static or a dynamic buffer (or the stack ?). In all cases, except one or two lines, there is no lot of changes to do...


Frédéric Bernon <fbernon>
Group Member
Mon 19 Nov 2007 06:51:49 PM UTC, comment #49: 

I'm not familiar with the layout/structs dns uses to address the buffer the pbuf points to, but I guess coping with a split response will be very difficult, so we have to copy the response somewhere.

As to the question where to copy it: the stack is a bad idea in my opinion, as you have to allocate the memory for every thread that will eventually use dns (create those threads with a bigger stack). Also, 512 bytes is quite big for a stack on some systems...

A global variable also won't work as multiple threads would access it at the same time, so what's left is mem_malloc, I suppose...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Nov 2007 05:40:55 PM UTC, comment #48: 


>I've found life is so much easier setting the pbuf size to (MSS + size of all headers) that I sometimes forget not everyone can get away with that... so as much as I hate adding additional memory requirements, the response buffer is a good idea. I'd prefer to see a local buffer, and only if a chain is received. What are our standard policies on local vars/stack allocation?


"local buffer" need to increase stack size. It's not a problem for me, but for others for smaller targets, I'm not sure? Else, we can do a local copy only "if p->next!=NULL". Comments?

>One week for a default MAX_TTL? Do you have a reference for that?


Yes, "RFC1035 - 7.3. Processing responses" ask a "one week" max TTL.

>Also, the @todo for response processing is still in there, and comment #46 indicates that has already been addressed. Am I not clear on all that @todo item implies or did it just get left in as an oversight??


Yes, sorry, it's not very clear: MAX_TTL is compliant with one of the requirements of "7.3. Processing responses", but not all others requirements are done. So, that why I said that TTL problem is done, but I let the @todo for others requirements.

>Little things... should dns_setserver() have a return value? Also, the comments are wrong for that function (cut/paste from dns_getserver() without modification).


Sorry for the comment, I will fix it. Except that, the only possible error is to give a too high value to "numdns". Since most of time, 2 DNS servers are used (even in DHCP), this is not a real problem.

>Should we explicitly init secondary server(s) to 0.0.0.0?


No, since dns_servers table is static, all the table is set to zero. We have decide to avoid explicit initializations in a previous task.









Frédéric Bernon <fbernon>
Group Member
Mon 19 Nov 2007 04:46:46 PM UTC, comment #47: 

Thanks Frédéric and Simon for all your work on this... looks great to me.

I've found life is so much easier setting the pbuf size to (MSS + size of all headers) that I sometimes forget not everyone can get away with that... so as much as I hate adding additional memory requirements, the response buffer is a good idea. I'd prefer to see a local buffer, and only if a chain is received. What are our standard policies on local vars/stack allocation?


Questions...

One week for a default MAX_TTL? Do you have a reference for that?

Also, the @todo for response processing is still in there, and comment #46 indicates that has already been addressed. Am I not clear on all that @todo item implies or did it just get left in as an oversight??

Little things... should dns_setserver() have a return value? Also, the comments are wrong for that function (cut/paste from dns_getserver() without modification).

Should we explicitly init secondary server(s) to 0.0.0.0?

I have a new computer, so not able to access CVS currently - I'll find time to get Cygwin installed on the new machine and set up for CVS access as soon as I can, so forgive me for not just addressing some of these things myself.

Jim Pettinato <jim_pettinato>
Group Member
Sun 18 Nov 2007 09:32:11 PM UTC, comment #46: 

Changes for comments #23,24,25 are done. It's also compliant with "RFC1035 - 7.3. Processing responses".

Frédéric Bernon <fbernon>
Group Member
Sun 18 Nov 2007 08:55:25 PM UTC, comment #45: 

Ok, I have check in a change for this problem.

Frédéric Bernon <fbernon>
Group Member
Sun 18 Nov 2007 08:35:00 PM UTC, comment #44: 

About comment #42, what I want to say is this problem only appears with "large" responses, so, perhaps it's not a problem for lot of users.

But I will check in a change for that: a static buffer where we can copy data before processing. It can also be a local one.

Frédéric Bernon <fbernon>
Group Member
Sun 18 Nov 2007 08:06:41 PM UTC, comment #43: 


>> I think the found callback should have an error parameter so the
>> caller knows why the IP was not found (memory
>> error/timoeut/negative answer/string error...)
>
> Not so useful. But we can.


I think it is important to know the source of a failure (as in the rest of the stack, when ERR_MEM is returned). The problem arises in dns.c because old queries are aborted if too many new queries are begun.

If we don't allow new queries when the dns_table is full (so return something like ERR_MEM instead), that would solve the problem as well.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 18 Nov 2007 07:45:32 PM UTC, comment #42: 


> I have add an assert if the recv DNS response doesn't fit in one pbuf


I think we should rather solve that part differently. As it is now, it implies a PBUF_POOL bufsize of 512 minimum when LWIP_DNS is turned on, which is quite wasteful when receiving e.g. TCP ACK packets with only 64 bytes...

> same problem with DHCP


That would be bad!

> I'm not sure this is something to fix, but, I put it in the todo list.


What do you mean with that? If that problem exists, I think it should be fixed! PBUF_POOL should allowed to be smaller than 512, even with DHCP and DNS turned on!

Simon Goldschmidt <goldsimon>
Group administrator
Sun 18 Nov 2007 06:51:23 PM UTC, comment #41: 

I have add an assert if the recv DNS response doesn't fit in one pbuf. I also add a sanity check warning to see if PBUF_POOL_BUFSIZE is large enough (of course, we could receive another kind of pbuf, but it should be good for most of cases).

It seems there is the same problem with DHCP: there is no check that the full message fit in one pbuf. So, it's possible to have a memory overflow...

I'm not sure this is something to fix, but, I put it in the todo list.


Frédéric Bernon <fbernon>
Group Member
Sun 18 Nov 2007 04:36:59 PM UTC, comment #40: 


>maybe we should support multiple response IPs?

Why not?

>I think the found callback should have an error parameter so the caller knows why the IP was not found (memory error/timoeut/negative answer/string error...)

Not so useful. But we can.

I would like to be able to query the MX record.

Frédéric Bernon <fbernon>
Group Member
Sun 18 Nov 2007 01:31:00 PM UTC, comment #39: 

Some more todos:

- convert C++ style comments to C style comments
- better function/parameter documentation
(I'll work on these 2)

- maybe we should support multiple response IPs?
- I think the found callback should have an error parameter so the caller knows why the IP was not found (memory error/timoeut/negative answer/string error...)

Simon Goldschmidt <goldsimon>
Group administrator
Sun 18 Nov 2007 10:15:18 AM UTC, comment #38: 


>  I'm happy you have used a specific struct for do_gethostbyname


I was using struct api_msg_msg because I was using tcpip_msg at first. Then I realized gethostbyname couldn't have a netconn parameter, I only didn't do the struct change in the patch I posted. It's also better this way because it doesn't enlarge the struct api_msg_msg (the current gethostbyname parameters would already be bigger than all other union members in that struct.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 17 Nov 2007 01:22:37 PM UTC, comment #37: 

Ok, last changes are good for me. I'm happy you have used a specific struct for do_gethostbyname, it will be better if one day, we want to add more fields (aliases, more ip addresses...).

I will do my last changes this weekend...

Frédéric Bernon <fbernon>
Group Member
Fri 16 Nov 2007 08:27:58 AM UTC, comment #36: 


>Isn't that MS only?


No, you got the same in FreeBSD :

http://www.gsp.com/cgi-bin/man.cgi?section=3&topic=gethostbyname

>I made it a define because it will be reused in other functions using hosten (e.g. gethostbyname_r)


In all cases, it's not something which have to be see by the users (not in the task #7410 spirit ?). So, move it in sockets.c allows to reuse it. But I think it should give the "static/per thread" option more complex.

>Hehe, I really didn't know how that thing is used and was too lazy to look it up then. Obviously, it would have to be changed. Please change it if you know how it should be.


Ok, I will did once you will have check in your patch. hostent provide two tables, terminated by a NULL pointer. To set it in a struct sockaddr_in, you should write something like that :

sin.sin_addr.s_addr = (((u32_t)(phostent->h_addr_list[0])));

In fact, even if dns client doesn't support it now, you could got several ip addresses in h_addr_list table. You could do something like :

u8_t idx=0;
while (phostent->h_addr_list[idx]) {
  /* or something else */
  sin.sin_addr.s_addr = (((u32_t)(phostent->h_addr_list[idx])));
  idx++;
}


>I don't know either, I only wanted to protect a hostent being mixed between different calls (e.g. name1, addr2), but that's probably not worth it...


Same, I think it's something to do in the same time that the "static/per thread" option.



Frédéric Bernon <fbernon>
Group Member
Fri 16 Nov 2007 07:17:12 AM UTC, comment #35: 


> define h_addr h_addr_list[0]


Isn't that MS only?

> lwip_fill_hostent define, and move it in sockets.c (or directly
> in gethostbyname since this is the only place where it is used)


I made it a define because it will be reused in other functions using hosten (e.g. gethostbyname_r)

> lwip_fill_hostent seems wrong (?): h_aliases should not be NULL


Hehe, I really didn't know how that thing is used and was too lazy to look it up then. Obviously, it would have to be changed. Please change it if you know how it should be.

> m not sure that SYS_ARCH_DECL_PROTECT / SYS_ARCH_PROTECT /
> SYS_ARCH_UNPROTECT (in gethostbyname) give any protection


I don't know either, I only wanted to protect a hostent being mixed between different calls (e.g. name1, addr2), but that's probably not worth it...

I'll check hostent before checking in.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Nov 2007 10:45:31 PM UTC, comment #34: 

Good job ! Only some details to see:

- I will add in sockets.h :

#define h_addr h_addr_list[0]  /* for backward compatibility */

- I will remove from sockets.h the lwip_fill_hostent define, and move it in sockets.c (or directly in gethostbyname since this is the only place where it is used)

- lwip_fill_hostent seems wrong (?): h_aliases should not be NULL, but h_aliases[0] can be set to NULL. h_addr_list can't be a pointer to s_hostent_addr, but h_addr_list[0] is.

- I'm not sure that SYS_ARCH_DECL_PROTECT/SYS_ARCH_PROTECT/SYS_ARCH_UNPROTECT (in gethostbyname) give any protection. But in all cases, as I said in comment #29, I want to got a solution with macros to let the port choose between a "static" or a "per-thread" set of variables if it's need it (and support it).

In all cases, you can check in your patch (I will do my changes later)...

Frédéric Bernon <fbernon>
Group Member
Thu 15 Nov 2007 09:09:42 PM UTC, comment #33: 

... and please ignore the wrong or missing comments :-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Nov 2007 09:07:28 PM UTC, comment #32: 

I've created a function netconn_gethostbyname (in api_lib.c), do_gethostbyname(in api_msg.c) and gethostbyname (in sockets.c, using netconn_gethostbyname).

Almost everything is pretty straightforward except for the method to block the application thread: I've had to allocate a semaphore for every call to netconn_gethostbyname, since there is no netconn available (or else gethostbyname would need a socket parameter).

If this is OK, implementing the remaining functions (_r or whatever) isn't that hard.

Pls see attached patch.

(file #14408)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Nov 2007 08:14:24 PM UTC, comment #31: 

Can we convert all the function parameters char* hostname into const chars?

- dns_table_entry->found
- dns_lookup
- dns_send
- dns_query
- dns_gethostbyname

that would make integration into api_lib.c easier...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Nov 2007 09:29:53 AM UTC, comment #30: 

Simon wrote :

>> Some implementations provide an alternative gethostbyname_r function, e.g.


>If anyone cares to know: I'd vote for implementing gethostbyname() and gethostbyname_r() so the user can decide what to use.


I'm agree, as long that both are based on a "netconn_gethostbyname_r", and not "gethostbyname" using "gethostbyname_r" (like this, the function not used by the user will be remove at link time).

Frédéric Bernon <fbernon>
Group Member
Thu 15 Nov 2007 09:04:15 AM UTC, comment #29: 


>Unfortunately per-thread info isn't really sensible here - it is not just the struct hostent that needs to be per-thread, but the contents of all the pointers it points to.


Yes, my own DNS client (socket based) need 5 fields per thread (hostent, h_name, h_aliases, h_addr_list + addr) to do a clean job.

>This constraint (not being thread-safe) of gethostbyname is widely known and any existing code should already be aware of it. People writing new code for lwIP should read some documentation so they know it isn't thread-safe :-).


Yes, this "socket" function design is not very good (like some old functions). One of good point is we can add this feature to netconn wihtout such problem (always under LWIP_DNS=1, of course). I think that such problem (thread-safe) should be a port problem (like sys_timeouts). I understand that some ports can be happy with just a simple static "set" of variables, but I think we can provide a cleaner solution using macros for both cases. I will thought about that and I'll propose something...

>Some implementations provide an alternative gethostbyname_r function, e.g. from glibc


Yes, I have the same kind of interfaces on some stack

>I'm undecided on that personally, but I think this should not hold up 1.3.


No, you're right, it shouldn't. But I will continue to work on that (sequential api, and "todo list"): if it's good before 1.3.0, I will check in, else, of course, it will wait the next release...

Frédéric Bernon <fbernon>
Group Member
Thu 15 Nov 2007 03:44:49 AM UTC, comment #28: 

Unfortunately per-thread info isn't really sensible here - it is not just the struct hostent that needs to be per-thread, but the contents of all the pointers it points to.

This constraint (not being thread-safe) of gethostbyname is widely known and any existing code should already be aware of it. People writing new code for lwIP should read some documentation so they know it isn't thread-safe :-).

Some implementations provide an alternative gethostbyname_r function, e.g. from glibc:


int gethostbyname_r (const char *name,
struct hostent *ret, char *buf, size_t buflen,
struct hostent **result, int *h_errnop);



Officially, the new API to sort this out is getaddrinfo() (and getnameinfo() for the equivalent of gethostbyaddr()). I'm not sure it's worth the pain, and doing a gethostbyname_r() may be fine for most people in practice. I'm undecided on that personally, but I think this should not hold up 1.3.

Jonathan Larmour <jifl>
Group Member
Wed 14 Nov 2007 11:46:58 PM UTC, comment #27: 

Ok, dns client is check in. "msvc6" port is updated to test it. The "todo list" is updated. We can also thought to sequential api. The main problem is how to provide a safe-thread function :

struct hostent *gethostbyname(const char *name);

In "http://www.opengroup.org/onlinepubs/007908799/xns/endhostent.html", we can read :

"The gethostent(), gethostbyaddr(), and gethostbyname() functions may return pointers to static data, which may be overwritten by subsequent calls to any of these functions."

But the best solution should be to provide a "per-thread" hostent.



Frédéric Bernon <fbernon>
Group Member
Wed 14 Nov 2007 09:13:33 PM UTC, comment #26: 

About comment #22, since there is no objections, I will add dns files this evening.

Frédéric Bernon <fbernon>
Group Member
Tue 13 Nov 2007 09:03:24 PM UTC, comment #25: 

From RFC2181, section 8, page 10:

   Implementations are always free to place an upper bound on any TTL
   received, and treat any larger values as if they were that upper
   bound.  The TTL specifies a maximum time to live, not a mandatory
   time to live.



Jonathan Larmour <jifl>
Group Member
Tue 13 Nov 2007 08:51:19 PM UTC, comment #24: 


>but we don't care, we should be using the TTL passed to us for the cache. It is up to the SERVER, not the client, to default to the MINIMUM ttl specified in the SOA. (Note: RFC1035 does say a TTL of zero means do not cache, only use immediate value)


I still think a client should be able to cache a shorter time than the dns zone ttl. This last is most of time for zone replication. Microsoft (yes, no comments;) ) do it, I suppose than linux got a same feature.

http://support.microsoft.com/?scid=kb%3Ben-us%3B318803&x=14&y=10


Frédéric Bernon <fbernon>
Group Member
Tue 13 Nov 2007 08:25:27 PM UTC, comment #23: 

From RFC 1035 (DNS):

2.3.4. Size limits

Various objects and parameters in the DNS have size limits.  They are listed below.  Some could be easily changed, others are more
fundamental.

labels          63 octets or less
names           255 octets or less
TTL             positive values of a signed 32 bit number.
UDP messages    512 octets or less

So we're good with max name length... as to TTL, variable size is defined but not typical values... but we don't care, we should be using the TTL passed to us for the cache. It is up to the SERVER, not the client, to default to the MINIMUM ttl specified in the SOA.
(Note: RFC1035 does say a TTL of zero means do not cache, only use immediate value)

Jim Pettinato <jim_pettinato>
Group Member
Tue 13 Nov 2007 08:12:16 PM UTC, comment #22: 

Is there any objections to add dns files in 1.3.0 ?

lwip/src/core/dns.c
lwip/src/include/lwip/dns.h

(so, at the same level than dhcp files)

Since the code is under a LWIP_DNS option, there is no code increase, or performance loss if it not set to 1 (0 should be the default).




Frédéric Bernon <fbernon>
Group Member
Tue 13 Nov 2007 08:07:51 PM UTC, comment #21: 


>I saw the comments when I reviewed the file... one question - which 'default' values are you not confident are good now?? Does the todo item associated with that merely require an RFC investigation?


I thought to DNS_TTL_ENTRY and DNS_MAX_NAME_LENGTH.

>I'm referring to the DHCP integration - initializing the server DNS entries would be a nice touch and make life easier for the end customers of our developments.


I think it's a very simple change to do

Frédéric Bernon <fbernon>
Group Member
Tue 13 Nov 2007 06:34:04 PM UTC, comment #20: 


I saw the comments when I reviewed the file... one question - which 'default' values are you not confident are good now?? Does the todo item associated with that merely require an RFC investigation?

Your last point in your previous comment was something I'd like to hightlight... it's an important point.

I'm referring to the DHCP integration - initializing the server DNS entries would be a nice touch and make life easier for the end customers of our developments.

Jim Pettinato <jim_pettinato>
Group Member
Tue 13 Nov 2007 02:22:55 PM UTC, comment #19: 


>It looks like you've addressed just about everything in the todo list required for the raw API except for secondary server support and compressed answer support, neither of which appear to be trivial, and maybe should be separate tasks?


the "todo list" is :

/** @todo: define good default values (rfc compliance) */
/** @todo: secondary server support */
/** @todo: compressed answer support */
/** @todo: improve answer parsing, more checkings... */

the two first ones are thing to do "now", the two others can be done later, to my point of view.

>So... we still have the outstanding questions... does this module get integrated into the stack at some point, or is it to be considered an app? And do we want to release it with 1.3.0 if we do integrate it into the core?


My point of view is that something which doesn't need to change the "core" should be external (in "contrib"), but I can understand that DNS should be provided as a real part of the stack. More, there is a little change to do in tcpip.c to add the DNS timer. I had a LWIP_DNS option to enable to integrate or not the code. So, it could be used in 1.3.0 in some minutes, without breaking anything.

About the DNS server settings, I thought to have a function to initialize them (primary and secondary), and to use some macros like DNS_PRIMARY_SERVER and DNS_SECONDARY_SERVER (to do a static - and default - initialization). A collaboration with dhcp module could be useful...


Frédéric Bernon <fbernon>
Group Member
Tue 13 Nov 2007 01:48:22 PM UTC, comment #18: 


Thanks Frédéric, everything looks fine to me. I appreciate your tackling this; I have just been unable to find much time.

It looks like you've addressed just about everything in the todo list required for the raw API except for secondary server support and compressed answer support, neither of which appear to be trivial, and maybe should be separate tasks?

I will roll this into our current development project here and do some testing using your updates. Any other individuals wishing to try it out are appreciated as well as always!

So... we still have the outstanding questions... does this module get integrated into the stack at some point, or is it to be considered an app? And do we want to release it with 1.3.0 if we do integrate it into the core?


Jim Pettinato <jim_pettinato>
Group Member
Tue 13 Nov 2007 12:48:46 AM UTC, comment #17: 

Hi Jim,

I did some works on the dns client, based on "task #7017" notes. Most of "todo list" is done. It stay some points I have signaled with @todo in "dns.c". I have change the "found" callback, mainly to be able to pass an "arg" parameter to be able to wrote a synchrone call (with a sys_sem_t be example).  I join the files. Can I have your comments?


(file #14372, file #14373)

Frédéric Bernon <fbernon>
Group Member
Fri 02 Nov 2007 06:21:53 PM UTC, comment #16: 

Hi Jim,

Do you have something new about that ?


Frédéric Bernon <fbernon>
Group Member
Mon 16 Jul 2007 09:36:27 PM UTC, comment #15: 


Another item for the TODO list (note to self):

> RE: [lwip-users] Usage example for DNS client (resolv.c)


> I found your lwIP port of resolv.c and noticed that isdigit
> (hostname[0]) is called to see if we are looking up hostname.  > Domain names are allowed to lead with a digit (3com.com , 
> 3m.com).  If you already knew this or just don't care, I
> apologize for wasting your time.


> Sincerely,


> Dustin Stahlback
> Development Engineer
> Multi-Tech Systems, Inc.


Jim Pettinato <jim_pettinato>
Group Member
Mon 16 Jul 2007 01:36:02 PM UTC, comment #14: 


Thanks... have added including the PACK_ macros around the s_dns_answer struct to the list of changes to include for this task.

Jim Pettinato <jim_pettinato>
Group Member
Sat 14 Jul 2007 08:17:24 AM UTC, comment #13: 

(Winformatik Info wrote)

Hi Jim,

I have tested the resolver code from you. It is working great... after a
little debugging. I always had the problem if the domain name to resolve
had an odd number of bytes. If this was the case, the resolver did not
work (I use a 16-bit CPU with an alignment of 2).
To solve the problem I think we should surround the struct
"s_dns_answer" by the two macros PACK_STRUCT_BEGIN and PACK_STRUCT_END
(please see below). This handles a mapping of the struct "s_dns_answer"
starting on an odd address the right way.

/* The DNS answer message structure */
PACK_STRUCT_BEGIN
typedef struct s_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;
  u16_t ttl[2];
  u16_t len;
  struct ip_addr ipaddr;
} DNS_ANSWER;
PACK_STRUCT_END

Hope this also helps others...

Roland

Frédéric Bernon <fbernon>
Group Member
Thu 28 Jun 2007 07:54:53 AM UTC, comment #12: 


>Did I forget anything??


- MAX_NAME_LENGTH is a too generic name (MAX_RETRIES also). Add something ine hte name like DNS or FQDN and LWIP. Put the define in dns.h, with a block like #ifndef/#define/#endif to let the possibility to set the value in lwipopts.h (like you have proposed). In a general way, all this kind of define should have this kind of block, and default should be RFC compliant (timeouts, delays, ...).

- I think you have forget : "- If your send in a short time (shorter than the "not found" timeout) more than LWIP_RESOLV_ENTRIES, the older client of resolv is "lost" but not its callback "found" will never be called (like in STATE_ERROR case). " (from comment #7).

- Tips in comment #8

- And of course, some typo and coding style to do, like explain in contrib.txt file (If you want to see some samples, look in last month "lwip-commits", Simon have do a big job on that, you can see what he changed).

- Perhaps replace #define STATE_ by enum like in some other lwip files (tcp_state, netconn_state...). To my point of view, it's not very nice to have define inside typedefs, and declarations from MAX_NAME_LENGTH to DNS_TABLE_ENTRY should be more in dns.h (but not all lwip modules do that)...

>(I'll need CVS write access, btw..)


Send an email to Kieran for that...


Frédéric Bernon <fbernon>
Group Member
Thu 28 Jun 2007 06:26:01 AM UTC, comment #11: 


> Did I forget anything??


Maybe implementing a synchrone function for gethostbyname in api_lib.c? (If you want to do that.) And renaming the current gethostbyname to something else (though I think that's included in 'prefix all exported
functions with dns_xxxx' :)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 27 Jun 2007 07:27:16 PM UTC, comment #10: 



Looks like I'll be winding up the final test/release soon on the product I'm working on here at the office, and so I'll have some time to work on this... Kieran, feel free to assign this to me and I'll tackle some of the issues mentioned in the comments and get it in a state suitable for integration.

(I'll need CVS write access, btw..)

Things I plan to address:

 - change module name to dns.c - and prefix all exported
   functions with dns_xxxx
 - check all return values from other lwip functions and handle
   appropriately
 - periodically expire entries (check RFC for default time?)
 - changing default #define of MAX_NAME_LENGTH to RFQ
   recommendations with ability for end user override in
   LWIPOPTS.H.
 - send initial request immediately instead of waiting
   for timer if a query is required
 - secondary server support
 - Initializing state table to UNUSED instead of DONE
   (and testing that)
 - testing compressed answer expansion

Did I forget anything??



Jim Pettinato <jim_pettinato>
Group Member
Wed 20 Jun 2007 12:17:08 PM UTC, comment #9: 

(answers from Jim)

>I used resolv_ only because that was uIP's naming convention for the module. We can use whatever everyone thinks is the most clear.


>The DNS cache is only cleared manually in _init() I believe. I agree, the timer function should clear old entries as per the RFC.


>No arguments that a secondary server entry would be beneficial.


>Dns table was initialized in the same manner (to DONE) as it was in uIP - I left it alone since I wasn't sure why.


>I agree checking return values is a good idea - as I said when I posted it, this is still a work in progress and I wanted to develop it a bit more. Others seemed to need it now though so I made it available.


>MAX_NAME_LENGTH was 32 in uIP; our device can only handle host names up to 29 chars anyway so I didn't worry about this - but it's a define. We should conditionally define it so it can be overridden in LWIPOPTS.H (like the number of cache entries is). If you want the default to match the RFC that's fine but that's going to be a large chunk of RAM.


>I have not checked compressed answer expansion - I copied that portion of the code from uIP and have not tested the functionality explicitly.


>I tried to use indent and comment style matching the lwIP standards with the idea in mind that this would be included eventually... I'm sure there's tweaks required since it wasn't the primary goal however. Typos though? Me? Never! Lol

Frédéric Bernon <fbernon>
Group Member
Wed 20 Jun 2007 11:05:44 AM UTC, comment #8: 

Some other remarks for integration:

- a request is not send until the next resolv_tmr. It should be done the first time directly in resolv_query.

- "if((ans->type == 1) && (ans->class == 1) && (ans->len == 4) )" in resolv_recv should use some define or sizeof...

- we can only receive the first entry of a "A" record.

Frédéric Bernon <fbernon>
Group Member
Wed 20 Jun 2007 08:56:12 AM UTC, comment #7: 

You're right about resolv, but "dns" seems more clear than "resolv".

Some remarks about the resolv.c code:

- MAX_NAME_LENGTH is 32. But a FQDN can be really longer. From memory, each "label" of the FQDN can have 63 bytes, the number of labels of a FQDN can be 127, and the FQDN can be max 254. I will check the chapter in RFC to get exact values, but it's sure it's more than 2.

- I don't see in the code where the dns_table cache is cleaned from old entries. So, if a host already resolved change its IP address, we can't reach it... Perhaps I miss simething.

- Only one DNS server ("primary") is defined. As I said in comment #2, a secondary would be good.

- It would be good to check return values for pbuf_alloc, udp_new()..

- dns_table in resolv_init should be initialized with STATE_UNUSED (and not STATE_DONE).

- If your send in a short time (shorter than the "not found" timeout) more than LWIP_RESOLV_ENTRIES, the older client of resolv is "lost" but not its callback "found" will never be called (like in STATE_ERROR case).

- I'm not sure than compressed answers is working (I have to go deeply). Jim, do you have check that?

- And of course, some typo and coding style to do...


 


Frédéric Bernon <fbernon>
Group Member
Tue 19 Jun 2007 03:14:51 PM UTC, comment #6: 


> [..] So, dns_ prefix seems better for RAW API...


That's also true, but why dns when the file is named resolv? Anyway, I don't really care as long as it does get a different name than simply gethostbyname!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jun 2007 02:58:34 PM UTC, comment #5: 

Agreed with Frédéric on the naming question.

Kieran Mansley <kieranm>
Group Member
Tue 19 Jun 2007 02:27:22 PM UTC, comment #4: 


>lwip_gethostbyname() for the raw API version??


It's not a so good name, since all functions prefixed with "lwip_" are sockets functions. RAW API use a prefix from the module name: udp_, tcp_, dhcp_... So, dns_ prefix seems better for RAW API...


Frédéric Bernon <fbernon>
Group Member
Tue 19 Jun 2007 02:22:27 PM UTC, comment #3: 

Jim Pettinato wrote:

>So... how's about lwip_gethostbyname() for the raw API version??


I think that's the best name since it says what it's for!

I'd put the sequential version into the api directory, either api_lib.c/api_msg.c or sockets.c...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jun 2007 01:41:29 PM UTC, comment #2: 

dns_gethostbyname for raw api and keep gethostbyname for socket layer, and netconn_gethostbyname for netconn layer ?

More, I would like to get a secondary DNS server in the code.

For socket layer, I have something like that (file attached), but only providing the last IP address if the A request return several entries. Comments are in french, and some improvements are need to be in lwip spirit (some alignements check, coding style, etc...). You can use "as is". Also note it's based on the fact to get LWIP_SO_RCVTIMEO=1...



(file #13103)

Frédéric Bernon <fbernon>
Group Member
Tue 19 Jun 2007 01:10:16 PM UTC, comment #1: 

I'd change the function name gethostbyname() to something else for RAW api and create other (blocking) functions that can be used in sequential socket api applications, only to be more compatible to other gethostbyname functions.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jun 2007 12:37:35 PM UTC, original submission:  

Several users ask for a DNS client to get functions like "gethostbyname".

Jim Pettinato has ported uIP "resolver" on lwIP (based on RAW API).

We have to check if we have right to use it, and decide where put the code (contrib, attached on a task, ...)
.

Frédéric Bernon <fbernon>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14560:  netdb.patch added by fbernon (2KiB - application/octet-stream)
file #14408:  dns_api.patch added by goldsimon (11KiB - text/x-patch)
file #14372:  dns.c added by fbernon (19KiB - text/x-csrc)
file #14373:  dns.h added by fbernon (3KiB - text/x-chdr)
file #13103:  dns.c added by fbernon (13KiB - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dempson (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by jim_pettinato (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-12-15 fbernon StatusIn Progress Done
        Percent Complete90% 100%
        Open/ClosedOpen Closed
    2007-12-04 fbernon Attached File- Added netdb.patch, #14560
    2007-11-19 jim_pettinato Should be Finished on2007-06-18 2007-12-18
    2007-11-15 goldsimon Attached File- Added dns_api.patch, #14408
    2007-11-14 fbernon Percent Complete80% 90%
        Planned ReleaseNone 1.3.0
    2007-11-13 jim_pettinato Priority3 - Low 5 - Normal
        StatusNone In Progress
        Percent Complete10% 80%
    2007-11-13 fbernon Attached File- Added dns.c, #14372
        Attached File- Added dns.h, #14373
        Percent Complete0% 10%
    2007-07-16 jim_pettinato Assigned toNone jim_pettinato
    2007-06-19 fbernon Attached File- Added dns.c, #13103

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code