> -----Original Message-----
> From: Samuel Ortiz [mailto:[email protected]]
> Sent: Wednesday, November 10, 2010 11:56 PM
> To: Xu, Martin
> Cc: [email protected]
> Subject: Re: [V1 DHCP SERVER: PATCH 2/4] add dhcp-server-lib support
>
> Hi Martin,
I will update the patch according to your feedback. :-)
>
> 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
