dhcp: fix unicast pkts, clean up state machine

Nominally a bug-fix cherry-pick, but completely manual. Closer to a
full feature backport minus binary api changes.

Send dhcp unicast packets to ip4-lookup. Otherwise, these packets
won't reach a dhcp server on a different subnet.

Do an immediate client scan after processing wakeup events.

Calculate the next process wakeup time by scanning all
clients.

Increase maximum (idle, no-clients-configured) timeout to 1000
seconds.

Reduce log spew.

Type: fix

Signed-off-by: Dave Barach <dave@barachs.net>
Change-Id: I3d10cd4c353298ed0b19e7e30887dc1d8d07b19e
(cherry picked from commit c54162981cdd41d65ed283df36955007552ddffe)

8 files changed, 442 insertions, 276 deletions

diff --git a/src/vlibmemory/api.h b/src/vlibmemory/api.h
index dc1e75e484d..6cd645bb1d5 100644
--- a/src/vlibmemory/api.h
+++ b/src/vlibmemory/api.h
@@ -26,6 +26,7 @@
 #include <vlibmemory/socket_client.h>
 
 void vl_api_rpc_call_main_thread (void *fp, u8 * data, u32 data_length);
+void vl_api_force_rpc_call_main_thread (void *fp, u8 * data, u32 data_length);
 u16 vl_client_get_first_plugin_msg_id (const char *plugin_name);
 void vl_api_send_pending_rpc_requests (vlib_main_t * vm);
 u8 *vl_api_serialize_message_table (api_main_t * am, u8 * vector);
diff --git a/src/vnet/CMakeLists.txt b/src/vnet/CMakeLists.txt
index 4b7972dd960..d237867fc1b 100644
--- a/src/vnet/CMakeLists.txt
+++ b/src/vnet/CMakeLists.txt
@@ -890,6 +890,7 @@ list(APPEND VNET_API_FILES lisp-gpe/lisp_gpe.api)
 ##############################################################################
 list(APPEND VNET_SOURCES
   dhcp/client.c
+  dhcp/dhcp4_packet.c
   dhcp/dhcp_client_detect.c
   dhcp/dhcp6_client_common_dp.c
   dhcp/dhcp6_pd_client_dp.c
diff --git a/src/vnet/dhcp/client.c b/src/vnet/dhcp/client.c
index aaeda96327d..5412dab3f7e 100644
--- a/src/vnet/dhcp/client.c
+++ b/src/vnet/dhcp/client.c
@@ -19,10 +19,17 @@
 #include <vnet/fib/fib_table.h>
 #include <vnet/qos/qos_types.h>
 
+vlib_log_class_t dhcp_logger;
+
 dhcp_client_main_t dhcp_client_main;
-static u8 *format_dhcp_client_state (u8 * s, va_list * va);
 static vlib_node_registration_t dhcp_client_process_node;
 
+#define DHCP_DBG(...)                           \
+    vlib_log_debug (dhcp_logger, __VA_ARGS__)
+
+#define DHCP_INFO(...)                          \
+    vlib_log_notice (dhcp_logger, __VA_ARGS__)
+
 #define foreach_dhcp_sent_packet_stat           \
 _(DISCOVER, "DHCP discover packets sent")       \
 _(OFFER, "DHCP offer packets sent")             \
@@ -52,15 +59,113 @@ static char *dhcp_client_process_stat_strings[] = {
     "DHCP unknown packets sent",
 };
 
+static u8 *
+format_dhcp_client_state (u8 * s, va_list * va)
+{
+  dhcp_client_state_t state = va_arg (*va, dhcp_client_state_t);
+  char *str = "BOGUS!";
+
+  switch (state)
+    {
+#define _(a)                                    \
+    case a:                                     \
+      str = #a;                                 \
+        break;
+      foreach_dhcp_client_state;
+#undef _
+    default:
+      break;
+    }
+
+  s = format (s, "%s", str);
+  return s;
+}
+
+static u8 *
+format_dhcp_client (u8 * s, va_list * va)
+{
+  dhcp_client_main_t *dcm = va_arg (*va, dhcp_client_main_t *);
+  dhcp_client_t *c = va_arg (*va, dhcp_client_t *);
+  int verbose = va_arg (*va, int);
+  ip4_address_t *addr;
+
+  s = format (s, "[%d] %U state %U installed %d", c - dcm->clients,
+	      format_vnet_sw_if_index_name, dcm->vnet_main, c->sw_if_index,
+	      format_dhcp_client_state, c->state, c->addresses_installed);
+
+  if (0 != c->dscp)
+    s = format (s, " dscp %d", c->dscp);
+
+  if (c->installed.leased_address.as_u32)
+    {
+      s = format (s, " addr %U/%d gw %U server %U",
+		  format_ip4_address, &c->installed.leased_address,
+		  c->installed.subnet_mask_width,
+		  format_ip4_address, &c->installed.router_address,
+		  format_ip4_address, &c->installed.dhcp_server);
+
+      vec_foreach (addr, c->domain_server_address)
+	s = format (s, " dns %U", format_ip4_address, addr);
+    }
+  else
+    {
+      s = format (s, " no address");
+    }
+
+  if (verbose)
+    {
+      s =
+	format (s,
+		"\n lease: lifetime:%d renewal-interval:%d expires:%.2f (now:%.2f)",
+		c->lease_lifetime, c->lease_renewal_interval,
+		c->lease_expires, vlib_time_now (dcm->vlib_main));
+      s =
+	format (s, "\n retry-count:%d, next-xmt:%.2f", c->retry_count,
+		c->next_transmit);
+      s = format (s, "\n broadcast adjacency:%d", c->ai_bcast);
+    }
+  return s;
+}
+
 static void
 dhcp_client_acquire_address (dhcp_client_main_t * dcm, dhcp_client_t * c)
 {
   /*
    * Install any/all info gleaned from dhcp, right here
    */
-  ip4_add_del_interface_address (dcm->vlib_main, c->sw_if_index,
-				 (void *) &c->leased_address,
-				 c->subnet_mask_width, 0 /*is_del */ );
+  if (!c->addresses_installed)
+    {
+      ip4_add_del_interface_address (dcm->vlib_main, c->sw_if_index,
+				     (void *) &c->learned.leased_address,
+				     c->learned.subnet_mask_width,
+				     0 /*is_del */ );
+      if (c->learned.router_address.as_u32)
+	{
+	  fib_prefix_t all_0s = {
+	    .fp_len = 0,
+	    .fp_proto = FIB_PROTOCOL_IP4,
+	  };
+	  ip46_address_t nh = {
+	    .ip4 = c->learned.router_address,
+	  };
+
+          /* *INDENT-OFF* */
+          fib_table_entry_path_add (
+              fib_table_get_index_for_sw_if_index (
+                  FIB_PROTOCOL_IP4,
+                  c->sw_if_index),
+              &all_0s,
+              FIB_SOURCE_DHCP,
+              FIB_ENTRY_FLAG_NONE,
+              DPO_PROTO_IP4,
+              &nh, c->sw_if_index,
+              ~0, 1, NULL,	// no label stack
+              FIB_ROUTE_PATH_FLAG_NONE);
+          /* *INDENT-ON* */
+	}
+    }
+  clib_memcpy (&c->installed, &c->learned, sizeof (c->installed));
+  c->addresses_installed = 1;
 }
 
 static void
@@ -70,10 +175,33 @@ dhcp_client_release_address (dhcp_client_main_t * dcm, dhcp_client_t * c)
    * Remove any/all info gleaned from dhcp, right here. Caller(s)
    * have not wiped out the info yet.
    */
+  if (c->addresses_installed)
+    {
+      ip4_add_del_interface_address (dcm->vlib_main, c->sw_if_index,
+				     (void *) &c->installed.leased_address,
+				     c->installed.subnet_mask_width,
+				     1 /*is_del */ );
+
+      /* Remove the default route */
+      if (c->installed.router_address.as_u32)
+	{
+	  fib_prefix_t all_0s = {
+	    .fp_len = 0,
+	    .fp_proto = FIB_PROTOCOL_IP4,
+	  };
+	  ip46_address_t nh = {
+	    .ip4 = c->installed.router_address,
+	  };
 
-  ip4_add_del_interface_address (dcm->vlib_main, c->sw_if_index,
-				 (void *) &c->leased_address,
-				 c->subnet_mask_width, 1 /*is_del */ );
+	  fib_table_entry_path_remove (fib_table_get_index_for_sw_if_index
+				       (FIB_PROTOCOL_IP4, c->sw_if_index),
+				       &all_0s, FIB_SOURCE_DHCP,
+				       DPO_PROTO_IP4, &nh, c->sw_if_index, ~0,
+				       1, FIB_ROUTE_PATH_FLAG_NONE);
+	}
+    }
+  clib_memset (&c->installed, 0, sizeof (c->installed));
+  c->addresses_installed = 0;
 }
 
 static void
