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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel