Hi Julien,
On Tue, Nov 09, 2010 at 05:41:50PM +0000, Julien Massot wrote:
> Zeroconf implementation conform to rfc 3927.
> Ready for a review
Before reviewing the details, I'd like the following to happen:
- s/gzcip/ipv4ll/
- The IPv4LL steps should be integrated into gdhcp/client.c, as a fallback
step when not receiving a DHCP lease.
- I'm really fine with having a separate gdhcp/ipv4ll.c file for not stuffing
everything into gdhcp/client.c.
Now, the details:
> +typedef enum _listen_mode {
> + L_NONE,
> + L_ARP,
> +} ListenMode;
> +
> +typedef enum _zcip_client_state {
> + PROBE,
> + ANNOUNCE,
> + MONITOR,
> + DEFEND,
> +} ClientState;
You could add those to the ClientState ones, maybe with an IPV4LL_ prefix.
> +struct _GZCIPClient {
> + GZCIPType type;
> + ClientState state;
> + int ifindex;
> + uint8_t mac_address[6];
> + struct in_addr requested_ip;
> + struct in_addr null_ip;
> + uint8_t null_addr[6];
I don't think you need to carry a NULL MAC address with all your clients, see
my comments on the arp() routine.
> + uint8_t broadcast_addr[6];
This one is not needed.
> + char* assigned_ip;
> + int sockfd;
> + struct sockaddr_ll saddr;
> + uint8_t retry_times;
> + guint timeout;
> + guint conflicts;
> + ListenMode listen_mode;
> + int listener_sockfd;
> + GIOChannel *listener_channel;
> + guint listener_watch;
> + GZCIPClientEventFunc lease_available_cb;
> + GZCIPClientEventFunc lease_lost_cb;
Instead of those, we should have ipv4ll specific callbacks for letting the
gdhcp user know about it. For example, ipv4ll_available_cb and ipv4ll_lost_cb.
This way the dhcp plugin will be able to make his own decision.
> + GZCIPClientEventFunc address_conflict_cb;
This one is not needed.
> + gpointer lease_lost_data;
> + gpointer address_conflict_data;
> + gpointer lease_available_data;
> + GZCIPDebugFunc debug_func;
> + gpointer debug_data;
> +};
>
> +static gboolean probe_timeout(gpointer zcip_data);
> +static gboolean announce_timeout(gpointer zcip_data);
> +static gboolean defend_timeout(gpointer zcip_data);
> +
> +static void out(const char *str, gpointer data)
> +{
> + printf("%s", str);
> +}
This piece of code doesn't seem to be used.
> +static inline void debug(GZCIPClient *zcip_client, const char *format, ...)
> +{
> + char str[256];
> + va_list ap;
> +
> + if (zcip_client->debug_func == NULL)
> + return;
> +
> + va_start(ap, format);
> +
> + if (vsnprintf(str, sizeof(str), format, ap) > 0)
> + zcip_client->debug_func(str, zcip_client->debug_data);
> +
> + va_end(ap);
> +}
This will be the client.c debug routine.
> +
> +/**
> + * Pick a random link local IP address on 169.254/16, except that
> + * the first and last 256 addresses are reserved.
> + *
> + */
> +static uint32_t pick(void)
Let's be more explicit and call this one random_ip().
> +{
> + unsigned tmp;
> + struct timeval tv;
> + do {
> + gettimeofday(&tv, NULL);
> + srand(tv.tv_usec);
> + tmp = rand();
> + tmp = tmp & IN_CLASSB_HOST;
> + } while (tmp > (IN_CLASSB_HOST - 0x0200));
> + return ((LINKLOCAL_ADDR + 0x0100) + tmp);
It would be neat if we could call srand() with e.g. the last 2 interface MAC
address bytes, before calling rand(). That would give our ipv4ll
implementation a chance to assign the same ipv4ll IP to the same interface
over a reboot.
> +}
> + /* static uint32_t pick(void){ */
> + /* uint8_t tmp[4]; */
> + /* uint32_t addr; */
> + /* tmp[3] = 192; */
> + /* tmp[2] = 168; */
> + /* tmp[1] = 1; */
> + /* tmp[0] = 1; */
> +
> + /* memcpy(&addr, tmp, sizeof(addr)); */
> + /* return addr; */
> + /* } */
Please remove commented code.
> +char *g_zcip_client_get_address(GZCIPClient *zcip_client)
> +{
> + return g_strdup(zcip_client->assigned_ip);
> +}
You could use the client.c routine for this one.
> +char *g_zcip_client_get_netmask(GZCIPClient *zcip_client)
> +{
> + return g_strdup("255.255.0.0");
> +}
For this one, you could add it to client.c, and that should return
g_dhcp_client_get_option(dhcp_client, G_DHCP_SUBNET) or
g_strdup("255.255.0.0"), depending if we're a local link address or not.
> +static int switch_listening_mode(GZCIPClient *zcip_client,
> + ListenMode listen_mode){
> +
> + GIOChannel *listener_channel;
> + int listener_sockfd;
> +
> + if (zcip_client->listen_mode == listen_mode)
> + return 0;
> +
> + debug(zcip_client, "switching listen from %d to %d\n",
> + zcip_client->listen_mode, listen_mode);
> + if (zcip_client->listen_mode != L_NONE) {
> + g_source_remove(zcip_client->listener_watch);
> + zcip_client->listener_channel = NULL;
> + zcip_client->listen_mode = L_NONE;
> + zcip_client->listener_sockfd = -1;
> + zcip_client->listener_watch = 0;
> + }
> + if (listen_mode == L_NONE)
> + return 0;
> +
> + if (listen_mode != L_ARP)
> + return -EIO;
> +
> + listener_sockfd = zcip_socket(zcip_client->ifindex);
> +
> + if (listener_sockfd < 0)
> + return -EIO;
> +
> + listener_channel = g_io_channel_unix_new(listener_sockfd);
> + if (listener_channel == NULL) {
> + close(listener_sockfd);
> + return -EIO;
> + }
> + zcip_client->listen_mode = listen_mode;
> + zcip_client->listener_sockfd = listener_sockfd;
> + zcip_client->listener_channel = listener_channel;
> +
> + g_io_channel_set_close_on_unref(listener_channel, TRUE);
> + zcip_client->listener_watch =
> + g_io_add_watch_full(listener_channel,
> + G_PRIORITY_HIGH, G_IO_IN,
You need to support G_IO_NVAL | G_IO_ERR | G_IO_HUP as well, or you could end
up polling forever.
I realize this is an issue in client.c, please fix it there.
> + listener_event, zcip_client,
> + NULL);
> + g_io_channel_unref(zcip_client->listener_channel);
> +
> + return 0;
> +}
So I suppose you'll need to enhance the existing client.c one to support L_ARP
mode.
> +static void arp(GZCIPClient *zcip_client, struct in_addr source_ip,
> + uint8_t *target_eth, struct in_addr target_ip){
2 remarks:
- Please add the '{' after a line break.
- Let's call this one send_arp_packet()
> +
> + struct ether_arp p;
> + uint32_t ip_source;
> + uint32_t ip_target;
> + ip_source = htonl(source_ip.s_addr);
> + ip_target = htonl(target_ip.s_addr);
> + memset(&p, 0, sizeof(p));
> + // arp request
Please use /* */ for comments.
> + p.arp_hrd = htons(ARPHRD_ETHER);
> + p.arp_pro = htons(ETHERTYPE_IP);
> + p.arp_hln = ETH_ALEN;
> + p.arp_pln = 4;
> + p.arp_op = htons(ARPOP_REQUEST);
> + memcpy(&p.arp_sha, zcip_client->mac_address, ETH_ALEN);
> + memcpy(&p.arp_spa, &ip_source, sizeof(p.arp_spa));
> + memcpy(&p.arp_tha, target_eth, ETH_ALEN);
You probably don't need the target_eth argument here, just memset it to 0.
> + memcpy(&p.arp_tpa, &ip_target, sizeof(p.arp_tpa));
> +
> + sendto((zcip_client->sockfd), &p, sizeof(p), 0,
> + (struct sockaddr*) &(zcip_client->saddr),
> + sizeof(zcip_client->saddr));
> +}
> +
> +static gboolean send_probe_packet(gpointer zcip_data)
> +{
> + GZCIPClient *zcip_client;
> + guint timeout;
> +
> + zcip_client = zcip_data;
> + /*if requested_ip is not valid, pick a new address*/
> + if ( zcip_client->requested_ip.s_addr == 0)
> + {
"if (bla) {" and not
"if (bla)
{"
> + debug(zcip_client, "pick a new address \n");
> + zcip_client->requested_ip.s_addr = pick();
> + }
> +
> + debug(zcip_client, "sending probe \n");
> +
> + arp(zcip_client, zcip_client->null_ip, zcip_client->null_addr,
> + zcip_client->requested_ip);
> +
> + if ( zcip_client->retry_times < 2 )
> + timeout =
> + random_delay_ms(PROBE_MAX-PROBE_MIN) + (PROBE_MIN*1000);
> + else
> + timeout = (ANNOUNCE_WAIT * 1000);
> +
> + zcip_client->timeout = g_timeout_add_full(G_PRIORITY_HIGH,
> + timeout,
> + probe_timeout,
> + zcip_client,
> + NULL);
Please fix your indentation here.
Also, you need to check for zcip_client->timeout not being 0. Same applies to
most of your g_timeout_add_full() calls in this patch.
> + switch_listening_mode(zcip_client, L_ARP);
> + zcip_client->retry_times++;
> + return FALSE;
> +}
> +
> +static gboolean send_announce_packet(gpointer zcip_data)
> +{
> + GZCIPClient *zcip_client;
> + zcip_client = zcip_data;
> + debug(zcip_client, "sending announce \n");
> + arp(zcip_client, zcip_client->requested_ip, zcip_client->null_addr,
> + zcip_client->requested_ip);
> + if (zcip_client->state == DEFEND){
> + zcip_client->timeout =
> + g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + DEFEND_INTERVAL,
> + defend_timeout,
> + zcip_client,
> + NULL);
> + return TRUE;
> + }
> + else
> + zcip_client->timeout =
> + g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + ANNOUNCE_INTERVAL,
> + announce_timeout,
> + zcip_client,
> + NULL);
> + zcip_client->retry_times++;
> + return TRUE;
> +}
> +
> +static int zcip_recv_arp_packet( GZCIPClient *zcip_client ){
> + int bytes;
> + struct ether_arp arp;
> + uint32_t ip_requested;
> + int source_conflict;
> + int target_conflict;
> +
> + memset(&arp, 0, sizeof(arp));
> + bytes = 0;
> + bytes = read(zcip_client->listener_sockfd, &arp, sizeof(arp));
> + if (bytes < 0)
> + return -1;
return bytes;
> +
> + if ( arp.arp_op != htons(ARPOP_REPLY) &&
if (arp.arp_op...
> + arp.arp_op != htons(ARPOP_REQUEST)){
> + return -1;
Please return a relevant error code, -EINVAL for example.
> + }
> +
> + ip_requested = ntohl(zcip_client->requested_ip.s_addr);
> +
> + source_conflict = memcmp(arp.arp_spa, &ip_requested,
> + sizeof(ip_requested));
> + if (source_conflict != 0){
No need for brackets here.
> + zcip_client->conflicts++;
> + }
> + target_conflict = memcmp(arp.arp_tpa, &ip_requested,
> + sizeof(ip_requested));
> +
> + if (source_conflict !=0 && target_conflict != 0)
!= 0 and not !=0
> + return -1;
Shouldn't that return 0 ? We have no conflicts, it's not an error.
> + debug(zcip_client, "conflict detected\n");
> + if (zcip_client->state == MONITOR){
> + if (source_conflict != 0)
> + return -1;
This doesn't seem to be an error neither.
> + zcip_client->state = DEFEND;
> + debug(zcip_client, "DEFEND mode conflicts : %d\n",
> + zcip_client->conflicts);
> + send_announce_packet(zcip_client);
> + return 0;
> + }
> + if (zcip_client->state == DEFEND){
> + if (source_conflict != 0)
> + return -1;
Ditto.
> + else if (zcip_client->lease_lost_cb != NULL)
> + zcip_client->lease_lost_cb(zcip_client,
> + zcip_client->lease_lost_data);
So, as I said, that should probably call an ipv4 specific hook.
> + }
> + g_zcip_client_stop(zcip_client);
> +
> + zcip_client->conflicts++;
> + if(zcip_client->conflicts < MAX_CONFLICTS)
if (), not if().
> + g_zcip_client_start(zcip_client);
> + else
So here we got a lot of conflicts. Maybe we should have an additional callback
to let the gdhcp user know about it, and decide if they want to continue
trying or not.
>From a UI point of view, it will look like we're configuring the interface for
ever, which is not nice. At some point we have to admit we failed.
> + g_timeout_add_seconds(RATE_LIMIT_INTERVAL,
> + g_zcip_client_start,
> + zcip_client);
> + return 0;
> +}
> +
> +static gboolean defend_timeout(gpointer zcip_data)
> +{
> + GZCIPClient *zcip_client = zcip_data;
> +
> + zcip_client->conflicts = 0;
> + debug(zcip_client, "back to MONITOR mode\n");
> + zcip_client->state = MONITOR;
> + switch_listening_mode(zcip_client, L_ARP);
> +
> + return FALSE;
> +}
> +
> +static char *get_ip(uint32_t ip)
> +{
> + struct in_addr addr;
> + addr.s_addr = ip;
> + return g_strdup(inet_ntoa(addr));
> +}
You should be able to get this one from client.c
> +static gboolean announce_timeout(gpointer zcip_data)
> +{
> + GZCIPClient *zcip_client = zcip_data;
> + uint32_t ip;
> +
> + debug(zcip_client, "request timeout (retries %d) \n",
> + zcip_client->retry_times);
> +
> + if( zcip_client->retry_times != ANNOUNCE_NUM ){
> + send_announce_packet(zcip_client);
> + return FALSE;
> + }
> +
> + ip = htonl(zcip_client->requested_ip.s_addr);
> + debug(zcip_client, "switching to monitor mode\n");
> + zcip_client->state = MONITOR;
> + if(zcip_client->assigned_ip == NULL)
> + zcip_client->assigned_ip = get_ip(ip);
> + if (zcip_client->lease_available_cb != NULL)
> + zcip_client->lease_available_cb(zcip_client,
> + zcip_client->lease_available_data);
Here again, I'd like to see an IPv4LL specific hook.
> + zcip_client->conflicts = 0;
> +
> + return FALSE;
> +}
> +
> +static gboolean probe_timeout(gpointer zcip_data)
> +{
> +
> + GZCIPClient *zcip_client = zcip_data;
> + debug(zcip_client, "request timeout (retries %d) \n",
> + zcip_client->retry_times);
> +
> + if( zcip_client->retry_times == PROBE_NUM ) {
if (bla) and not if( bla )
> + zcip_client->state = ANNOUNCE;
> + zcip_client->retry_times = 0;
> + send_announce_packet(zcip_client);
> + return FALSE;
> + }
> + send_probe_packet(zcip_client);
> + return FALSE;
> +}
> +
> +static gboolean listener_event(GIOChannel *channel, GIOCondition condition,
> + gpointer zcip_data)
> +
> +{
> + GZCIPClient *zcip_client = zcip_data;
> + int re;
> +
> + if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> + zcip_client->listener_watch = 0;
> + return TRUE;
> + }
> +
> + if (zcip_client->listen_mode == L_NONE)
> + return FALSE;
> +
> + if (zcip_client->listener_sockfd < 0){
> + debug(zcip_client, "socket close \n");
> + return -EIO;
> + }
> + if (zcip_client->listen_mode == L_ARP)
> + re = zcip_recv_arp_packet(zcip_client);
> + return TRUE;
> +}
Here I suppose you'll have to integrate that into the client.c listening event
state machine.
So once you'll have this code integrated, it would be nice to get some
plugins/dhcp.c changes that will implement the ipv4ll specific hooks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