@@ -86,9 +214,12 @@ dhcp_client_proc_callback (uword * client_index)
 }
 
 static void
-dhcp_client_addr_callback (dhcp_client_t * c)
+dhcp_client_addr_callback (u32 * cindex)
 {
   dhcp_client_main_t *dcm = &dhcp_client_main;
+  dhcp_client_t *c;
+
+  c = pool_elt_at_index (dcm->clients, *cindex);
 
   /* disable the feature */
   vnet_feature_enable_disable ("ip4-unicast",
@@ -96,48 +227,11 @@ dhcp_client_addr_callback (dhcp_client_t * c)
 			       c->sw_if_index, 0 /* disable */ , 0, 0);
   c->client_detect_feature_enabled = 0;
 
-  /* if renewing the lease, the address and route have already been added */
-  if (c->state == DHCP_BOUND)
-    return;
-
-  /* add the address to the interface */
-  dhcp_client_acquire_address (dcm, c);
-
-  /*
-   * Configure default IP route:
-   */
-  if (c->router_address.as_u32)
-    {
-      fib_prefix_t all_0s = {
-	.fp_len = 0,
-	.fp_addr.ip4.as_u32 = 0x0,
-	.fp_proto = FIB_PROTOCOL_IP4,
-      };
-      ip46_address_t nh = {
-	.ip4 = c->router_address,
-      };
-
-      /* *INDENT-OFF* */
-      fib_table_entry_path_add (
-	fib_table_get_index_for_sw_if_index (
-	  FIB_PROTOCOL_IP4,
-	  c->sw_if_index),
-	  &all_0s,
-	  FIB_SOURCE_DHCP,
-	  FIB_ENTRY_FLAG_NONE,
-          DPO_PROTO_IP4,
-          &nh, c->sw_if_index,
-          ~0, 1, NULL,	// no label stack
-          FIB_ROUTE_PATH_FLAG_NONE);
-      /* *INDENT-ON* */
-    }
-  if (c->dhcp_server.as_u32)
+  /* add the address to the interface if they've changed since the last time */
+  if (0 != clib_memcmp (&c->installed, &c->learned, sizeof (c->learned)))
     {
-      ip46_address_t dst = {
-	.ip4 = c->dhcp_server,
-      };
-      c->ai_ucast = adj_nbr_add_or_lock (FIB_PROTOCOL_IP4,
-					 VNET_LINK_IP4, &dst, c->sw_if_index);
+      dhcp_client_release_address (dcm, c);
+      dhcp_client_acquire_address (dcm, c);
     }
 
   /*
@@ -145,6 +239,28 @@ dhcp_client_addr_callback (dhcp_client_t * c)
    */
   if (c->event_callback)
     c->event_callback (c->client_index, c);
+
+  DHCP_INFO ("update: %U", format_dhcp_client, dcm, c, 1 /* verbose */ );
+}
+
+static void
+dhcp_client_reset (dhcp_client_main_t * dcm, dhcp_client_t * c)
+{
+  if (c->client_detect_feature_enabled == 1)
+    {
+      vnet_feature_enable_disable ("ip4-unicast",
+				   "ip4-dhcp-client-detect",
+				   c->sw_if_index, 0, 0, 0);
+      c->client_detect_feature_enabled = 0;
+    }
+
+  dhcp_client_release_address (dcm, c);
+  clib_memset (&c->learned, 0, sizeof (c->installed));
+  c->state = DHCP_DISCOVER;
+  c->next_transmit = vlib_time_now (dcm->vlib_main);
+  c->retry_count = 0;
+  c->lease_renewal_interval = 0;
+  vec_free (c->domain_server_address);
 }
 
 /*
@@ -194,9 +310,9 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
 
   /* parse through the packet, learn what we can */
   if (dhcp->your_ip_address.as_u32)
-    c->leased_address.as_u32 = dhcp->your_ip_address.as_u32;
+    c->learned.leased_address.as_u32 = dhcp->your_ip_address.as_u32;
 
-  c->dhcp_server.as_u32 = dhcp->server_ip_address.as_u32;
+  c->learned.dhcp_server.as_u32 = dhcp->server_ip_address.as_u32;
 
   o = (dhcp_option_t *) dhcp->options;
 
@@ -230,19 +346,19 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
 	  break;
 
 	case 54:		/* dhcp server address */
-	  c->dhcp_server.as_u32 = o->data_as_u32[0];
+	  c->learned.dhcp_server.as_u32 = o->data_as_u32[0];
 	  break;
 
 	case 1:		/* subnet mask */
 	  {
 	    u32 subnet_mask = clib_host_to_net_u32 (o->data_as_u32[0]);
-	    c->subnet_mask_width = count_set_bits (subnet_mask);
+	    c->learned.subnet_mask_width = count_set_bits (subnet_mask);
 	  }
 	  break;
 	case 3:		/* router address */
 	  {
 	    u32 router_address = o->data_as_u32[0];
-	    c->router_address.as_u32 = router_address;
+	    c->learned.router_address.as_u32 = router_address;
 	  }
 	  break;
 	case 6:		/* domain server address */
@@ -297,29 +413,9 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
 	{
 	  vlib_node_increment_counter (vm, dhcp_client_process_node.index,
 				       DHCP_STAT_NAK, 1);
-	  /* Probably never happens in bound state, but anyhow... */
-	  if (c->state == DHCP_BOUND)
-	    {
-	      ip4_add_del_interface_address (dcm->vlib_main, c->sw_if_index,
-					     (void *) &c->leased_address,
-					     c->subnet_mask_width,
-					     1 /*is_del */ );
-	      vnet_feature_enable_disable ("ip4-unicast",
-					   "ip4-dhcp-client-detect",
-					   c->sw_if_index, 1 /* enable */ ,
-					   0, 0);
-	      c->client_detect_feature_enabled = 1;
-	    }
-	  /* Wipe out any memory of the address we had... */
-	  c->state = DHCP_DISCOVER;
-	  c->next_transmit = now;
-	  c->retry_count = 0;
-	  c->leased_address.as_u32 = 0;
-	  c->subnet_mask_width = 0;
-	  c->router_address.as_u32 = 0;
-	  c->lease_renewal_interval = 0;
-	  c->dhcp_server.as_u32 = 0;
-	  vec_free (c->domain_server_address);
+	  /* Probably never happens in bound state, but anyhow...
+	     Wipe out any memory of the address we had... */
+	  dhcp_client_reset (dcm, c);
 	  break;
 	}
 
@@ -335,8 +431,13 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
 	  break;
 	}
       /* OK, we own the address (etc), add to the routing table(s) */
-      vl_api_rpc_call_main_thread (dhcp_client_addr_callback,
-				   (u8 *) c, sizeof (*c));
+      {
+	/* Send the index over to the main thread, where it can retrieve
+	 * the original client */
+	u32 cindex = c - dcm->clients;
+	vl_api_force_rpc_call_main_thread (dhcp_client_addr_callback,
+					   (u8 *) & cindex, sizeof (u32));
+      }
 
       c->state = DHCP_BOUND;
       c->retry_count = 0;
@@ -351,8 +452,7 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
       break;
     }
 
-  /* drop the pkt, return 1 */
-  vlib_buffer_free (vm, &bi, 1);
+  /* return 1 so the call disposes of this packet */
   return 1;
 }
 
@@ -377,6 +477,10 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
   u16 udp_length, ip_length;
   u32 counter_index, node_index;
 
+  DHCP_INFO ("send: type:%U bcast:%d %U",
+	     format_dhcp_packet_type, type,
+	     is_broadcast, format_dhcp_client, dcm, c, 1 /* verbose */ );
+
   /* Interface(s) down? */
   if ((hw->flags & VNET_HW_INTERFACE_FLAG_LINK_UP) == 0)
     return;
