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
