patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6786, task #7507: Hook to support static...

 
 

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

patch #6786: task #7507: Hook to support static hosts file in DNS lookup

Submitter:  Robert Sprowson <sprow>
Submitted:  Thu 19 Mar 2009 10:09:08 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  1.4.0

Jump to the original submission

Sat 09 May 2009 01:45:05 PM UTC, comment #13: 


> One minor addition I would suggest: currently there's no means
> of defining the prototype for the callout function so the
> compiler warns about its implicit declaration.


I've also thought about that, but sys.h or sys_arch.h is definitively not the right header, because they are for OS adaption. It would be enough if the function is defined in cc.h or even lwipopts.h.

I deliberately did not prototype the function as you could decide to have it static, extern or inline, which would be pre-decided if I made that prototype.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 09 May 2009 09:29:18 AM UTC, comment #12: 

Nice! So now we support
 a) runtime maintained static DNS held in RAM
 b) compile time assigned static DNS anywhere in mapped memory
 c) runtime maintained static DNS held off chip

I've just tested (c) and it works just fine, reading through (a) and (b) looks sane too.

One minor addition I would suggest: currently there's no means of defining the prototype for the callout function so the compiler warns about its implicit declaration.

I added to line 79
 #include "lwip/sys.h"
since that includes sys_arch, which as discussed earlier is the sensible place to put the callout function.

Robert Sprowson <sprow>
Thu 07 May 2009 03:29:11 PM UTC, comment #11: 

Robert, good news for you: I've added an #ifdef for an external function (DNS_LOOKUP_LOCAL_EXTERN()) that you can use.

Also, I've combined the remove-functions like you suggested and updated the !DYNAMIC case (can be put into a different section, e.g. flash, and is read-only).

I hope that does it.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Apr 2009 05:41:17 AM UTC, comment #10: 


> You've lost me, how would I point the list at my EEPROM? It's a 6 pin serial
> device that I have to clock commands to to get bytes back.


OK, that cleary doesn't work like I thought it would. I thought it was memory-mapped...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 22 Apr 2009 09:35:33 PM UTC, comment #9: 

You've lost me, how would I point the list at my EEPROM? It's a 6 pin serial device that I have to clock commands to to get bytes back.

I see from the diffs that it is possible to find out how many entries were removed, but the asymmetry still seems odd since the entries aren't stored in the list in groups. Maybe a single function that allows a NULL string or wildcard address, viz

 err_t dns_local_addhost(const char *hostname, const struct ip_addr *addr)
 int dns_local_removehostaddr(const struct ip_addr *addr)
 int dns_local_removehostname(const char *hostname)

becomes
 err_t dns_local_addhost(const char *hostname, const struct ip_addr *addr)
 int dns_local_removehost(const char *hostname, const struct ip_addr *addr)

and called like
 dns_local_removehost(NULL, 127.0.0.1)
or
 dns_local_removehost("localhost", 255.255.255.255)
or both parameters for a single unique entry?

Robert Sprowson <sprow>
Wed 22 Apr 2009 07:25:24 AM UTC, comment #8: 


> I guess it could be done by just defining DNS_LOCAL_HOSTLIST_INIT to point to an array of structs local_hostlist_entry in FLASH...?


What I meant with that is that that you could essentially point the list to your EEPROM. I just discovered my current code does not handle this correctly (tries to write-access the list), but I'll change that. It should work exactly like you want, I'll make sure I'll test it that way.

As to the 2 functions: they both remove all matching entries and return the number of removed entries. So you can, for example, either remove all 127.0.0.1-entries or all addresses for 'localhost'.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 21 Apr 2009 09:46:25 PM UTC, comment #7: 

I'd have a couple of comments on what you've suggested, naturally my suggested patch fulfilled my personal requirements and might not be the best approach.

The function call out/hook allowed me to tackle the specific problem (which is possibly quite common for embedded systems) where I don't have much RAM, the flash that the code is running from is either large sectored or can't be erased/written in place, but I want to allow for quite a few 'hosts' file entries.

