Anonymous | Login | Signup for a new account | 11-10-2008 10:47 PST |
Main | My View | View Issues | Change Log | Docs |
Viewing Issue Simple Details [ Jump to Notes ] | [ View Advanced ] [ Issue History ] [ Print ] | ||||||||
ID | Category | Severity | Reproducibility | Date Submitted | Last Update | ||||
0000349 | [udhcp] | trivial | always | 07-22-05 13:38 | 02-12-08 12:42 | ||||
Reporter | itmission | View Status | public | ||||||
Assigned To | |||||||||
Priority | normal | Resolution | fixed | ||||||
Status | closed | ||||||||
Summary | 0000349: max_leases off by one? | ||||||||
Description |
If i set a start address of .100 and an end address of .105, there are 6 leases available. .100, .101, .102, .103, .104, .105 However, if I set max_leases to 6, I get a message in the syslog that reads: udhcpd[682]: max_leases value (6) not sane, setting to 5 instead |
||||||||
Additional Information | |||||||||
Attached Files | |||||||||
|
Notes | |
(0000469) FlyingElephant 08-29-05 04:18 |
********** Overview ********** Many users have been bothered, one way or the other, by a message generated by udhcpd, a sample of which follows: " Aug 29 07:19:19 udhcpd[462]: udhcpd (v0.9.9-pre) started max_leases value (254) not sane, setting to 99 instead " We are bothered by this message because, heck, what did we do to cause insanity. Just so that normal folks can understand programmers in this. "Sanity Check" is the phrase commonly used to refer to the performing of "edits" on parameters - default or not. Thus, during a "Sanity Check", if something is odd, they apparently defer to using the phrase "sane" sometimes. This is misleading to end users when it is used in messages. After looking at the code, I concluded: 1) The message is inappropriate. It is nondescript and it is not really an error - it is more of an "edit" (aka "Sanity Checking") - the kind of edit normally hidden and certainly not seemingly an error (LOG_ERROR is used). 2) The code contains a simple math error which drives some users nuts because they don't see the need for max_leases in the first place and, moreover, part of the implimentation of this feature screws with what should be a simple thing. Basically, the code subtracts the high IP from the low IP to determine how many (memory) structures it needs to allocate, in this case "offer" structures. Thus, it *wanted* the number 100 if one supplied the range 10.42.42.100 to 10.42.42.199 (100 distinct addresses). However, there is a math error, and the number generated is 99. The code does this: NumIPs = Hi - Lo If one wants to properly obtain this number mathimatically they must do this (a strict example - more complex than sometimes needed): NumIPs = AbsoluteOf ( Hi - Lo ) + 1 The current code is just plain wrong and results in users "losing" a potential "offer". One of the addresses will not get used (not necessarily the last or first). Even though they said hey, use 10.42.42.100 to 10.42.42.199 ... simply put, *one* won't get assigned; we can only service 99 PC's wherein our range said we had room for 100. (And that's all we specified! Why won't it work!) ********** Towards A Solution ********** This is the cuprit code segment: num_ips = ntohl(server_config.end) - ntohl(server_config.start); if (server_config.max_leases > num_ips) { LOG(LOG_ERR, "max_leases value (%lu) not sane, " "setting to %lu instead", server_config.max_leases, num_ips); server_config.max_leases = num_ips; } Here is *one* solution to address the issue(s): num_ips = ( ntohl(server_config.end) - ntohl(server_config.start) ) + 1; if (server_config.max_leases > num_ips) { LOG(LOG_NOTICE, "max_leases value of %lu ignored, " "IP range only requires %lu", server_config.max_leases, num_ips); server_config.max_leases = num_ips; } (One may wish to use ABS or a variant here - that is up for further investigation and discussion.) Here I have fixed the math error, changed the message, and changed the log level (from ERROR to NOTICE). There have been some who don't see a need for max_leases, but I do - it is an empowering option. However, the default of this/an option *should not* interfere with normal operation. Here it does because of a coding error and the other metioned reasons. ********** More ********** By the way, and testing validates, "max_leases" *does not* relate / interact with static leases (i.e. ffw users use the ethers file). Therefore, apparently, if we had 100 entries in the ethers file, we could set max_leases to anything (lets pick 10) and yet all 100 (static) IP's would still get assigned. ********** Tests Performed ********** 1) 10.42.42.200 to 10.42.42.201; empty ethers file " Aug 29 07:07:17 udhcpd[462]: udhcpd (v0.9.9-pre) started max_leases value (254) not sane, setting to 1 instead " PC1 set to IP by DHCP - 10.42.42.200 obtained PC2 set to IP by DHCP - no IP obtained 2) 10.42.42.200 to 10.42.42.202; empty ethers file " Aug 29 07:19:19 udhcpd[462]: udhcpd (v0.9.9-pre) started max_leases value (254) not sane, setting to 2 instead " PC1 set to IP by DHCP - 10.42.42.200 obtained PC2 set to IP by DHCP - 10.42.42.201 obtained 3) 10.42.42.200 to 10.42.42.201; ethers file PC1_MAC = 10.42.42.16 PC1 set to IP by DHCP - 10.42.42.16 obtained PC2 set to IP by DHCP - 10.42.42.201 obtained (it coulda been 200, but the PC itself will first ask for it's last used IP) ********** Conclusion ********** Out attention was brought to this because of a message. Our testing indicates there is an issue. A look at the code shows the culprit. We have procided herein one solution. We will attempt next to post a message on the busybox site to the appropriate contact. Regards, K. Jay Rogozinsky |
(0000472) FlyingElephant 08-30-05 09:51 |
We have been working with this in floppyfw newgroups, here are more current suggestions. There are two. The first may be more "comfortable" for the developers to consider; the second is ideal. Of course, to a good C coder, slight tweaks may be in order. ********** 1 ********** num_ips = ( ntohl(server_config.end) - ntohl(server_config.start) ) + 1; if (server_config.max_leases < num_ips) { LOG(LOG_NOTICE, "max_leases will restrict the lease count to %lu" , server_config.max_leases); } if (server_config.max_leases > num_ips) { server_config.max_leases = num_ips; } You will notice that the message has been "moved". We felt that the message means little in the other "place" and means much in this "place". Also, we have removed the reference to IP usage because that could be "lying" (don't forget static allocation). These comments apply to method (2) also. ********** 2********** leases_size = ( ntohl(server_config.end) - ntohl(server_config.start) ) + 1 if ( server_config.max_leases < leases_size ) { leases_size = server_config.max_leases LOG(LOG_NOTICE, "max_leases will restrict the lease count to %lu", server_config.max_leases); } Here, we get the luxury of leaving server_config.max_leases alone and thus alleviating confusion in future reading / understanding of the code. Now: 1) num_ips gets removed from code. 2) leases_size gets added as a global unsigned long. 3) server_config.max_leases, where it is *used* (in me thinks 3 places - one just after this code block and two in other source files), gets changed to leases_size. Regards, K. Jay Rogozinsky et al. at the FloppyFW newsgroups |
(0000569) FlyingElephant 09-23-05 20:17 edited on: 09-23-05 20:46 |
This issue has been "addressed" ... It was fixed about three weeks ago by "landley". S(he) said "Off by one error in max_leases sanity check. Bug 349, apparently." S(he) *did not* alter the error message. It's revision 11305. *Documented* here: http://www.busybox.net/cgi-bin/viewcvs.cgi/trunk/busybox/networking/udhcp/dhcpd.c?rev=11305&view=auto [^] I don't think it's the ideal solution - don't think they even read the supplied material heretofore - shame. |
(0004254) vda 02-12-08 12:42 |
Apparently fixed in svn: /* Sanity check */ num_ips = server_config.end_ip - server_config.start_ip + 1; <==== +1 if (server_config.max_leases > num_ips) { bb_error_msg("max_leases=%u is too big, setting to %u", (unsigned)server_config.max_leases, num_ips); server_config.max_leases = num_ips; } |
Copyright © 2000 - 2006 Mantis Group |