@@ -399,6 +503,7 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
   ASSERT (b->current_data == 0);
 
   vnet_buffer (b)->sw_if_index[VLIB_RX] = c->sw_if_index;
+  b->flags |= VNET_BUFFER_F_LOCALLY_ORIGINATED;
 
   if (is_broadcast)
     {
@@ -406,15 +511,7 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
       vnet_buffer (b)->ip.adj_index[VLIB_TX] = c->ai_bcast;
     }
   else
-    {
-      ip_adjacency_t *adj = adj_get (c->ai_ucast);
-
-      if (IP_LOOKUP_NEXT_ARP == adj->lookup_next_index)
-	node_index = ip4_arp_node.index;
-      else
-	node_index = ip4_rewrite_node.index;
-      vnet_buffer (b)->ip.adj_index[VLIB_TX] = c->ai_ucast;
-    }
+    node_index = dcm->ip4_lookup_node_index;
 
   /* Enqueue the packet right now */
   f = vlib_get_frame_to_node (vm, node_index);
@@ -456,8 +553,8 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
   else
     {
       /* Renewing an active lease, plain old ip4 src/dst */
-      ip->src_address.as_u32 = c->leased_address.as_u32;
-      ip->dst_address.as_u32 = c->dhcp_server.as_u32;
+      ip->src_address.as_u32 = c->learned.leased_address.as_u32;
+      ip->dst_address.as_u32 = c->learned.dhcp_server.as_u32;
     }
 
   udp->src_port = clib_host_to_net_u16 (UDP_DST_PORT_dhcp_to_client);
@@ -473,7 +570,7 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
 
   /* Lease renewal, set up client_ip_address */
   if (is_broadcast == 0)
-    dhcp->client_ip_address.as_u32 = c->leased_address.as_u32;
+    dhcp->client_ip_address.as_u32 = c->learned.leased_address.as_u32;
 
   dhcp->opcode = 1;		/* request, all we send */
   dhcp->hardware_type = 1;	/* ethernet */
@@ -508,20 +605,20 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
    * If server ip address is available with non-zero value,
    * option 54 (DHCP Server Identifier) is sent.
    */
-  if (c->dhcp_server.as_u32)
+  if (c->learned.dhcp_server.as_u32)
     {
       o->option = 54;
       o->length = 4;
-      clib_memcpy (o->data, &c->dhcp_server.as_u32, 4);
+      clib_memcpy (o->data, &c->learned.dhcp_server.as_u32, 4);
       o = (dhcp_option_t *) (((uword) o) + (o->length + 2));
     }
 
   /* send option 50, requested IP address */
-  if (c->leased_address.as_u32)
+  if (c->learned.leased_address.as_u32)
     {
       o->option = 50;
       o->length = 4;
-      clib_memcpy (o->data, &c->leased_address.as_u32, 4);
+      clib_memcpy (o->data, &c->learned.leased_address.as_u32, 4);
       o = (dhcp_option_t *) (((uword) o) + (o->length + 2));
     }
 
