Hi, > -----Original Message----- > From: Zbigniew Jędrzejewski-Szmek [mailto:zbys...@in.waw.pl] > Sent: den 28 februari 2014 04:08 > To: Umut Tezduyar Lindskog > Cc: systemd-devel@lists.freedesktop.org; Umut Tezduyar Lindskog > Subject: Re: [systemd-devel] [PATCH] sd-dhcp: implement IPv4 link-local > support > > 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.
I agree. > > > > + 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... Sorry, didn't understand it. What is this? > > > + > > + 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. It is actually just a conflict. Meaning you will drop your previously ANNOUNCED address. And after CONFLICT, you will see PROBE & ANNOUNCE with new picked address. But maybe it is best to write down the conflicting address in case logs were rotated many many times. Either way for me. > > > + 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. Thanks > > > > +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. Makes sense. > > > + 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... It doesn't matter for me but we should do it all same everywhere I guess. Last time I checked, DHCP was doing it this way. > > > > > - 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. Again, it doesn't matter for me but we should stick to same. In DHCP code, it was mixed usage. I have converted them too to use reference. > > > + if (link->ipv4ll) { > > + r = sd_ipv4ll_stop (link->ipv4ll); > Whitespace. Thanks. > > > + 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 Thanks for doing code review Umut _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel