dhcp: dhclient: remove the --timeout argument from the command linefg/dhcp_timeout_rh1491243

the --timeout command line option is a custom feature added in some
linux distributions (fedora). Passing that command line argument will
make dhclient fail if the binary does not support it, preventing
activation of dhcp based connections.
Worse, the option has just been recently changed from "-timeout", so
that we are currently incompatibile with Centos, RedHat and older
versions of Fedora too.

Leverage the "timeout" option in dhclient config file: it will produce
the expected behavior and will be universally supported.

Fixes test: dhcp-timeout
Fixes: fa46736013fa1e3df1508b1f67b495ce45daf94a

https://bugzilla.redhat.com/show_bug.cgi?id=1491243

4 files changed, 57 insertions, 39 deletions

diff --git a/src/dhcp/nm-dhcp-dhclient-utils.c b/src/dhcp/nm-dhcp-dhclient-utils.c
index 9ae355ece1..309dbc6cee 100644
--- a/src/dhcp/nm-dhcp-dhclient-utils.c
+++ b/src/dhcp/nm-dhcp-dhclient-utils.c
@@ -33,7 +33,9 @@
 #include "platform/nm-platform.h"
 #include "NetworkManagerUtils.h"
 
-#define CLIENTID_TAG            "send dhcp-client-identifier"
+#define TIMEOUT_TAG      "timeout"
+#define RETRY_TAG        "retry"
+#define CLIENTID_TAG     "send dhcp-client-identifier"
 
 #define HOSTNAME4_TAG    "send host-name"
 #define HOSTNAME4_FORMAT HOSTNAME4_TAG " \"%s\"; # added by NetworkManager"
