[systemd-devel] [PATCH] sd-dhcp: implement IPv4 link-local support
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Thu Feb 27 19:07:32 PST 2014
On Thu, Feb 27, 2014 at 09:54:17PM +0100, Umut Tezduyar Lindskog 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.
Looks good.
Minor thoughts below:
> +#define log_ipv4ll(ll, fmt, ...) log_meta(LOG_DEBUG, __FILE__, __LINE__, __func__, "IPv4LL: " fmt, ##__VA_ARGS__)
I imagine that a user might want to control the log level of various
subsystems independently. It's nice that this is already a macro:
at some point we might want to turn the level into something configurable
on its own.
> + ipv4ll_set_state (ll, IPV4LL_STATE_INIT, 1);
Indentation.
> + if (ll->address) {
> + do {
> + uint32_t r = random_u32() & 0x0000FFFF;
> + addr = htonl(IPV4LL_NETWORK | r);
> + } while (addr == ll->address || !(
> + ((ntohl(addr) & IPV4LL_NETMASK) == IPV4LL_NETWORK) &&
> + ((ntohl(addr) & 0x0000FF00) != 0x0000) &&
> + ((ntohl(addr) & 0x0000FF00) != 0xFF00)));
Maybe one of the negations can be inverted:
} while (addr == ll->address ||
(ntohl(addr) & IPV4LL_NETMASK) != IPV4LL_NETWORK ||
(ntohl(addr) & 0x0000FF00) == 0x0000 ||
(ntohl(addr) & 0x0000FF00) == 0xFF00);
This seems more readable without all those parantheses.
> + if (ll->state == IPV4LL_STATE_ANNOUNCING ||
> + ll->state == IPV4LL_STATE_RUNNING) {
IN_SET(ll->state, IPV4LL_STATE_ANNOUNCING, IPV4LL_STATE_RUNNING)
> +
> + if (ipv4ll_arp_conflict(ll, in_packet)) {
> +
> + r = sd_event_get_now_monotonic(ll->event, &time_now);
> + if (r < 0)
> + goto out;
> +
> + /* Defend address */
> + if (time_now > ll->defend_window) {
> + ll->defend_window = time_now + DEFEND_INTERVAL * USEC_PER_SEC;
> + arp_packet_announcement(&out_packet, ll->address, &ll->mac_addr);
> + out_packet_ready = 1;
> + } else
> + conflicted = 1;
> + }
> +
> + } else if (ll->state == IPV4LL_STATE_WAITING_PROBE ||
> + ll->state == IPV4LL_STATE_PROBING ||
> + ll->state == IPV4LL_STATE_WAITING_ANNOUNCE) {
IN_SET...
> +
> + conflicted = ipv4ll_arp_probe_conflict(ll, in_packet);
> + }
> +
> + if (conflicted) {
> + log_ipv4ll(ll, "CONFLICT");
Could we log more information here... With this we'd just have "IPv4LL: CONFLICT"
in the logs. It would be nice to at least see the details of the conflicting address
of something.
> + r = ipv4ll_client_notify(ll, IPV4LL_EVENT_CONFLICT);
> + ll->claimed_address = 0;
> +
> + /* Pick a new address */
> + ll->address = ipv4ll_pick_address(ll);
> + ll->conflict++;
> + ll->defend_window = 0;
> + ipv4ll_set_state(ll, IPV4LL_STATE_WAITING_PROBE, 1);
> +
> + if (ll->conflict >= MAX_CONFLICTS) {
> + log_ipv4ll (ll, "MAX_CONFLICTS");
Whitespace.
> +int sd_ipv4ll_start (sd_ipv4ll *ll) {
> + int r;
> +
> + assert_return(ll, -EINVAL);
> + assert_return(ll->event, -EINVAL);
> + assert_return(ll->index > 0, -EINVAL);
> + assert_return(ll->state == IPV4LL_STATE_INIT, -EBUSY);
> +
> + r = arp_network_bind_raw_socket(ll->index, &ll->link);
> +
> + if (r < 0)
> + goto error;
> +
> + ll->fd = r;
> + ll->conflict = 0;
> + ll->defend_window = 0;
> + ll->claimed_address = 0;
> +
> + if (ll->address == 0)
> + ll->address = ipv4ll_pick_address(ll);
> +
> + ipv4ll_set_state (ll, IPV4LL_STATE_INIT, 1);
> +
> + r = sd_event_add_io(ll->event, &ll->receive_message, ll->fd,
> + EPOLLIN, ipv4ll_receive_message, ll);
> + if (r < 0)
> + goto error;
> +
> + r = sd_event_source_set_priority(ll->receive_message, ll->event_priority);
> + if (r < 0)
> + goto error;
> +
> + r = sd_event_add_monotonic(ll->event, &ll->timer, now(CLOCK_MONOTONIC), 0,
> + ipv4ll_timer, ll);
> +
> + if (r < 0)
> + goto error;
> +
> + r = sd_event_source_set_priority(ll->timer, ll->event_priority);
> +
> +error:
> + if (r < 0)
> + ipv4ll_stop(ll, IPV4LL_EVENT_STOP);
Please not 'error', if the return value might actually be successful.
> + return 0;
> +}
> +
> +int sd_ipv4ll_stop(sd_ipv4ll *ll) {
> + return ipv4ll_stop(ll, IPV4LL_EVENT_STOP);
> +}
> +
> +void sd_ipv4ll_free (sd_ipv4ll *ll) {
> + if (!ll)
> + return;
> +
> + sd_ipv4ll_stop(ll);
> + sd_ipv4ll_detach_event(ll);
> +
> + free(ll);
> +}
> +
> +DEFINE_TRIVIAL_CLEANUP_FUNC(sd_ipv4ll*, sd_ipv4ll_free);
> +#define _cleanup_ipv4ll_free_ _cleanup_(sd_ipv4ll_freep)
Defs at the top...
>
> - if (!link->network->static_routes && !link->dhcp_lease)
> + if (!link->network->static_routes && !link->dhcp_lease &&
> + (!link->ipv4ll || sd_ipv4ll_get_address(link->ipv4ll, &a) < 0))
> return link_enter_configured(link);
Hm, sd_ipv4ll_get_address() < 0 and link_enter_configured()? This is confusing.
> static int set_hostname_handler(sd_bus *bus, sd_bus_message *m, void *userdata, sd_bus_error *ret_error) {
> @@ -493,7 +592,7 @@ static int dhcp_lease_lost(Link *link) {
> address->in_addr.in = addr;
> address->prefixlen = prefixlen;
>
> - address_drop(address, link, address_drop_handler);
> + address_drop(address, link, &address_drop_handler);
I don't think this adds clarity... And we don't actually use & with functions
anywhere.
> + if (link->ipv4ll) {
> + r = sd_ipv4ll_stop (link->ipv4ll);
Whitespace.
> + log_struct_link(LOG_INFO, link,
> + "MESSAGE=%s: IPv4 link-local address %u.%u.%u.%u",
> + link->ifname,
> + ADDRESS_FMT_VAL(address),
> + NULL);
Indentation.
Zbyszek
More information about the systemd-devel
mailing list