@@ -538,8 +635,10 @@ send_dhcp_pkt (dhcp_client_main_t * dcm, dhcp_client_t * c,
   if (vec_len (c->client_identifier))
     {
       o->option = 61;
-      o->length = vec_len (c->client_identifier);
-      clib_memcpy (o->data, c->client_identifier,
+      o->length = vec_len (c->client_identifier) + 1;
+      /* Set type to zero, apparently some dhcp servers care */
+      o->data[0] = 0;
+      clib_memcpy (o->data + 1, c->client_identifier,
 		   vec_len (c->client_identifier));
       o = (dhcp_option_t *) (((uword) o) + (o->length + 2));
     }
@@ -594,7 +693,15 @@ dhcp_discover_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
    * State machine "DISCOVER" state. Send a dhcp discover packet,
    * eventually back off the retry rate.
    */
-
+  /*
+   * In order to accept any OFFER, whether broadcasted or unicasted, we
+   * need to configure the dhcp-client-detect feature as an input feature
+   * so the DHCP OFFER is sent to the ip4-local node. Without this a
+   * broadcasted OFFER hits the 255.255.255.255/32 address and a unicast
+   * hits 0.0.0.0/0 both of which default to drop and the latter may forward
+   * of box - not what we want. Nor to we want to change these route for
+   * all interfaces in this table
+   */
   if (c->client_detect_feature_enabled == 0)
     {
       vnet_feature_enable_disable ("ip4-unicast",
@@ -620,6 +727,9 @@ dhcp_request_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
    * State machine "REQUEST" state. Send a dhcp request packet,
    * eventually drop back to the discover state.
    */
+  DHCP_INFO ("enter request: %U", format_dhcp_client, dcm, c,
+	     1 /*verbose */ );
+
   send_dhcp_pkt (dcm, c, DHCP_PACKET_REQUEST, 1 /* is_broadcast */ );
 
   c->retry_count++;
@@ -638,13 +748,6 @@ static int
 dhcp_bound_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
 {
   /*
-   * State machine "BOUND" state. Send a dhcp request packet to renew
-   * the lease.
-   * Eventually, when the lease expires, forget the dhcp data
-   * and go back to the stone age.
-   */
-
-  /*
    * We disable the client detect feature when we bind a
    * DHCP address. Turn it back on again on first renew attempt.
    * Otherwise, if the DHCP server replies we'll never see it.
@@ -657,6 +760,24 @@ dhcp_bound_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
       c->client_detect_feature_enabled = 1;
     }
 
+  /*
+   * State machine "BOUND" state. Send a dhcp request packet to renew
+   * the lease.
+   * Eventually, when the lease expires, forget the dhcp data
+   * and go back to the stone age.
+   */
+  if (now > c->lease_expires)
+    {
+      DHCP_INFO ("lease expired: %U", format_dhcp_client, dcm, c,
+		 1 /*verbose */ );
+
+      /* reset all data for the client. do not send any more messages
+       * since the objects to do so have been lost */
+      dhcp_client_reset (dcm, c);
+      return 1;
+    }
+
+  DHCP_INFO ("enter bound: %U", format_dhcp_client, dcm, c, 1 /* verbose */ );
   send_dhcp_pkt (dcm, c, DHCP_PACKET_REQUEST, 0 /* is_broadcast */ );
 
   c->retry_count++;
@@ -665,39 +786,6 @@ dhcp_bound_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
   else
     c->next_transmit = now + 1.0;
 
-  if (now > c->lease_expires)
-    {
-      /* Remove the default route */
-      if (c->router_address.as_u32)
-	{
-	  fib_prefix_t all_0s = {
-	    .fp_len = 0,
-	    .fp_addr.ip4.as_u32 = 0x0,
-	    .fp_proto = FIB_PROTOCOL_IP4,
-	  };
-	  ip46_address_t nh = {
-	    .ip4 = c->router_address,
-	  };
-
-	  fib_table_entry_path_remove (fib_table_get_index_for_sw_if_index
-				       (FIB_PROTOCOL_IP4, c->sw_if_index),
-				       &all_0s, FIB_SOURCE_DHCP,
-				       DPO_PROTO_IP4, &nh, c->sw_if_index, ~0,
-				       1, FIB_ROUTE_PATH_FLAG_NONE);
-	}
-      /* Remove the interface address */
-      dhcp_client_release_address (dcm, c);
-      c->state = DHCP_DISCOVER;
-      c->next_transmit = now;
-      c->retry_count = 0;
-      /* Wipe out any memory of the address we had... */
-      c->leased_address.as_u32 = 0;
-      c->subnet_mask_width = 0;
-      c->router_address.as_u32 = 0;
-      c->lease_renewal_interval = 0;
-      c->dhcp_server.as_u32 = 0;
-      return 1;
-    }
   return 0;
 }
 
@@ -715,7 +803,9 @@ dhcp_client_sm (f64 now, f64 timeout, uword pool_index)
 
   /* Time for us to do something with this client? */
   if (now < c->next_transmit)
-    return timeout;
+    return c->next_transmit;
+
+  DHCP_INFO ("sm active session %d", c - dcm->clients);
 
 again:
   switch (c->state)
@@ -741,17 +831,15 @@ again:
       break;
     }
 
-  if (c->next_transmit < now + timeout)
-    return c->next_transmit - now;
-
-  return timeout;
+  return c->next_transmit;
 }
 
 static uword
 dhcp_client_process (vlib_main_t * vm,
 		     vlib_node_runtime_t * rt, vlib_frame_t * f)
 {
-  f64 timeout = 100.0;
+  f64 timeout = 1000.0;
+  f64 next_expire_time, this_next_expire_time;
   f64 now;
   uword event_type;
   uword *event_data = 0;
@@ -771,19 +859,32 @@ dhcp_client_process (vlib_main_t * vm,
 	{
 	case EVENT_DHCP_CLIENT_WAKEUP:
 	  for (i = 0; i < vec_len (event_data); i++)
-	    timeout = dhcp_client_sm (now, timeout, event_data[i]);
-	  break;
+	    (void) dhcp_client_sm (now, timeout, event_data[i]);
+	  /* FALLTHROUGH */
 
 	case ~0:
-          /* *INDENT-OFF* */
-	  pool_foreach (c, dcm->clients,
-          ({
-            timeout = dhcp_client_sm (now, timeout,
-                                      (uword) (c - dcm->clients));
-          }));
-          /* *INDENT-ON* */
-	  if (pool_elts (dcm->clients) == 0)
-	    timeout = 100.0;
+	  if (pool_elts (dcm->clients))
+	    {
+              /* *INDENT-OFF* */
+              next_expire_time = 1e70;
+              pool_foreach (c, dcm->clients,
+              ({
+                this_next_expire_time = dhcp_client_sm
+                  (now, timeout, (uword) (c - dcm->clients));
+                next_expire_time = this_next_expire_time < next_expire_time ?
+                  this_next_expire_time : next_expire_time;
+              }));
+              if (next_expire_time > now)
+                timeout = next_expire_time - now;
+              else
+                {
+                  clib_warning ("BUG");
+                  timeout = 1.13;
+                }
+              /* *INDENT-ON* */
+	    }
+	  else
+	    timeout = 1000.0;
 	  break;
 	}
 
@@ -805,75 +906,6 @@ VLIB_REGISTER_NODE (dhcp_client_process_node,static) = {
 };
 /* *INDENT-ON* */
 
-static u8 *
-format_dhcp_client_state (u8 * s, va_list * va)
-{
-  dhcp_client_state_t state = va_arg (*va, dhcp_client_state_t);
-  char *str = "BOGUS!";
-
-  switch (state)
-    {
-#define _(a)                                    \
-    case a:                                     \
-      str = #a;                                 \
-        break;
-      foreach_dhcp_client_state;
-#undef _
-    default:
-      break;
-    }
-
-  s = format (s, "%s", str);
-  return s;
-}
-
-static u8 *
-format_dhcp_client (u8 * s, va_list * va)
-{
-  dhcp_client_main_t *dcm = va_arg (*va, dhcp_client_main_t *);
-  dhcp_client_t *c = va_arg (*va, dhcp_client_t *);
-  int verbose = va_arg (*va, int);
-  ip4_address_t *addr;
-
-  s = format (s, "[%d] %U state %U ", c - dcm->clients,
-	      format_vnet_sw_if_index_name, dcm->vnet_main, c->sw_if_index,
-	      format_dhcp_client_state, c->state);
-
-  if (0 != c->dscp)
-    s = format (s, "dscp %d ", c->dscp);
-
-  if (c->leased_address.as_u32)
-    {
-      s = format (s, "addr %U/%d gw %U",
-		  format_ip4_address, &c->leased_address,
-		  c->subnet_mask_width, format_ip4_address,
-		  &c->router_address);
-
-      vec_foreach (addr, c->domain_server_address)
-	s = format (s, " dns %U", format_ip4_address, addr);
-    }
-  else
-    {
-      s = format (s, "no address\n");
-    }
-
-  if (verbose)
-    {
-      s =
-	format (s,
-		"\n lease: lifetime:%d renewal-interval:%d expires:%.2f (now:%.2f)",
-		c->lease_lifetime, c->lease_renewal_interval,
-		c->lease_expires, vlib_time_now (dcm->vlib_main));
-      s =
-	format (s, "\n retry-count:%d, next-xmt:%.2f", c->retry_count,
-		c->next_transmit);
-      s =
-	format (s, "\n adjacencies:[unicast:%d broadcast:%d]", c->ai_ucast,
-		c->ai_bcast);
-    }
-  return s;
-}
-
 static clib_error_t *
 show_dhcp_client_command_fn (vlib_main_t * vm,
 			     unformat_input_t * input,
@@ -934,11 +966,6 @@ dhcp_client_add_del (dhcp_client_add_del_args_t * a)
   vlib_main_t *vm = dcm->vlib_main;
   dhcp_client_t *c;
   uword *p;
-  fib_prefix_t all_0s = {
-    .fp_len = 0,
-    .fp_addr.ip4.as_u32 = 0x0,
-    .fp_proto = FIB_PROTOCOL_IP4,
-  };
 
   p = hash_get (dcm->client_by_sw_if_index, a->sw_if_index);
 
@@ -960,7 +987,6 @@ dhcp_client_add_del (dhcp_client_add_del_args_t * a)
       c->client_identifier = a->client_identifier;
       c->set_broadcast_flag = a->set_broadcast_flag;
       c->dscp = a->dscp;
-      c->ai_ucast = ADJ_INDEX_INVALID;
       c->ai_bcast = adj_nbr_add_or_lock (FIB_PROTOCOL_IP4,
 					 VNET_LINK_IP4,
 					 &ADJ_BCAST_ADDR, c->sw_if_index);
@@ -973,42 +999,17 @@ dhcp_client_add_del (dhcp_client_add_del_args_t * a)
 
       hash_set (dcm->client_by_sw_if_index, a->sw_if_index, c - dcm->clients);
 
-      /*
-       * In order to accept any OFFER, whether broadcasted or unicasted, we
-       * need to configure the dhcp-client-detect feature as an input feature
-       * so the DHCP OFFER is sent to the ip4-local node. Without this a
-       * broadcasted OFFER hits the 255.255.255.255/32 address and a unicast
-       * hits 0.0.0.0/0 both of which default to drop and the latter may forward
-       * of box - not what we want. Nor to we want to change these route for
-       * all interfaces in this table
-       */
-      vnet_feature_enable_disable ("ip4-unicast",
-				   "ip4-dhcp-client-detect",
-				   c->sw_if_index, 1 /* enable */ , 0, 0);
-      c->client_detect_feature_enabled = 1;
-
       vlib_process_signal_event (vm, dhcp_client_process_node.index,
 				 EVENT_DHCP_CLIENT_WAKEUP, c - dcm->clients);
+
+      DHCP_INFO ("create: %U", format_dhcp_client, dcm, c, 1 /* verbose */ );
     }
   else
     {
       c = pool_elt_at_index (dcm->clients, p[0]);
 
-      if (c->router_address.as_u32)
-	{
-	  ip46_address_t nh = {
-	    .ip4 = c->router_address,
-	  };
+      dhcp_client_reset (dcm, c);
 
-	  fib_table_entry_path_remove (fib_table_get_index_for_sw_if_index
-				       (FIB_PROTOCOL_IP4, c->sw_if_index),
-				       &all_0s, FIB_SOURCE_DHCP,
-				       DPO_PROTO_IP4, &nh, c->sw_if_index, ~0,
-				       1, FIB_ROUTE_PATH_FLAG_NONE);
-	}
-      dhcp_client_release_address (dcm, c);
-
-      adj_unlock (c->ai_ucast);
       adj_unlock (c->ai_bcast);
 
       vec_free (c->domain_server_address);
@@ -1094,15 +1095,13 @@ dhcp_client_config (u32 is_add,
       vec_free (a->option_55_data);
 
       if (is_add)
-	clib_warning ("dhcp client already enabled on intf_idx %d",
-		      sw_if_index);
+	DHCP_INFO ("dhcp client already enabled on intf_idx %d", sw_if_index);
       else
-	clib_warning ("dhcp client not enabled on on intf_idx %d",
-		      sw_if_index);
+	DHCP_INFO ("not enabled on on intf_idx %d", sw_if_index);
       break;
 
     default:
-      clib_warning ("dhcp_client_add_del returned %d", rv);
+      DHCP_INFO ("dhcp_client_add_del returned %d", rv);
     }
 
   return rv;
@@ -1161,7 +1160,7 @@ dhcp_client_set_command_fn (vlib_main_t * vm,
   a->is_add = is_add;
   a->sw_if_index = sw_if_index;
   a->hostname = hostname;
-  a->client_identifier = format (0, "vpe 1.0%c", 0);
+  a->client_identifier = format (0, "vpp 1.1%c", 0);
   a->set_broadcast_flag = set_broadcast_flag;
 
   /*
@@ -1237,10 +1236,22 @@ static clib_error_t *
 dhcp_client_init (vlib_main_t * vm)
 {
   dhcp_client_main_t *dcm = &dhcp_client_main;
+  vlib_node_t *ip4_lookup_node;
+
+  ip4_lookup_node = vlib_get_node_by_name (vm, (u8 *) "ip4-lookup");
 
+  /* Should never happen... */
+  if (ip4_lookup_node == 0)
+    return clib_error_return (0, "ip4-lookup node not found");
+
+  dcm->ip4_lookup_node_index = ip4_lookup_node->index;
   dcm->vlib_main = vm;
   dcm->vnet_main = vnet_get_main ();
   dcm->seed = (u32) clib_cpu_time_now ();
+
+  dhcp_logger = vlib_log_register_class ("dhcp", "client");
+  DHCP_INFO ("plugin initialized");
+
   return 0;
 }
 
diff --git a/src/vnet/dhcp/client.h b/src/vnet/dhcp/client.h
index 5191fcf0fa8..c236a0da74e 100644
--- a/src/vnet/dhcp/client.h
+++ b/src/vnet/dhcp/client.h
@@ -42,6 +42,24 @@ struct dhcp_client_t_;
 typedef void (*dhcp_event_cb_t) (u32 client_index,
 				 const struct dhcp_client_t_ * client);
 
+/**
+ * The set of addresses/mask that contribute forwarding info
+ * and are installed.
+ */
+typedef struct dhcp_client_fwd_addresses_t_
+{
+  /** the address assigned to this client and it's mask */
+  ip4_address_t leased_address;
+  u32 subnet_mask_width;
+
+  /** the address of the DHCP server handing out the address.
+      this is used to send any unicast messages */
+  ip4_address_t dhcp_server;
+
+  /** The address of this client's default gateway - may not be present */
+  ip4_address_t router_address;
+} dhcp_client_fwd_addresses_t;
+
 typedef struct dhcp_client_t_
 {
   dhcp_client_state_t state;
@@ -59,11 +77,16 @@ typedef struct dhcp_client_t_
   /* DHCP transaction ID, a random number */
   u32 transaction_id;
 
-  /* leased address, other learned info DHCP */
-  ip4_address_t leased_address;	/* from your_ip_address field */
-  ip4_address_t dhcp_server;
-  u32 subnet_mask_width;	/* option 1 */
-  ip4_address_t router_address;	/* option 3 */
+  /**
+   * leased address, other learned info DHCP
+   *  the learned set is updated by new messages recieved in the DP
+   *  the installed set is what's actually been added
+   */
+  dhcp_client_fwd_addresses_t learned;
+  dhcp_client_fwd_addresses_t installed;
+  /* have local Addresses and default route been installed */
+  u8 addresses_installed;
+
   ip4_address_t *domain_server_address;	/* option 6 */
   u32 lease_renewal_interval;	/* option 51 */
   u32 lease_lifetime;		/* option 59 */
@@ -85,8 +108,6 @@ typedef struct dhcp_client_t_
   u8 client_hardware_address[6];
   u8 client_detect_feature_enabled;
 
-  /* the unicast adjacency for the DHCP server */
-  adj_index_t ai_ucast;
   /* the broadcast adjacency on the link */
   adj_index_t ai_bcast;
   /* IP DSCP to set in sent packets */
@@ -102,6 +123,9 @@ typedef struct
   uword *client_by_sw_if_index;
   u32 seed;
 
+  /* ip4-lookup node index */
+  u32 ip4_lookup_node_index;
+
   /* convenience */
   vlib_main_t *vlib_main;
   vnet_main_t *vnet_main;
diff --git a/src/vnet/dhcp/dhcp4_packet.c b/src/vnet/dhcp/dhcp4_packet.c
new file mode 100644
index 00000000000..7354e56e4d1
--- /dev/null
+++ b/src/vnet/dhcp/dhcp4_packet.c
@@ -0,0 +1,122 @@
+/*
+ * dhcp4_packet.c: dhcp packet format functions
+ *
+ * Copyright (c) 2013 Cisco and/or its affiliates.
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <vnet/dhcp/dhcp4_packet.h>
+#include <vnet/ip/format.h>
+
+u8 *
+format_dhcp_packet_type (u8 * s, va_list * args)
+{
+  dhcp_packet_type_t pt = va_arg (*args, dhcp_packet_type_t);
+
+  switch (pt)
+    {
+    case DHCP_PACKET_DISCOVER:
+      s = format (s, "discover");
+      break;
+    case DHCP_PACKET_OFFER:
+      s = format (s, "offer");
+      break;
+    case DHCP_PACKET_REQUEST:
+      s = format (s, "request");
+      break;
+    case DHCP_PACKET_ACK:
+      s = format (s, "ack");
+      break;
+    case DHCP_PACKET_NAK:
+      s = format (s, "nack");
+      break;
+    }
+  return (s);
+}
+
+u8 *
+format_dhcp_header (u8 * s, va_list * args)
+{
+  dhcp_header_t *d = va_arg (*args, dhcp_header_t *);
+  u32 max_bytes = va_arg (*args, u32);
+  dhcp_option_t *o;
+  u32 tmp;
+
+  s = format (s, "opcode:%s", (d->opcode == 1 ? "request" : "reply"));
+  s = format (s, " hw[type:%d addr-len:%d addr:%U]",
+	      d->hardware_type, d->hardware_address_length,
+	      format_hex_bytes, d->client_hardware_address,
+	      d->hardware_address_length);
+  s = format (s, " hops%d", d->hops);
+  s = format (s, " transaction-ID:0x%x", d->transaction_identifier);
+  s = format (s, " seconds:%d", d->seconds);
+  s = format (s, " flags:0x%x", d->flags);
+  s = format (s, " client:%U", format_ip4_address, &d->client_ip_address);
+  s = format (s, " your:%U", format_ip4_address, &d->your_ip_address);
+  s = format (s, " server:%U", format_ip4_address, &d->server_ip_address);
+  s = format (s, " gateway:%U", format_ip4_address, &d->gateway_ip_address);
+  s = format (s, " cookie:%U", format_ip4_address, &d->magic_cookie);
+
+  o = (dhcp_option_t *) d->options;
+
+  while (o->option != 0xFF /* end of options */  &&
+	 (u8 *) o < (u8 *) d + max_bytes)
+    {
+      switch (o->option)
+	{
+	case 53:		/* dhcp message type */
+	  tmp = o->data[0];
+	  s =
+	    format (s, ", option-53: type:%U", format_dhcp_packet_type, tmp);
+	  break;
+	case 54:		/* dhcp server address */
+	  s = format (s, ", option-54: server:%U",
+		      format_ip4_address, &o->data_as_u32[0]);
+	  break;
+	case 58:		/* lease renew time in seconds */
+	  s = format (s, ", option-58: renewal:%d",
+		      clib_host_to_net_u32 (o->data_as_u32[0]));
+	  break;
+	case 1:		/* subnet mask */
+	  s = format (s, ", option-1: subnet-mask:%d",
+		      clib_host_to_net_u32 (o->data_as_u32[0]));
+	  break;
+	case 3:		/* router address */
+	  s = format (s, ", option-3: router:%U",
+		      format_ip4_address, &o->data_as_u32[0]);
+	  break;
+	case 6:		/* domain server address */
+	  s = format (s, ", option-6: domian-server:%U",
+		      format_hex_bytes, o->data, o->length);
+	  break;
+	case 12:		/* hostname */
+	  s = format (s, ", option-12: hostname:%U",
+		      format_hex_bytes, o->data, o->length);
+	  break;
+	default:
+	  tmp = o->option;
+	  s = format (s, " option-%d: skipped", tmp);
+	  break;
+	}
+      o = (dhcp_option_t *) (((u8 *) o) + (o->length + 2));
+    }
+  return (s);
+}
+
+/*
+ * fd.io coding-style-patch-verification: ON
+ *
+ * Local Variables:
+ * eval: (c-set-style "gnu")
+ * End:
+ */
diff --git a/src/vnet/dhcp/dhcp4_packet.h b/src/vnet/dhcp/dhcp4_packet.h
index 3076dd9529d..abd60127d6c 100644
--- a/src/vnet/dhcp/dhcp4_packet.h
+++ b/src/vnet/dhcp/dhcp4_packet.h
@@ -51,6 +51,8 @@ typedef struct
   dhcp_option_t options[0];
 } dhcp_header_t;
 
+extern u8 *format_dhcp_header (u8 * s, va_list * args);
+
 typedef enum
 {
   DHCP_PACKET_DISCOVER = 1,
@@ -60,6 +62,8 @@ typedef enum
   DHCP_PACKET_NAK,
 } dhcp_packet_type_t;
 
+extern u8 *format_dhcp_packet_type (u8 * s, va_list * args);
+
 typedef enum dhcp_packet_option_t_
 {
   DHCP_PACKET_OPTION_MSG_TYPE = 53,
diff --git a/src/vnet/dhcp/dhcp_api.c b/src/vnet/dhcp/dhcp_api.c
index 7935ad8ba01..fffce78e6cc 100644
--- a/src/vnet/dhcp/dhcp_api.c
+++ b/src/vnet/dhcp/dhcp_api.c
@@ -228,10 +228,12 @@ dhcp_client_lease_encode (vl_api_dhcp_lease_t * lease,
   clib_memcpy (&lease->hostname, client->hostname, len);
   lease->hostname[len] = 0;
 
-  lease->mask_width = client->subnet_mask_width;
-  clib_memcpy (&lease->host_address[0], (u8 *) & client->leased_address,
+  lease->mask_width = client->installed.subnet_mask_width;
+  clib_memcpy (&lease->host_address[0],
+	       (u8 *) & client->installed.leased_address,
 	       sizeof (ip4_address_t));
-  clib_memcpy (&lease->router_address[0], (u8 *) & client->router_address,
+  clib_memcpy (&lease->router_address[0],
+	       (u8 *) & client->installed.router_address,
 	       sizeof (ip4_address_t));
 
   lease->count = vec_len (client->domain_server_address);
diff --git a/test/test_dhcp.py b/test/test_dhcp.py
index 175f6493eb7..5d9c89d5f38 100644
--- a/test/test_dhcp.py
+++ b/test/test_dhcp.py
@@ -242,6 +242,7 @@ class TestDHCP(VppTestCase):
         self.verify_dhcp_msg_type(pkt, "discover")
         self.verify_dhcp_has_option(pkt, "hostname", hostname)
         if client_id:
+            client_id = '\x00' + client_id
             self.verify_dhcp_has_option(pkt, "client_id", client_id)
         bootp = pkt[BOOTP]
         self.assertEqual(bootp.ciaddr, "0.0.0.0")