Hi Martin,
On Wed, Nov 10, 2010 at 01:59:54PM +0800, Xu, Martin wrote:
> > > +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
Fair enough.
> > > + uint8_t pad[2];
> > > + /* total size is a multiply of 4 */
> > > +} __attribute__((packed));
> > Why does this need to be packed ?
So why does this need to be packed ?
> > > + 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?
Sounds better, yes.
> > > + 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.
>
Ok. So when we receive a DHCP renewal packet through a DHCPREQUEST packet, we
should set a new lease time. Calling lease_set_expire() would be enough as it
keeps the lease list sorted. I'm just realizing that we are actually doing
that through add_lease when sending a DHCPACK. So it's all good, sorry for the
noise.
> > > +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?
We do, tethering clients will need a nameserver.
> 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.
>
Yes, and for that we use DHCP_LEASE_SEC. What I'd like to be able to do is
setting the server lease time before starting it, through
g_dhcp_server_set_option().
> > > + 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
>
Giving a fixed lease time is fine, yes. But as far as I understand the DHCP
rfc, the client can still send a proposed lease time in the DHCPREQUEST
packet. What I'm saying is that we should just check that if it's there, it's
lower than our own fixed one, otherwise we shouldn't ACK it.
> > 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.
That would work, yes.
> > > + 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.
>
The set_expire() mechanism is fine, yes. It avoids using a bunch of timers at
the cost of keeping all expired leases around, as the expired leases will be
re-used once all our IP range is full.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
