My modifications

[Ted Lemon]

> 		// data_string_forget (&data, "dhcprequest");  /* What shall I forget? */

When you evaluate an option cache entry, a reference count is made to
the data resulting from the evaluation.   If you don't forget that
reference count, you have a memory leak.   Since in this case you are
immediately copying out of the reference counted storage, and then not
using that storage anymore, you must release that storage.

BTW, if you are thinking of contributing the code back, please mimic
the existing DHCP coding conventions, which are essentially
Berkeley-style.   In particular, this means spaces between keywords,
8-space tabs, no lines longer than 80 columns, comments lined up with
the code indentation, and no c++-style comments.   Braces are after
the keyword that introduces them if possible, not on lines by
themselves.

Also, please do not submit code for review before you've tried to
compile it - the code you submitted will not compile.   Oops,
actually, it will - it's just indented wrong:

		if (packet -> raw -> giaddr.s_addr) {
			struct subnet *subnet;
			ia.len = 4;
			memcpy (ia.iabuf, &packet -> raw -> giaddr, 4);
			subnet = find_subnet (ia);
			if (subnet)
				packet -> shared_network = subnet -> shared_network;
			else
				packet -> shared_network = (struct shared_network *)0;
			} else {
				packet -> shared_network =
					packet -> interface -> shared_network;
				}

Should be:

		if (packet -> raw -> giaddr.s_addr) {
			struct subnet *subnet;
			ia.len = 4;
			memcpy (ia.iabuf, &packet -> raw -> giaddr, 4);
			subnet = find_subnet (ia);
			if (subnet)
				packet -> shared_network = subnet -> shared_network;
			else
				packet -> shared_network = (struct shared_network *)0;
		} else {
			packet -> shared_network =
				packet -> interface -> shared_network;
		}


Your SSO implementation looks fine other than these obnoxious
complaints.   :')

Here are my indentation style settings for emacs:

(c-add-style "kernel"
   '("bsd"
     (c-basic-offset . 8)
     (c-comment-only-line-offset . 0)
     (c-offsets-alist
      (case-label . 6)
      (statement-block-intro . +)
      (knr-argdecl-intro . +)
      (substatement-open . 0)
      (label . 6)
      (statement-case-open . 6)
      (statement-case-intro . 2)
     (statement-cont . +))))

> 	/* scan all leases for a matching address */	
> 		for (lp = ip -> client -> leases; lp; lp = lp -> next) 
> 		{  

Here you need an additional loop to go through all the client
structures hanging off of the interface.   Pseudo-interfaces are
implemented as a linked list of client state structures hanging off of
an interface structure.   The first client state in the list is the
one that is created when you declare the interface or when the DHCP
client discovers it at startup.

I'm not sure why you're doing this:

				if (ip -> client -> active) /* This lease is active */
				{	/* Expire the lease after relase*/
					make_release (ip, lp);
					send_release (ip);
					ip -> client -> active -> expiry = cur_time;
					write_client_lease (ip, ip -> client -> active, 0); 
				}
				/* In SMIP the unconfiguration of the lease is already done by the main process so don't do it here. */
				/* The lease is not active. Send release */
				/* When is a lease active and when isn't it? */
				else {	
					make_release (ip, lp);
					send_release (ip);
					}
				/* A lease was found and released. Now destroy that lease */

The meaning of client -> active is that this is the current lease the
client believes is configured on the network interface, or that should
be configured.   The other leases hanging off of a client state
structure are leases the client remembers, but is not currently
using.   If you acquire a new lease on a new subnet, the old lease
will never be active.

What you should really probably do is check the lease to see if it's
expired, and if it hasn't, release it and rewrite it.   If you are
releasing leases after you've acquired new ones, the old lease will
never be active.

> 				destroy_client_lease (lp);

You'd better not do this unless you take it out of the linked list of
leases and also make sure that the active pointer isn't pointing at
it.   Leases aren't refcnted, so there's no automatic cleanup - you
just have to traverse the list and do it yourself.

> 	return 1; /* No matching leases found */

The convention in the DHCP code is that a return value of 1 means
success, and zero means failure.   This isn't crucial, but consistency
is always a good thing...   :')

> /* Ths function feels a bit thin... It seems as if every old leases will trig
> * a state_init() and that feels dangerous. */

You're correct - state_init() is specific to a client state structure,
not a lease structure.   You should add a loop to loop through the
client structures, and then only consider ip -> client -> active.
You should check the expiry time on the lease against cur_time to see
if the lease has expired - the expiry of a lease does not result in
client -> active being zeroed out.

Anyway, that's the sum of my code review so far - I hope it's more
useful than obnoxious... :')

			       _MelloN_