dhcp: always clean up DHCP client watch callback when stopping DHCP

Also fix a bug where failure to start the DHCP client wouldn't be
handled.

src/dhcp-manager/nm-dhcp-dhclient.c

@@ -215,14 +215,13 @@ dhclient_child_setup (gpointer user_data G_GNUC_UNUSED)
 }
 
 
-gboolean
+GPid
 nm_dhcp_client_start (NMDHCPDevice *device, NMSettingIP4Config *s_ip4)
 {
-	GPtrArray *		dhclient_argv = NULL;
+	GPtrArray *dhclient_argv = NULL;
-	GPid			pid;
+	GPid pid = 0;
-	GError *		error = NULL;
+	GError *error = NULL;
-	gboolean		success = FALSE;
+	char *pid_contents = NULL;
-	char *			pid_contents = NULL;
 
 	if (!g_file_test (DHCP_CLIENT_PATH, G_FILE_TEST_EXISTS)) {
 		nm_warning (DHCP_CLIENT_PATH " does not exist.");
@@ -249,7 +248,7 @@ nm_dhcp_client_start (NMDHCPDevice *device, NMSettingIP4Config *s_ip4)
 		unsigned long int tmp = strtoul (pid_contents, NULL, 10);
 
 		if (!((tmp == ULONG_MAX) && (errno == ERANGE)))
-			nm_dhcp_client_stop (device->iface, (pid_t) tmp);
+			nm_dhcp_client_stop (device, (pid_t) tmp);
 		remove (device->pid_file);
 	}
 
@@ -282,13 +281,10 @@ nm_dhcp_client_start (NMDHCPDevice *device, NMSettingIP4Config *s_ip4)
 
 	nm_info ("dhclient started with pid %d", pid);
 
-	device->pid = pid;
-	success = TRUE;
-
 out:
 	g_free (pid_contents);
 	g_ptr_array_free (dhclient_argv, TRUE);
-	return success;
+	return pid;
 }
 
 static const char **

src/dhcp-manager/nm-dhcp-dhcpcd.c

@@ -59,14 +59,13 @@ dhcpcd_child_setup (gpointer user_data G_GNUC_UNUSED)
 }
 
 
-gboolean
+GPid
 nm_dhcp_client_start (NMDHCPDevice *device, NMSettingIP4Config *s_ip4)
 {
-	GPtrArray *		argv = NULL;
+	GPtrArray *argv = NULL;
-	GPid			pid;
+	GPid pid = 0;
-	GError *		error = NULL;
+	GError *error = NULL;
-	gboolean		success = FALSE;
+	char *pid_contents = NULL;
-	char *			pid_contents = NULL;
 
 	if (!g_file_test (DHCP_CLIENT_PATH, G_FILE_TEST_EXISTS)) {
 		nm_warning (DHCP_CLIENT_PATH " does not exist.");
@@ -84,7 +83,7 @@ nm_dhcp_client_start (NMDHCPDevice *device, NMSettingIP4Config *s_ip4)
 		unsigned long int tmp = strtoul (pid_contents, NULL, 10);
 
 		if (!((tmp == ULONG_MAX) && (errno == ERANGE)))
-			nm_dhcp_client_stop (device->iface, (pid_t) tmp);
+			nm_dhcp_client_stop (device, (pid_t) tmp);
 		remove (device->pid_file);
 	}
 
@@ -112,13 +111,10 @@ nm_dhcp_client_start (NMDHCPDevice *device, NMSettingIP4Config *s_ip4)
 
 	nm_info ("dhcpcd started with pid %d", pid);
 
-	device->pid = pid;
-	success = TRUE;
-
 out:
 	g_free (pid_contents);
 	g_ptr_array_free (argv, TRUE);
-	return success;
+	return pid;
 }
 
 gboolean

src/dhcp-manager/nm-dhcp-manager.c

@@ -172,10 +172,9 @@ nm_dhcp_device_destroy (NMDHCPDevice *device)
 	int ret;
 
 	nm_dhcp_device_timeout_cleanup (device);
-	nm_dhcp_device_watch_cleanup (device);
 
 	if (device->pid)
-		nm_dhcp_client_stop (device->iface, device->pid);
+		nm_dhcp_client_stop (device, device->pid);
 
 	if (device->options)
 		g_hash_table_destroy (device->options);