I store the 'hosts' file in a serially accessed EEPROM (ie. it's not mapped to a region the processor can get at by just casting flash to a pointer), the values rarely change but could be up to 2k in size - I'd rather have that 2k of RAM for buffers.

Anyway, on the proposed API

  int dns_local_removehostname(const char *hostname);
  int dns_local_removehostaddr(const struct ip_addr *addr);

I can see a problem there when there are aliases in the hosts file, in my (FreeBSD) hosts file I have loopback and localhost and loghost all as aliases for 127.0.0.1 so the removehostaddr wouldn't know which one to remove. The removehostname is probably OK as one address maps to only one name doesn't it?

Not sure how best to proceed, but not being able to retain the list off chip without burning up free RAM is a killer.

Robert Sprowson <sprow>
Tue 21 Apr 2009 06:45:31 PM UTC, comment #6: 

Robert, I'm afraid I have to disappoint you a bit: reading through task #7507, I remembered our previous decision to make the 'static' local table a bit more 'unstatic' in the sense of providing add/remove functions. I couldn't see how to integrate this neatly with your proposal unless adding yet another configuration option à la first-look-into-hook-then-look-into-local-table, which I'm not really convinced of.

Would you mind integrating with the changes I made to that task? I guess it could be done by just defining DNS_LOCAL_HOSTLIST_INIT to point to an array of structs local_hostlist_entry in FLASH...?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Apr 2009 07:39:42 PM UTC, comment #5: 

Gah, you're right. Let's make the example

 if (stricmp(name, "loopback") == 0) return htonl(0x7F000001);

I just wanted to choose an example that was generic as obviously listing some machines on my home network wouldn't make much sense! For my architechture I scan some EEPROM where extra host names can be saved.

Robert Sprowson <sprow>
Tue 14 Apr 2009 06:01:49 PM UTC, comment #4: 


> I guess the simplest reference implementation I could think of
> would be ...


Note that 'localhost' is already covered by dns_gethostbyname.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 03 Apr 2009 11:09:36 AM UTC, comment #3: 

I've marked this as 1.4.0 but would be happy to take it earlier if someone wants to test and commit

Kieran Mansley <kieranm>
Group Member
Fri 20 Mar 2009 08:47:33 PM UTC, comment #2: 

Changing  sys_ -> sys_arch_  good point, it's definitely architecture specific.

I guess the simplest reference implementation I could think of would be

u32_t sys_arch_hosts_file_dns_lookup(const char *name)
{
  if (stricmp(name, "localhost") == 0) return htonl(0x7F000001);
  return 0;
}

since any inspection of a hosts file (be it ROM/RAM/filing system) is going to be platform specific too.

Robert Sprowson <sprow>
Fri 20 Mar 2009 06:58:19 AM UTC, comment #1: 

Looks fine to me although we should provide a reference implementation: Most systems will have the file either in RAM or ROM/flash since we are talking mostly about embedded systems here.

Although, for functions that a port has to implement, the prefix should be sys_arch_, not sys_ only.

Oh, and I've renamed the summary to reflect the fact that there is already a task for this and it should get closed together with this patch.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 19 Mar 2009 10:09:08 PM UTC, original submission:  

Most systems allow a hosts file so that a few static IP addresses can be defined, say for things on a local subnet which can't be resolved.

This patch adds a callout to 'dns.c', provisionally called sys_hosts_file_dns_lookup() for want of a better name. This allows (under the control of define DNS_HAS_HOSTS_FILE) a system specific hosts file to optionally return a result.

Some systems may opt to cache the hosts file in RAM, others may parse its contents and store in a compact form, other mays need to scan a disc drive (hence sys_ territory).

Robert Sprowson <sprow>

 

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

Attached Files
file #17736:  diffs.txt added by sprow (456B - text/plain - Unified diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by sprow (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-05-07 goldsimon StatusNeed Info Done
        Open/ClosedOpen Closed
    2009-04-21 goldsimon StatusNone Need Info
        Assigned toNone goldsimon
    2009-04-03 kieranm Planned ReleaseNone 1.4.0
    2009-03-20 goldsimon SummaryHook to support static hosts file in DNS lookup task #7507: Hook to support static hosts file in DNS lookup
    2009-03-19 sprow Attached File- Added diffs.txt, #17736

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code