> -----Original Message-----
> From: Samuel Ortiz [mailto:[email protected]]
> Sent: Friday, November 05, 2010 10:59 PM
> To: Xu, Martin
> Cc: [email protected]
> Subject: Re: [V1 DHCP SERVER: PATCH 2/4] add dhcp-server-lib support
>
> 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.
Ok, I will add a API to set the 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 ?
nip means network style ip, big endian
ip is the host style ip, at X86 it is little endian. So the name is used to
discriminate them.
So I'd like to keep the name
>
> > + 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.
Good point. ;-)
>
> > + 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.
Good point.
>
> > + 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.
I will add arp checking part, and all the stuff will be changed a lot at the
patch
> > +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.
Can I use ip_addr?
>
>
> > + 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.
1. the renew happens at half of the lease time, so it never use the expired
lease.
2. and the renew has already be supported, renew in fact is from request package
> So:
> 1) Shouldn't we simply use a hash table for the lease per MAC representation ?
MAC is char[6], I can't find a proper method to map from char[6] to lease hash.
> 2) Shouldn't we traverse the said hash table and return the first expired
> lease ?
We have nip hash table to map from nip to lease
We have dhcp_server->lease_list, and this list keep sorted according expire
time from old to new.
>
>
>
> > +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.
Good point.
>
> > +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.
For DNS_SERVER, do we really need that at our tethering feature for now?
LEASE_TIME, to keep it simple, we do not support lease time from client, and
the lease time is set when create offer, ack package.
see dhcp_add_simple_option(&packet, DHCP_LEASE_TIME, ...), so here we
LEASE_TIME is not needed.
>
> > +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.
I think give a fixed lease time is enough for us, so it does not check the
DHCP_LEASE_TIME option
> 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.
Oh, You are right I set a wrong parameter when call add_lease() in function
send_offer()
That should be:
Lease = add_lease(dhcp_server, OFFER_TIME, packet.chaddr,...).
So after the OFFER_TIME, the lease is expired and can be used by others.
>
>
> > + 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 ?
Set expire is the good. if the client request again, it can got the old
lease/IP very quickly.
>
> > +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().
Good point
>
>
> > +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() ?
Good point
> Cheers,
> Samuel.
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
