patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5952, Integration of AutoIP module

 
 

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

patch #5952: Integration of AutoIP module

Submitter:  Dominik Spies <dspies>
Submitted:  Tue 22 May 2007 01:45:20 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  fbernon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Thu 28 Jun 2007 06:09:38 PM UTC, comment #39: 

Ok, I have check in a change where rand() calls are replaced by a LWIP_AUTOIP_RAND macro. This one use MAC address and autoip's tried_llipaddr to return a "pseudo random value" (it's not perfect, but...). Of course, you can always use rand() by defining LWIP_AUTOIP_RAND in your lwipopts.h.

So, I close it, but reopen if you want to improve LWIP_AUTOIP_RAND.

Thank you Dominik for your module...

Frédéric Bernon <fbernon>
Group Member
Mon 25 Jun 2007 04:51:07 PM UTC, comment #38: 

I sort of agree with our current philosophy, which is to use lwIP local functions where possible, and libc only where we have to.  My current environment is very lean, with a very sparse libc.  For example, I don't have a rand. 

One of the reasons I chose lwIP was because it was easily ported to such an environment.  Adding a rand() function in itself is no big deal, but adds to the porting cost, and time involved.  Having a default lwIP defined rand for autoip would be nice, with maybe an option to use the libc rand.

Thomas Taranowski <taranowski>
Group Member
Fri 22 Jun 2007 08:05:48 PM UTC, comment #37: 

Hmm, I can't say anything about what ttw should be...

> I'm not sure than in this case, it will be really useful
> (the C-Library got lots of functions, and I suppose we will
> not redefine all that lwIP used...).


What exactly do you mean with that? The problem is other functions can easily self-written (like memcpy/memcmp/strlen/strcpy etc.; with the exception of the printf-functions that are really big in some c-libraries - but there are small substitutes, e.g. from paulOS). The problem with rand() is that it is not easy to create such a function. Because of that, I think it is better (for the sake of lw), to see if we can get good results with 'randomizing' ourself without using rand().

Simon Goldschmidt <goldsimon>
Group administrator
Fri 22 Jun 2007 07:03:25 PM UTC, comment #36: 

I have change the autoip_init (do nothing now) and autoip_create_rand_addr to generate the "autoip", based on MAC address. I attach a test file with some tries (64x 4 MAC addresses).

There is again 2 rand() in the code. Perhaps we can simply create a s32_t based on hwaddr[2],hwaddr[3],hwaddr[4],hwaddr[5]. This is not really something "random", but yes, it can be a solution. These two rand() are:

/* time to wait to first probe, this is randomly
   * choosen out of 0 to PROBE_WAIT seconds.
   * compliant to RFC 3927 Section 2.2.1
   */
autoip->ttw = (rand() % (PROBE_WAIT * AUTOIP_TICKS_PER_SECOND));

...and...

/* calculate time to wait to next probe */
netif->autoip->ttw = (rand() % ((PROBE_MAX - PROBE_MIN) AUTOIP_TICKS_PER_SECOND) ) + PROBE_MIN AUTOIP_TICKS_PER_SECOND;

