On Thu, 27.02.14 21:54, Umut Tezduyar Lindskog (umut.tezdu...@axis.com) wrote:
> Implements IPv4LL with respect to RFC 3927 > (http://tools.ietf.org/rfc/rfc3927.txt) and integrates it > with networkd. Majority of the IPv4LL state machine is > taken from avahi (http://avahi.org/) project's autoip. > > IPv4LL can be enabled by IPv4LL=yes under [Network] > section of .network file. > > IPv4LL works independent of DHCP but if DHCP lease is > aquired, then LL address will be dropped. Hmm, how is this hooked up in detail? i.e. when is the IPv4LL state machine started? I think I'd like to see this started after a short while when no DHCP response is seen, and immediately stopped as soon as one is found later on. Or what are your ideas here? Tom? I assume the IPv4ll state machine is started again if the DHCP leas runs out and no DHCP wants to provide a new one? How is this hooked up with the link beat? I mean, if we watch the link beat and it changes, and we already have an ipv4ll address we probably want to check for conflicts immediately? If an IPv4LL address has been acquired, and then a DHCP server becomes available, do we really want to drop the address entirely? At least for IPv6 there's this concept of "deprecated" addresses for this purpose. I am pretty sure that's actually available for IPv4 as well, so maybe we should keep the ipv4ll adderss around and just mark it "deprecated"? This would allow ongoing TCP conenctions to survive, but the kernel wouldn't use the address as source address anymore. I think IPv4LL would be a really valuable addition to networkd. But in contrast to what we did with avahi-autoipd and how it was used by NetworkManager I'd really make it so good that we can enable it by default, so that it just works. Some very superficial comments on the code (Tom and Patrik are probably better folks to review this). > + ll->receive_message = sd_event_source_unref(ll->receive_message); > + if (ll->fd) > + close_nointr_nofail(ll->fd); This looks wrong. You want to check >= 0 here, since 0 is actually a valid fd, and -1 is not... > + uint32_t a = 1; > + int i; > + > + for (i = 0; i < ETH_ALEN; i++) > + a += ll->mac_addr.ether_addr_octet[i]*i; > + a = (a % 0xFE00) + 0x0100; > + addr = htonl(IPV4LL_NETWORK | (uint32_t) a); Hmm, this hash function is probably not that great... We probably should just use siphash here, we kind try to standardize on that... > + if (random_sec) { > + next_timeout += random_u32() % (random_sec * USEC_PER_SEC); > + } Strictly speaking, this is evil, since it doesn't result in even distribution. But then again, it doesn't really matter.... Btw, please avoid extranous {} for single-line blocks like in the case above. This isn't PHP after all! Thanks for doing this work! Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