@@ -261,6 +263,7 @@ nm_dhcp_dhclient_create_config (const char *interface,
                                 GBytes *client_id,
                                 const char *anycast_addr,
                                 const char *hostname,
+                                guint32 timeout,
                                 gboolean use_fqdn,
                                 const char *orig_path,
                                 const char *orig_contents,
@@ -309,6 +312,17 @@ nm_dhcp_dhclient_create_config (const char *interface,
 			if (intf[0] && !nm_streq (intf, interface))
 				continue;
 
+			/* Some timing parameters in dhclient should not be imported (timeout, retry).
+			 * The retry parameter will be simply not used as we will exit on first failure.
+			 * The timeout one instead may affect NetworkManager behavior: if the timeout
+			 * elapses before dhcp-timeout dhclient will report failure and cause NM to
+			 * fail the dhcp process before dhcp-timeout. So, always skip importing timeout
+			 * as we will need to add one greater than dhcp-timeout.
+			 */
+			if (   !strncmp (p, TIMEOUT_TAG, strlen (TIMEOUT_TAG))
+			    || !strncmp (p, RETRY_TAG, strlen (RETRY_TAG)))
+				continue;
+
 			if (!strncmp (p, CLIENTID_TAG, strlen (CLIENTID_TAG))) {
 				/* Override config file "dhcp-client-id" and use one from the connection */
 				if (client_id)
@@ -374,6 +388,14 @@ nm_dhcp_dhclient_create_config (const char *interface,
 	} else
 		g_string_append_c (new_contents, '\n');
 
+	/* ensure dhclient timeout is greater than dhcp-timeout: as dhclient timeout default value is
+	 * 60 seconds, we need this only if dhcp-timeout is greater than 60.
+	 */
+	if (timeout >= 60) {
+		timeout = timeout < G_MAXINT32 ? timeout + 1 : G_MAXINT32;
+		g_string_append_printf (new_contents, "timeout %u;\n", timeout);
+	}
+
 	if (addr_family == AF_INET) {
 		add_ip4_config (new_contents, client_id, hostname, use_fqdn);
 		add_request (reqs, "rfc3442-classless-static-routes");
diff --git a/src/dhcp/nm-dhcp-dhclient-utils.h b/src/dhcp/nm-dhcp-dhclient-utils.h
index de69fb297c..a5dc9c2b62 100644
--- a/src/dhcp/nm-dhcp-dhclient-utils.h
+++ b/src/dhcp/nm-dhcp-dhclient-utils.h
@@ -27,6 +27,7 @@ char *nm_dhcp_dhclient_create_config (const char *interface,
                                       GBytes *client_id,
                                       const char *anycast_addr,
                                       const char *hostname,
+                                      guint32 timeout,
                                       gboolean use_fqdn,
                                       const char *orig_path,
                                       const char *orig_contents,
diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c
index 39aaaabfc9..f04c1862b7 100644
--- a/src/dhcp/nm-dhcp-dhclient.c
+++ b/src/dhcp/nm-dhcp-dhclient.c
@@ -194,6 +194,7 @@ merge_dhclient_config (NMDhcpDhclient *self,
                        GBytes *client_id,
                        const char *anycast_addr,
                        const char *hostname,
+                       guint32 timeout,
                        gboolean use_fqdn,
                        const char *orig_path,
                        GBytes **out_new_client_id,
@@ -215,7 +216,8 @@ merge_dhclient_config (NMDhcpDhclient *self,
 		}
 	}
 
-	new = nm_dhcp_dhclient_create_config (iface, addr_family, client_id, anycast_addr, hostname, use_fqdn, orig_path, orig, out_new_client_id);
+	new = nm_dhcp_dhclient_create_config (iface, addr_family, client_id, anycast_addr, hostname, timeout,
+	                                      use_fqdn, orig_path, orig, out_new_client_id);
 	g_assert (new);
 	success = g_file_set_contents (conf_file, new, -1, error);
 	g_free (new);
@@ -303,6 +305,7 @@ create_dhclient_config (NMDhcpDhclient *self,
                         GBytes *client_id,
                         const char *dhcp_anycast_addr,
                         const char *hostname,
+                        guint32 timeout,
                         gboolean use_fqdn,
                         GBytes **out_new_client_id)
 {
@@ -323,7 +326,7 @@ create_dhclient_config (NMDhcpDhclient *self,
 
 	error = NULL;
 	success = merge_dhclient_config (self, addr_family, iface, new, client_id, dhcp_anycast_addr,
-	                                 hostname, use_fqdn, orig, out_new_client_id, &error);
+	                                 hostname, timeout, use_fqdn, orig, out_new_client_id, &error);
 	if (!success) {
 		_LOGW ("error creating dhclient configuration: %s", error->message);
 		g_error_free (error);
@@ -352,8 +355,6 @@ dhclient_start (NMDhcpClient *client,
 	int addr_family;
 	gboolean success;
 	char *escaped, *preferred_leasefile_path = NULL;
-	guint32 timeout;
-	char timeout_str[64];
 
 	g_return_val_if_fail (priv->pid_file == NULL, FALSE);
 
@@ -456,17 +457,6 @@ dhclient_start (NMDhcpClient *client,
 		g_ptr_array_add (argv, (gpointer) priv->conf_file);
 	}
 
-	/* Specify a timeout longer than configuration's one,
-	 * so that dhclient doesn't send back a FAIL event before
-	 * that time.
-	 */
-	timeout = nm_dhcp_client_get_timeout (client);
-	if (timeout >= 60) {
-		timeout = timeout < G_MAXINT32 ? timeout + 1 : G_MAXINT32;
-		g_ptr_array_add (argv, (gpointer) "--timeout");
-		g_ptr_array_add (argv, (gpointer) nm_sprintf_buf (timeout_str, "%u", (unsigned) timeout));
-	}
-
 	/* Usually the system bus address is well-known; but if it's supposed
 	 * to be something else, we need to push it to dhclient, since dhclient
 	 * sanitizes the environment it gives the action scripts.
@@ -516,6 +506,7 @@ ip4_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const char *last
 	GBytes *client_id;
 	gs_unref_bytes GBytes *new_client_id = NULL;
 	const char *iface, *uuid, *hostname;
+	guint32 timeout;
 	gboolean success = FALSE;
 	gboolean use_fqdn;
 
@@ -523,10 +514,11 @@ ip4_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const char *last
 	uuid = nm_dhcp_client_get_uuid (client);
 	client_id = nm_dhcp_client_get_client_id (client);
 	hostname = nm_dhcp_client_get_hostname (client);
+	timeout = nm_dhcp_client_get_timeout (client);
 	use_fqdn = nm_dhcp_client_get_use_fqdn (client);
 
 	priv->conf_file = create_dhclient_config (self, AF_INET, iface, uuid, client_id, dhcp_anycast_addr,
-	                                          hostname, use_fqdn, &new_client_id);
+	                                          hostname, timeout, use_fqdn, &new_client_id);
 	if (priv->conf_file) {
 		if (new_client_id)
 			nm_dhcp_client_set_client_id (client, new_client_id);
@@ -549,12 +541,15 @@ ip6_start (NMDhcpClient *client,
 	NMDhcpDhclient *self = NM_DHCP_DHCLIENT (client);
 	NMDhcpDhclientPrivate *priv = NM_DHCP_DHCLIENT_GET_PRIVATE (self);
 	const char *iface, *uuid, *hostname;
+	guint32 timeout;
 
 	iface = nm_dhcp_client_get_iface (client);
 	uuid = nm_dhcp_client_get_uuid (client);
 	hostname = nm_dhcp_client_get_hostname (client);
+	timeout = nm_dhcp_client_get_timeout (client);
 
-	priv->conf_file = create_dhclient_config (self, AF_INET6, iface, uuid, NULL, dhcp_anycast_addr, hostname, TRUE, NULL);
+	priv->conf_file = create_dhclient_config (self, AF_INET6, iface, uuid, NULL, dhcp_anycast_addr,
+	                                          hostname, timeout, TRUE, NULL);
 	if (!priv->conf_file) {
 		_LOGW ("error creating dhclient configuration file");
 		return FALSE;
diff --git a/src/dhcp/tests/test-dhcp-dhclient.c b/src/dhcp/tests/test-dhcp-dhclient.c
index 879aa317e7..a338c13394 100644
--- a/src/dhcp/tests/test-dhcp-dhclient.c
+++ b/src/dhcp/tests/test-dhcp-dhclient.c
@@ -44,6 +44,7 @@ test_config (const char *orig,
              const char *expected,
              int addr_family,
              const char *hostname,
+             guint32 timeout,
              gboolean use_fqdn,
              const char *dhcp_client_id,
              GBytes *expected_new_client_id,
@@ -64,6 +65,7 @@ test_config (const char *orig,
 	                                      client_id,
 	                                      anycast_addr,
 	                                      hostname,
+	                                      timeout,
 	                                      use_fqdn,
 	                                      "/path/to/dhclient.conf",
 	                                      orig,
@@ -108,7 +110,7 @@ static const char *orig_missing_expected = \
 static void
 test_orig_missing (void)
 {
-	test_config (NULL, orig_missing_expected, AF_INET, NULL, FALSE, NULL, NULL, "eth0", NULL);
+	test_config (NULL, orig_missing_expected, AF_INET, NULL, 0, FALSE, NULL, NULL, "eth0", NULL);
 }
 
 /*****************************************************************************/
@@ -137,7 +139,7 @@ static void
 test_override_client_id (void)
 {
 	test_config (override_client_id_orig, override_client_id_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             "11:22:33:44:55:66",
 	             NULL,
 	             "eth0",
@@ -166,7 +168,7 @@ static void
 test_quote_client_id (void)
 {
 	test_config (NULL, quote_client_id_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             "1234",
 	             NULL,
 	             "eth0",
@@ -195,7 +197,7 @@ static void
 test_ascii_client_id (void)
 {
 	test_config (NULL, ascii_client_id_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             "qb:cd:ef:12:34:56",
 	             NULL,
 	             "eth0",
@@ -224,7 +226,7 @@ static void
 test_hex_single_client_id (void)
 {
 	test_config (NULL, hex_single_client_id_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             "ab:cd:e:12:34:56",
 	             NULL,
 	             "eth0",
@@ -261,7 +263,7 @@ test_existing_hex_client_id (void)
 
 	new_client_id = g_bytes_new (bytes, sizeof (bytes));
 	test_config (existing_hex_client_id_orig, existing_hex_client_id_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             new_client_id,
 	             "eth0",
@@ -301,7 +303,7 @@ test_existing_ascii_client_id (void)
 	memcpy (buf + 1, EACID, NM_STRLEN (EACID));
 	new_client_id = g_bytes_new (buf, sizeof (buf));
 	test_config (existing_ascii_client_id_orig, existing_ascii_client_id_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             new_client_id,
 	             "eth0",
@@ -330,7 +332,7 @@ static void
 test_fqdn (void)
 {
 	test_config (NULL, fqdn_expected,
-	             AF_INET, "foo.bar.com",
+	             AF_INET, "foo.bar.com", 0,
 	             TRUE, NULL,
 	             NULL,
 	             "eth0",
@@ -370,7 +372,7 @@ test_fqdn_options_override (void)
 {
 	test_config (fqdn_options_override_orig,
 	             fqdn_options_override_expected,
-	             AF_INET, "example2.com",
+	             AF_INET, "example2.com", 0,
 	             TRUE, NULL,
 	             NULL,
 	             "eth0",
@@ -403,7 +405,7 @@ static void
 test_override_hostname (void)
 {
 	test_config (override_hostname_orig, override_hostname_expected,
-	             AF_INET, "blahblah", FALSE,
+	             AF_INET, "blahblah", 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -431,7 +433,7 @@ static void
 test_override_hostname6 (void)
 {
 	test_config (override_hostname6_orig, override_hostname6_expected,
-	             AF_INET6, "blahblah.local", TRUE,
+	             AF_INET6, "blahblah.local", 0, TRUE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -456,8 +458,8 @@ test_nonfqdn_hostname6 (void)
 {
 	/* Non-FQDN hostname can now be used with dhclient */
 	test_config (NULL, nonfqdn_hostname6_expected,
-	             AF_INET6, "blahblah",
-	             TRUE, NULL,
+	             AF_INET6, "blahblah", 0, TRUE,
+	             NULL,
 	             NULL,
 	             "eth0",
 	             NULL);
@@ -491,8 +493,7 @@ static void
 test_existing_alsoreq (void)
 {
 	test_config (existing_alsoreq_orig, existing_alsoreq_expected,
-	             AF_INET, NULL,
-	             FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -530,8 +531,7 @@ static void
 test_existing_req (void)
 {
 	test_config (existing_req_orig, existing_req_expected,
-	             AF_INET, NULL,
-	             FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -570,7 +570,7 @@ static void
 test_existing_multiline_alsoreq (void)
 {
 	test_config (existing_multiline_alsoreq_orig, existing_multiline_alsoreq_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -784,7 +784,7 @@ static void
 test_interface1 (void)
 {
 	test_config (interface1_orig, interface1_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -829,7 +829,7 @@ static void
 test_interface2 (void)
 {
 	test_config (interface2_orig, interface2_expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth1",
@@ -883,7 +883,7 @@ test_config_req_intf (void)
 		"\n";
 
 	test_config (orig, expected,
-	             AF_INET, NULL, FALSE,
+	             AF_INET, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",