With default autoip.hvalues, we obtain a "ttw" value inside [0...100[ (+100 for the second one).


(file #13135)

Frédéric Bernon <fbernon>
Group Member
Fri 22 Jun 2007 06:20:23 PM UTC, comment #35: 

Some delays to get an address (with default timers values):

LWIP_DHCP=1, LWIP_AUTOIP=1, LWIP_DHCP_AUTOIP_COOP=1, without DHCP server, 65 sec to get an autoip address

LWIP_DHCP=1, LWIP_AUTOIP=1, LWIP_DHCP_AUTOIP_COOP=1, with DHCP
server, 4 sec to get an dhcp address

LWIP_DHCP=1, LWIP_AUTOIP=0, LWIP_DHCP_AUTOIP_COOP=0, with DHCP server, 4 sec to get an dhcp address (of course)

LWIP_DHCP=0, LWIP_AUTOIP=1, LWIP_DHCP_AUTOIP_COOP=0, 8 sec to get an autoip address

Dominik, is it normal delays for you?

Frédéric Bernon <fbernon>
Group Member
Fri 22 Jun 2007 03:47:54 PM UTC, comment #34: 


>Can we add MAKEWORD(mac[4], mac[5]) and MAKEWORD(mac[2], mac[3]) or does that get the IP address too high?


The "too high" case is always to process in all case, to avoid to get an IP address outside the range, but adding MAKEWORD(mac[2], mac[3]) could increase the number of case of conflicts (but I'm not a statistics expert)...

>Can't we use the mac addresses, too?


I will study that...

>Whatever we do, I would make it a define so that we can use the C-library rand/srand if someone wants that. I'd only like to be able to leave them away (in the spirit of lw :)


I'm not sure than in this case, it will be really useful (the C-Library got lots of functions, and I suppose we will not redefine all that lwIP used...). I hope to find something to replace it.

>Yes, why not!


Ok


Frédéric Bernon <fbernon>
Group Member
Fri 22 Jun 2007 01:10:46 PM UTC, comment #33: 

I think taking four bytes from the MAC address is better than only 2. Note that tried_llipaddr will be the same for all devices (worst case).

Can we add MAKEWORD(mac[4], mac[5]) and MAKEWORD(mac[2], mac[3]) or does that get the IP address too high?

For the second rand, I don't know. Can't we use the mac addresses, too?

Whatever we do, I would make it a define so that we can use the C-library rand/srand if someone wants that. I'd only like to be able to leave them away (in the spirit of lw :)

> Move llipaddr? That was my first idea, if everyone prefer, why not?


Yes, why not!

Simon Goldschmidt <goldsimon>
Group administrator
Fri 22 Jun 2007 12:43:28 PM UTC, comment #32: 


>Doesn't the compiler align it automatically, inserting unused memory in between? What you say should only happen with 'packed' structs!


You're right, but it seems that I got a problem with this one, and I don't know why (I have to investigate). Move llipaddr? That was my first idea, if everyone prefer, why not?

Else, about comment #29, what's your point of view? But There is another rand() in the autoip's code to remove...

Frédéric Bernon <fbernon>
Group Member
Fri 22 Jun 2007 11:20:36 AM UTC, comment #31: 


> In "struct autoip", the "llipaddr" could not aligned on 32bits (I got the case).


Doesn't the compiler align it automatically, inserting unused memory in between? What you say should only happen with 'packed' structs!

Why don't we simply move the llipaddr to the beginning, then ttw and then the rest? That would make the struct autoip as small as possible.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 22 Jun 2007 10:41:31 AM UTC, comment #30: 

Some remarks:

- In "struct autoip", the "llipaddr" could not aligned on 32bits (I got the case). To avoid than, I propose to change lastconflict & tried_llipaddr from u8_t to u16_t.

- In autoip_start, "if(autoip->tried_llipaddr > MAX_CONFLICTS)" is TRUE for me, so, ttw is 600, which is a bit longer to start the autoip process.

Frédéric Bernon <fbernon>
Group Member
Fri 22 Jun 2007 07:46:57 AM UTC, comment #29: 

Hi Dominik, I currently test Autoip.

About "(169.254.1.0 + MAKEWORD(hwaddr[4], hwaddr[5]) + tried_llipaddr)", note that MAKEWORD parameter are (lobyte,hibyte). So, if you have devices with such MAC Addresses:

00-13-bd-00-01-61 (dev1)
00-13-bd-00-01-62 (dev2)

At the boot time, the first try will be  :

(169.254.1.0 + 0x6101 + 0) (dev1) = 169.254.98.1
(169.254.1.0 + 0x6201 + 0) (dev2) = 169.254.99.1

So, even with lot of devices from one serie, you have to get more 256 devices on the same LAN to start to got some conflicts...

I hope it's clearer... (I think this is the last thing to do for this task).



Frédéric Bernon <fbernon>
Group Member
Fri 22 Jun 2007 07:33:51 AM UTC, comment #28: 

Simons patch for the caclic inclusion problem is okay for me, I tested it and it works fine for me.
So, the rand problem.
RFC3927 says:

The pseudo-random number generation algorithm MUST be chosen so that
   different hosts do not generate the same sequence of numbers.  If the
   host has access to persistent information that is different for each
   host, such as its IEEE 802 MAC address, then the pseudo-random number
   generator SHOULD be seeded using a value derived from this
   information.  This means that even without using any other persistent
   storage, a host will usually select the same IPv4 Link-Local address
   each time it is booted, which can be convenient for debugging and
   other operational reasons.  Seeding the pseudo-random number
   generator using the real-time clock or any other information which is
   (or may be) identical in every host is NOT suitable for this purpose,
   because a group of hosts that are all powered on at the same time
   might then all generate the same sequence, resulting in a never-
   ending series of conflicts as the hosts move in lock-step through
   exactly the same pseudo-random sequence, conflicting on every address
   they probe.


Why I used only 4 Byte of the MAC-Address for srand has no reason. I saw that somewhere and just did it the same way. The cut 2 Bytes of the "Manufacturerer Bytes" should not be a problem. What should be avoided is cut bytes from the end what could result in same seed for a series of devices.

To just increment the LL-Address by adding MAC[4], MAC[5] and the number of tries is not a good solution. I think the chance for conflicts is to high when many devices from one series is used.

Dominik Spies <dspies>
Tue 19 Jun 2007 12:50:39 PM UTC, comment #27: 

Yes, I use time(NULL) for generic system, not really for this case. Note that using time(NULL) or the magic number should be an usual case (because, most of time, the MAC address will be used).

Perhaps use sequential values (169.254.1.0 + MAKEWORD(hwaddr[4], hwaddr[5]) + tried_llipaddr) (with check of overflow of the range) will be efficient ? (perhaps it's what you propose with "xor for all mac addresses"?)

Frédéric Bernon <fbernon>
Group Member
Tue 19 Jun 2007 10:52:37 AM UTC, comment #26: 


> For information, why don't you like rand/srand?


Only because I want to use as little from a C library as we can.
using time(NULL) for srand is as bad as a magic number: if you have many same devices, they will boot in the same time, so srand will be the same!

I know rand is used for ppp also, but that doesn't make it better ;-)
We have a malloc-replacement, so we also should have a rand replacement. Calculating the xor for all mac addresses could be as good as seeding rand with them (as long as we don't use that random value for security reasons!).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jun 2007 10:16:58 AM UTC, comment #25: 

Ok, I have check in, but tell me if you have an idea to replace the srand/rand. But I note that these functions are also used in PPP part. Perhaps something based on jiffies/ticks? But they are not integrated in current lwIP...


Frédéric Bernon <fbernon>
Group Member
Tue 19 Jun 2007 10:07:52 AM UTC, comment #24: 


>Looks fine to me. Except for the 'magic number' passed to srand... But that shouldn't happen too often, anyway.


In my case, each time I use srand, I call it like this:

srand(time(NULL));

But we can't be sure that time is implemented. So, the magic number is not so bad as workaround...

>Anyway, the need for rand/srand is very unsatisfying to me. Isn't there another possibility?


We could use an iteration inside the range 169.254.1.0 to 169.254.254.255, but it could cause lot of ARP conflict. The idea is to get some random to avoid that all devices try the same address. For information, why don't you like rand/srand?


Frédéric Bernon <fbernon>
Group Member
Tue 19 Jun 2007 09:21:28 AM UTC, comment #23: 

Looks fine to me. Except for the 'magic number' passed to srand... But that shouldn't happen too often, anyway.

Anyway, the need for rand/srand is very unsatisfying to me. Isn't there another possibility?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jun 2007 09:05:55 AM UTC, comment #22: 

I join a patch with some changes for integration in tcpip.c, and some comments.



(file #13100)

Frédéric Bernon <fbernon>
Group Member
Mon 18 Jun 2007 09:46:09 PM UTC, comment #21: 

Ok, I check in your patch Simon...

Frédéric Bernon <fbernon>
Group Member
Mon 18 Jun 2007 09:07:13 PM UTC, comment #20: 

Perhaps it's a style preference, but I prefer the solution which avoid the forward-declare. But it's not my code, change that if you want (the warning disappear with both solutions)...

Frédéric Bernon <fbernon>
Group Member
Mon 18 Jun 2007 07:32:54 PM UTC, comment #19: 

What do you think of this patch (of netif.h) to solve the cyclic inclusion problem (simple forward declaration of the used structs instead of including the whole file, also done in tcp.h to forward-declare the struct tcp_pcb used as a pointer in tcp-functions):

>>>patch>>>

--- netif.h 9 Jun 2007 16:57:05 -0000 1.34
+++ netif.h 18 Jun 2007 19:26:50 -0000
@@ -41,10 +41,10 @@
 #include "lwip/inet.h"
 #include "lwip/pbuf.h"
 #if LWIP_DHCP
-#  include "lwip/dhcp.h"
+struct dhcp;
 #endif
 #if LWIP_AUTOIP
-#  include "lwip/autoip.h"
+struct autoip;
 #endif
 
 #ifdef __cplusplus
<<<patch<<<

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Jun 2007 07:20:57 PM UTC, comment #18: 

Oh, forgot to answer:

> I also propose to integrate autoip_fine_tmr in tcpip.c like
> dhcp timers.
> No objects?


No!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Jun 2007 04:03:03 PM UTC, comment #17: 


> You don't got something like this?


No, not with msvc2005. But I do with GCC, I only didn't compile it with that until now. The easiest solution is to convert the argument to a pbuf, that's right.

Another solution might be to forward-declare structs only used as pointers in header files, but since that requires more thinking (:-O), the pbuf is good for me. Keep in mind "Please coordinate changes and requests with Dominik Spies" though... ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Jun 2007 02:15:41 PM UTC, comment #16: 

The warning problem seems to be due to a circular reference between etharp.h and autoip.h. A simple solution is to replace ...

void autoip_arp_reply(struct netif *netif, struct etharp_hdr *hdr);

by...

void autoip_arp_reply(struct netif *netif, struct pbuf *p);

In autoip_arp_reply, we just have to add:

struct etharp_hdr *hdr;
hdr = p->payload;

No objects?

I also propose to integrate autoip_fine_tmr in tcpip.c like dhcp timers.

No objects?

Dominik, Can you provide some information about your autoip_init() code (why macaddr is use like that)?



Frédéric Bernon <fbernon>
Group Member
Mon 18 Jun 2007 07:26:16 AM UTC, comment #15: 

I just rebuild with LWIP_AUTOIP=1 and LWIP_DHCP_AUTOIP_COOP=1, and I always got these errors :

inc\ipv4/lwip/autoip.h (95) : warning: declaration is not visible outside of function

src\netif\etharp.c (577) : warning: argument of type "struct etharp_hdr " is incompatible with parameter (2 of autoip_arp_reply) of type "struct etharp_hdr "

You don't got something like this ?

Frédéric Bernon <fbernon>
Group Member
Sat 16 Jun 2007 11:46:48 AM UTC, comment #14: 

I think the TODOs in autoip.c directly above the includes are solved, aren't they?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 13 Jun 2007 08:54:16 AM UTC, comment #13: 

Take a look about TODO tags. I have note some specific code in autip_init, and use of constants like 6 for ethadr length (like in eth_addr_cmp). Actually, I have disable it. If you also can help us wih some comments (with javaDoc style) about your functions in autoip.c (search "TODO: Add comment"), it will be great...

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

Nice to see that my code seems to be not so bad at all ;)

about the etharp stuff, I'm not the expert of lwip and I just needed some possibilty to send ARP packages other then requests.. How this is achieved at best for codesize and so on I think you know this better.
Modify it where neccessary.

Dominik Spies <dspies>
Sun 10 Jun 2007 06:47:44 PM UTC, comment #11: 

About etharp_request, you're right, but I wouldn't change lot of thing before the first integration (except style coding). I think you can propose a patch for that, or  directly modify it (if Dominik is agree)... But there is several things "TODO" before getting something well integrated...

Frédéric Bernon <fbernon>
Group Member
Sun 10 Jun 2007 09:59:49 AM UTC, comment #10: 

I haven't tested the module, only looked at the diffs from checking in and it looks OK.

I'd change a part in etharp.c, though: I would favour having only one etharp_request function. If you make etharp_raw a static function if LWIP_AUTOIP=0, it should be nearly the same code as before. That would keep us from having nearly the same code twice.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 09 Jun 2007 06:07:16 PM UTC, comment #9: 

Ok, I have build the change with and without LWIP_AUTOIP. I think nothing change if LWIP_AUTOIP=0, so, I have check in. But I have note several thing to study, like some warning due to autoip.h, some initalizations to study in autoip.c, and some values to replace by defines... Some thing to review are tagged with a TODO mark.

Dominic, thank you for this new module. Tell me if I break something when I port your code to last CVS HEAD, I will fix...

Next week, I will do test when I will be at the office...


Frédéric Bernon <fbernon>
Group Member
Sat 09 Jun 2007 01:40:19 PM UTC, comment #8: 

I join the patch file. Warning, I just have produce it from CVS HEAD, and not build and test it...

Don't forget to copy autoip.c/.h files in ipv4 directories...

I think to test it next week...



(file #12990)

Frédéric Bernon <fbernon>
Group Member
Sat 09 Jun 2007 11:11:17 AM UTC, comment #7: 

I start to produce a patch file from that using the current CVS HEAD...

Frédéric Bernon <fbernon>
Group Member
Wed 23 May 2007 04:01:03 PM UTC, comment #6: 

Hallo again,

I did some work today on that module, fixed some issues and improved a little bit..

Because I need a possibility to send other ARP Packets then request I introduced a raw function in etharp.c. To save some code, I have rewritten etharp_arp_request to use this raw function.

I also modified the DHCP client to cooperate with the AutoIP module.
Configuration to use this is done by Compiler switches (LWIP_DHCP_AUTOIP_COOP)

I've changed name to autoip.

If you have time feel free to port this to CVS Head, I don't think I will have time for this in the near future.

Last, thanks for a great TCP/IP Stack and a great community :)

Dominik Spies <dspies>
Tue 22 May 2007 06:17:13 PM UTC, comment #5: 

Ok. For the name, so, choose the one you think better. To my point of view, autoip.c/.h is better, just because it's we know what the module do just by the name (IPv4LL is more difficult to understand).

ok, also to move it to src\core\ipv4.

For CVS HEAD, tell me, I can do it. I just need to know if you prefer rename files and types yourself, or if you want I do it (with some search&replace, it's only need some minutes)...

Last, thank for for this new module...

Frédéric Bernon <fbernon>
Group Member
Tue 22 May 2007 05:48:39 PM UTC, comment #4: 

Hi!

IPv4LL seems to be a good choice, same as AutoIP. If APIPA is not the name of Microsoft's implementation then I think apipa is good, but this will be not to much work to change.

IPv6 LL is so different from IPv4 (and not a option but more or the less mandatory) I think we should seperate this in any case.
So moving to core/ipv4 is okay. I will do that.

This module was created while my work at Siemens A&D, I'm happy that I can release it under a free license but I cannot spend time to port this to CVS-Head, because there's no benefit for my project.

I hope I can continue this at home, but at the moment I have no hardware, so this is not sure. If I have hardware and some time I will port this to CVS-Head.

In the next days I will programm a patch for dhcp-option to use apipa as a fallback option.

Dominik Spies <dspies>
Tue 22 May 2007 02:47:24 PM UTC, comment #3: 

IPV4LL seems to be a good name :

http://www.zeroconf.org/
http://www.zeroconf.org/AVH-IPv4LL.c

Your opinion?

I also note that ipv6 doesn't use the same RFC:
http://www.faqs.org/rfcs/rfc2462.html

Dominik, do you know if there is lot of difference ? Perhaps it will be good to move the files in src\core\ipv4 ?

Frédéric Bernon <fbernon>
Group Member
Tue 22 May 2007 02:33:13 PM UTC, comment #2: 

First remarks:

- APIPA SEEMS to be the name of Microsoft implementation, I propose you to rename it "autoip" in your files
http://wiki.networkdictionary.com/index.php/APIPA:_Automatic_Private_IP_Addressing

- You add a option LWIP_APIPA to enable/disable the code, and it's a good thing...

- No remarks for netif.h/.c, seems good

- I look for the rest.

Kieran, because for my project, I can be interest for that, I propose to do the update (there is no lot of code).

Just, before do that, I would like to know which name to keep (apipa, autoip, zeroconf...?). If someone can confirm that...

(note that Microsoft is one of Authors of http://www.faqs.org/rfcs/rfc3927.html, so, perhaps apipa is good?)


Frédéric Bernon <fbernon>
Group Member
Tue 22 May 2007 01:59:40 PM UTC, comment #1: 

As you mentioned on one of the mailing lists that these patches aren't against the CVS head (and I'm presuming are large enough that they don't apply cleanly to the CVS head) they're unlikely to get applied unless there is someone willing to bring them up to date.

I appreciate that this is a hassle for you if you're working on the stable release, but it is much easier for you to port them than for one of the other developers as you will be much more familiar with the code you have written.

Kieran Mansley <kieranm>
Group Member
Tue 22 May 2007 01:45:20 PM UTC, original submission:  

A first draft of my APIPA module for lwIP Stack.
There will be more in the next future, especially a cooperation between dhcp/apipa module.

Dominik Spies <dspies>

 

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

Attached Files
file #13135:  AutoIP tests.txt added by fbernon (15KiB - text/plain)
file #13100:  autoip.c.patch added by fbernon (15KiB - text/x-patch)
file #12990:  autoip.zip added by fbernon (9KiB - application/x-zip-compressed)
file #12841:  lwIP AutoIP.zip added by dspies (39KiB - application/x-zip-compressed - New version)
file #12831:  lwIP APIPA.zip added by dspies (24KiB - application/x-zip-compressed)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2007-06-28 fbernon StatusReady For Test Done
        Open/ClosedOpen Closed
    2007-06-22 fbernon Attached File- Added AutoIP tests.txt, #13135
    2007-06-19 fbernon Attached File- Added autoip.c.patch, #13100
    2007-06-09 fbernon StatusIn Progress Ready For Test
    2007-06-09 fbernon Attached File- Added autoip.zip, #12990
    2007-06-09 fbernon StatusNone In Progress
        Assigned toNone fbernon
    2007-05-23 dspies Attached File- Added lwIP AutoIP.zip, #12841
    2007-05-22 dspies Attached File- Added lwIP APIPA.zip, #12831

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code