@@ -577,9 +576,9 @@ static void dhcp_watch_cb (GPid pid, gint status, gpointer user_data)
 
 gboolean
 nm_dhcp_manager_begin_transaction (NMDHCPManager *manager,
-								   const char *iface,
+                                   const char *iface,
-								   NMSettingIP4Config *s_ip4,
+                                   NMSettingIP4Config *s_ip4,
-								   guint32 timeout)
+                                   guint32 timeout)
 {
 	NMDHCPManagerPrivate *priv;
 	NMDHCPDevice *device;
@@ -593,13 +592,17 @@ nm_dhcp_manager_begin_transaction (NMDHCPManager *manager,
 	if (!device)
 		device = nm_dhcp_device_new (manager, iface);
 
-	if (state_is_bound (device->state) || (device->state == DHC_START)) {
+	if (device->pid && (device->state < DHC_ABEND)) {
 		/* Cancel any DHCP transaction already in progress */
 		nm_dhcp_manager_cancel_transaction_real (device);
 	}
 
 	nm_info ("Activation (%s) Beginning DHCP transaction.", iface);
 
+	device->pid = nm_dhcp_client_start (device, s_ip4);
+	if (device->pid == 0)
+		return FALSE;
+
 	if (timeout == 0)
 		timeout = NM_DHCP_TIMEOUT;
 
@@ -607,8 +610,6 @@ nm_dhcp_manager_begin_transaction (NMDHCPManager *manager,
 	device->timeout_id = g_timeout_add_seconds (timeout,
 	                                            nm_dhcp_manager_handle_timeout,
 	                                            device);
-
-	nm_dhcp_client_start (device, s_ip4);
 	device->watch_id = g_child_watch_add (device->pid,
 					      (GChildWatchFunc) dhcp_watch_cb,
 					      device);
@@ -616,10 +617,17 @@ nm_dhcp_manager_begin_transaction (NMDHCPManager *manager,
 }
 
 void
-nm_dhcp_client_stop (const char * iface, pid_t pid)
+nm_dhcp_client_stop (NMDHCPDevice *device, pid_t pid)
 {
 	int i = 15; /* 3 seconds */
 
+	g_return_if_fail (pid > 0);
+
+	/* Clean up the watch handler since we're explicitly killing
+	 * the daemon
+	 */
+	nm_dhcp_device_watch_cleanup (device);
+
 	/* Tell it to quit; maybe it wants to send out a RELEASE message */
 	kill (pid, SIGTERM);
 
@@ -643,7 +651,7 @@ nm_dhcp_client_stop (const char * iface, pid_t pid)
 	}
 
 	if (i <= 0) {
-		nm_warning ("%s: dhcp client pid %d didn't exit, will kill it.", iface, pid);
+		nm_warning ("%s: dhcp client pid %d didn't exit, will kill it.", device->iface, pid);
 		kill (pid, SIGKILL);
 
 		nm_debug ("waiting for dhcp client pid %d to exit", pid);
@@ -655,10 +663,9 @@ nm_dhcp_client_stop (const char * iface, pid_t pid)
 static void
 nm_dhcp_manager_cancel_transaction_real (NMDHCPDevice *device)
 {
-	if (!device->pid)
+	g_return_if_fail (device->pid > 0);
-		return;
 
-	nm_dhcp_client_stop (device->iface, device->pid);
+	nm_dhcp_client_stop (device, device->pid);
 
 	nm_info ("%s: canceled DHCP transaction, dhcp client pid %d",
 	         device->iface,
@@ -688,7 +695,6 @@ nm_dhcp_manager_cancel_transaction_real (NMDHCPDevice *device)
 		device->conf_file = NULL;
 	}
 
-	nm_dhcp_device_watch_cleanup (device);
 	nm_dhcp_device_timeout_cleanup (device);
 	g_hash_table_remove_all (device->options);
 }

src/dhcp-manager/nm-dhcp-manager.h

@@ -102,8 +102,8 @@ gboolean       nm_dhcp_manager_foreach_dhcp4_option (NMDHCPManager *self,
                                                      gpointer user_data);
 
 /* The following are implemented by the DHCP client backends */
-gboolean       nm_dhcp_client_start                 (NMDHCPDevice *device, NMSettingIP4Config *s_ip4);
+GPid           nm_dhcp_client_start                 (NMDHCPDevice *device, NMSettingIP4Config *s_ip4);
-void           nm_dhcp_client_stop                  (const char *iface, pid_t pid);
+void           nm_dhcp_client_stop                  (NMDHCPDevice *device, pid_t pid);
 
 gboolean       nm_dhcp_client_process_classless_routes (GHashTable *options,
                                                         NMIP4Config *ip4_config,