[systemd-devel] [PATCH] sd-dhcp: implement IPv4 link-local support

[Lennart Poettering]

On Thu, 27.02.14 21:54, Umut Tezduyar Lindskog (umut.tezduyar at 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