Hi Martin, I have some comments on your patch:
On Mon, Oct 25, 2010 at 03:53:16PM +0800, [email protected] wrote: > diff --git a/gdhcp/server.c b/gdhcp/server.c > +/* 8 hours */ > +#define DHCP_LEASE_SEC (8*60*60) I'd like to be able to set the lease time as a server option, and use DHCP_LEASE_SEC as the fallback value. > +struct _GDHCPServer { > + gint ref_count; > + GDHCPType type; > + gboolean started; > + int ifindex; > + char *interface; > + uint32_t start_ip; > + uint32_t end_ip; > + uint32_t server_nip; You're using *_nip in several places where for an IP. Could you please use *_ip for that then ? > + uint32_t lease_seconds; > + int listener_sockfd; > + guint listener_watch; > + GIOChannel *listener_channel; > + GList *lease_list; > + GHashTable *nip_lease_hash; > + GHashTable *option_hash; /* Options send to client */ > + GDHCPSaveLeaseFunc save_lease_func; > + GDHCPDebugFunc debug_func; > + gpointer debug_data; > +}; > + > +struct dhcp_lease { > + uint32_t expire; That should be a time_t. > + uint32_t lease_nip; > + uint8_t lease_mac[6]; You can include if_ether.h to this code and replace all the 6 in this code with ETH_ALEN. > + uint8_t pad[2]; > + /* total size is a multiply of 4 */ > +} __attribute__((packed)); Why does this need to be packed ? > +/* Check if the IP is taken; if it is, add it to the lease table */ > +static gboolean nobody_responds_to_arp(uint32_t nip, const uint8_t *safe_mac) Let's call this one arp_check() and it only needs an IP argument. > +static uint32_t find_free_or_expired_nip(GDHCPServer *dhcp_server, > + const uint8_t *safe_mac) > +{ > + uint32_t addr; > + struct dhcp_lease *lease; > + GList *list; > + addr = dhcp_server->start_ip; Please use another name than addr for representing an IP here. An address sounds like a MAC address to me. > + for (; addr <= dhcp_server->end_ip; addr++) { > + /* ie, 192.168.55.0 */ Sorry for being pedantic, but I think you want e.g. instead of i.e. here. > + if ((addr & 0xff) == 0) > + continue; > + > + /* ie, 192.168.55.255 */ > + if ((addr & 0xff) == 0xff) > + continue; > + > + lease = find_lease_by_nip(dhcp_server, (uint32_t) htonl(addr)); > + if (lease != NULL) > + continue; > + > + if (nobody_responds_to_arp(htonl(addr), safe_mac) == TRUE) > + return htonl(addr); > + } > + > + /* The last lease is the oldest one */ > + list = g_list_last(dhcp_server->lease_list); > + if (list == NULL) > + return 0; So the logic here seems to be that if we haven't found non leased IP, we check for the first potentially expired one and return it if it's actually expired. I know we're not supporting DHCP renewal (see my comments below) yet, but once we do, the lease entries will be updated with new lease time, and your list may not be sorted as expected. So: 1) Shouldn't we simply use a hash table for the lease per MAC representation ? 2) Shouldn't we traverse the said hash table and return the first expired lease ? > +static void init_packet(GDHCPServer *dhcp_server, struct dhcp_packet *packet, > + struct dhcp_packet *old_packet, char type) Could we replace the old_packet naming with client_packet instead ? old_packet is a bit confusing I think. > +static void add_option(gpointer key, gpointer value, gpointer user_data) > +{ > + const char *option_value = value; > + uint8_t option_code = GPOINTER_TO_INT(key); > + struct in_addr nip; > + struct dhcp_packet *packet = user_data; > + > + if (option_value == NULL) > + return; > + > + switch (option_code) { > + case G_DHCP_SUBNET: > + case G_DHCP_ROUTER: I think we want some more basic options here: DNS_SERVER and LEASE_TIME at least. > +static gboolean listener_event(GIOChannel *channel, GIOCondition condition, > + gpointer user_data) > +{ > + GDHCPServer *dhcp_server = user_data; > + struct dhcp_packet packet; > + struct dhcp_lease *lease; > + uint32_t requested_nip = 0; > + uint8_t type, *server_id_option, *request_ip_option; > + int re; > + > + if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) { > + dhcp_server->listener_watch = 0; > + return FALSE; > + } > + > + re = dhcp_recv_l3_packet(&packet, dhcp_server->listener_sockfd); > + if (re < 0) > + return TRUE; > + > + type = check_packet_type(&packet); > + if (type == 0) > + return TRUE; > + > + server_id_option = dhcp_get_option(&packet, DHCP_SERVER_ID); > + if (server_id_option) { > + uint32_t server_nid = dhcp_get_unaligned( > + (uint32_t *) server_id_option); > + > + if (server_nid != dhcp_server->server_nip) > + return TRUE; > + } > + > + request_ip_option = dhcp_get_option(&packet, DHCP_REQUESTED_IP); > + if (request_ip_option) > + requested_nip = dhcp_get_unaligned( > + (uint32_t *) request_ip_option); > + > + lease = find_lease_by_mac(dhcp_server, packet.chaddr); > + > + switch (type) { > + case DHCPDISCOVER: > + debug(dhcp_server, "Received DISCOVER"); > + > + send_offer(dhcp_server, &packet, lease, requested_nip); > + break; > + case DHCPREQUEST: > + debug(dhcp_server, "Received REQUEST"); > + if (requested_nip == 0) { > + requested_nip = packet.ciaddr; > + if (requested_nip == 0) > + break; > + } > + > + if (lease && requested_nip == lease->lease_nip) { > + send_ACK(dhcp_server, &packet, > + lease->lease_nip); > + break; > + } > + > + if (server_id_option) > + send_NAK(dhcp_server, &packet); I have several comments on the DHCPREQUEST handling: 1) We should check for a DHCP_LEASE_TIME in the client packet. If there is such option, we should verify that it's lower than ours. If it's not then we should NAK. 2) If we get a DHCPREQUEST with a server ID different than the one we're expecting, we should remove the reserved lease. 3) We should handle the case where we do not receive a DHCPREQUEST at all for an offer we sent. We should have some sort of reasonable timer for that, otherwise we can potentially run out of leases quite easily. > + break; > + case DHCPDECLINE: > + debug(dhcp_server, "Received DECLINE"); > + > + if (server_id_option == NULL) > + break; > + > + if (request_ip_option == NULL) > + break; > + > + if (lease == NULL) > + break; > + > + if (requested_nip == lease->lease_nip) > + remove_lease(dhcp_server, lease); > + > + break; > + case DHCPRELEASE: > + debug(dhcp_server, "Received RELEASE"); > + > + if (server_id_option == NULL) > + break; > + > + if (lease == NULL) > + break; > + > + if (packet.ciaddr == lease->lease_nip) > + lease_set_expire(dhcp_server, lease, > + time(NULL)); Shouldn't we remove the lease when we get a DHCPRELEASE ? > +int g_dhcp_server_set_option(GDHCPServer *dhcp_server, > + unsigned char option_code, const char *option_value) > +{ > + const char *hash_option_value; > + struct in_addr nip; > + > + if (option_value == NULL) > + return -EINVAL; > + > + debug(dhcp_server, "option_code %d option_value %s", > + option_code, option_value); > + switch (option_code) { > + case G_DHCP_SUBNET: > + case G_DHCP_ROUTER: > + if (inet_aton(option_value, &nip) == 0) > + return -ENXIO; > + break; > + default: > + return -EINVAL; > + } > + > + /* Remove the exist option */ > + hash_option_value = g_hash_table_lookup(dhcp_server->option_hash, > + GINT_TO_POINTER((int) option_code)); > + if (hash_option_value != NULL) > + g_hash_table_remove(dhcp_server->option_hash, > + GINT_TO_POINTER((int) option_code)); > + > + g_hash_table_insert(dhcp_server->option_hash, > + GINT_TO_POINTER((int) option_code), > + (gpointer) option_value); Those 3 calls could be replaced with one g_hash_table_replace(). > +void g_dhcp_server_load_lease(GDHCPServer *dhcp_server, unsigned int expire, > + unsigned char *mac, unsigned int lease_ip) > +{ > + add_lease(dhcp_server, expire, mac, lease_ip); > +} > + > +int g_dhcp_server_set_ip_area(GDHCPServer *dhcp_server, > + const char *start_ip, const char *end_ip) Could we please call this one g_dhcp_server_set_ip_range() ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
