On Mon, Jan 31, 2011 at 10:11 AM, Vladislav Grishenko <[email protected]> wrote: > Hello, > >> This looks like a anti-improvement wrt code readability. >> Why do you do this change at all? The change isn't needed for your patch. > Kept as is, only jump to the address post-increment was added. > >> -//TODO: DHCP servers do not always sit on the same subnet as clients: >> should *ping*, not arp-ping! >> + /* TODO: DHCP servers do not always sit on >> the same >> + * subnet as clients: should *ping*, not >> arp-ping! >> + */ >> >> Please don't so this. The comment meant to stand out, because it indicates a >> bug we need to fix. > Roger that
Thanks, now it's easier to read. After the patch, when we detect (via ARP) that address is busy, we increment lease_epoch, which makes all future searches start on (hashed_address + 1). Why we don't do it when, for example, we find that address is busy because it is reserved for a static IP? Or rather: why do we do that lease_epoch++ thing? Who said that we need to prevent any future probes on that IP? Especially that: * we don't really prevent anything, because _other_ MAC addresses can still hash to it. * nobody_responds_to_arp already takes care of it by creating a temporary 1 hour (by default) lease for the busy IP. * this makes code a bit more complex. I propose to drop lease_epoch thing. Another thing. Before this, we had this failure mode (if I understood you right): * win7 laptop with two ethernets gets IP1 from us. * then it disconnects. * lease expires. * then it connects using second eth, gets the same IP1 and win7 becomes terribly confused by having two identical IP addresses on both interfaces. Well, before this patch it happens reliably (every time, and for every laptop user), and after it, it will happen to a small (~1%) proportion of users whose both MACs hash to the same starting IP. When they will come complaining to the mailing list, no one will be able to reproduce the problem. "But it works for me!" This is not a big improvement. We need a better solution, and in this case one better solution is, unfortunately (for Win7 users), to fix OS to not consider downed interfaces as valid for routing. -- vda _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
