dhcp: dhclient: remove the --timeout argument from the command line

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
(cherry picked from commit 1cb4832f0933dda36556c36b2b7b2306cf76139a)

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 28c290cf94..6a1b686523 100644
--- a/src/dhcp/nm-dhcp-dhclient-utils.c
+++ b/src/dhcp/nm-dhcp-dhclient-utils.c
@@ -31,7 +31,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"
@@ -263,6 +265,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,
@@ -310,6 +313,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)
@@ -375,6 +389,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 (is_ip6) {
 		add_hostname6 (new_contents, hostname);
 		add_request (reqs, "dhcp6.name-servers");
diff --git a/src/dhcp/nm-dhcp-dhclient-utils.h b/src/dhcp/nm-dhcp-dhclient-utils.h
index 994b1b9f02..2268890bb5 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 5a07d34766..f20158c64d 100644
--- a/src/dhcp/nm-dhcp-dhclient.c
+++ b/src/dhcp/nm-dhcp-dhclient.c
@@ -182,6 +182,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,
@@ -206,7 +207,8 @@ merge_dhclient_config (NMDhcpDhclient *self,
 	if (is_ip6 && hostname && !strchr (hostname, '.'))
 		_LOGW ("hostname is not a FQDN, it will be ignored");
 
-	new = nm_dhcp_dhclient_create_config (iface, is_ip6, client_id, anycast_addr, hostname, use_fqdn, orig_path, orig, out_new_client_id);
+	new = nm_dhcp_dhclient_create_config (iface, is_ip6, 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);
@@ -294,6 +296,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)
 {
@@ -314,7 +317,7 @@ create_dhclient_config (NMDhcpDhclient *self,
 
 	error = NULL;
 	success = merge_dhclient_config (self, iface, new, is_ip6, 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);
@@ -342,8 +345,6 @@ dhclient_start (NMDhcpClient *client,
 	char *binary_name, *cmd_str, *pid_file = NULL, *system_bus_address_env = NULL;
 	gboolean ipv6, success;
 	char *escaped, *preferred_leasefile_path = NULL;
-	guint32 timeout;
-	char timeout_str[64];
 
 	g_return_val_if_fail (priv->pid_file == NULL, FALSE);
 
@@ -446,17 +447,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.
@@ -506,6 +496,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;
 
@@ -513,10 +504,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, iface, FALSE, 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);
@@ -539,12 +531,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, iface, TRUE, uuid, NULL, dhcp_anycast_addr, hostname, TRUE, NULL);
+	priv->conf_file = create_dhclient_config (self, iface, TRUE, 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 3c9760ad67..5816932b3a 100644
--- a/src/dhcp/tests/test-dhcp-dhclient.c
+++ b/src/dhcp/tests/test-dhcp-dhclient.c
@@ -40,6 +40,7 @@ test_config (const char *orig,
              const char *expected,
              gboolean ipv6,
              const char *hostname,
+             guint32 timeout,
              gboolean use_fqdn,
              const char *dhcp_client_id,
              GBytes *expected_new_client_id,
@@ -60,6 +61,7 @@ test_config (const char *orig,
 	                                      client_id,
 	                                      anycast_addr,
 	                                      hostname,
+	                                      timeout,
 	                                      use_fqdn,
 	                                      "/path/to/dhclient.conf",
 	                                      orig,
@@ -104,7 +106,7 @@ static const char *orig_missing_expected = \
 static void
 test_orig_missing (void)
 {
-	test_config (NULL, orig_missing_expected, FALSE, NULL, FALSE, NULL, NULL, "eth0", NULL);
+	test_config (NULL, orig_missing_expected, FALSE, NULL, 0, FALSE, NULL, NULL, "eth0", NULL);
 }
 
 /*****************************************************************************/
@@ -133,7 +135,7 @@ static void
 test_override_client_id (void)
 {
 	test_config (override_client_id_orig, override_client_id_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             "11:22:33:44:55:66",
 	             NULL,
 	             "eth0",
@@ -162,7 +164,7 @@ static void
 test_quote_client_id (void)
 {
 	test_config (NULL, quote_client_id_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             "1234",
 	             NULL,
 	             "eth0",
@@ -191,7 +193,7 @@ static void
 test_ascii_client_id (void)
 {
 	test_config (NULL, ascii_client_id_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             "qb:cd:ef:12:34:56",
 	             NULL,
 	             "eth0",
@@ -220,7 +222,7 @@ static void
 test_hex_single_client_id (void)
 {
 	test_config (NULL, hex_single_client_id_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             "ab:cd:e:12:34:56",
 	             NULL,
 	             "eth0",
@@ -257,7 +259,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,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             new_client_id,
 	             "eth0",
@@ -297,7 +299,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,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             new_client_id,
 	             "eth0",
@@ -326,7 +328,7 @@ static void
 test_fqdn (void)
 {
 	test_config (NULL, fqdn_expected,
-	             FALSE, "foo.bar.com",
+	             FALSE, "foo.bar.com", 0,
 	             TRUE, NULL,
 	             NULL,
 	             "eth0",
@@ -366,7 +368,7 @@ test_fqdn_options_override (void)
 {
 	test_config (fqdn_options_override_orig,
 	             fqdn_options_override_expected,
-	             FALSE, "example2.com",
+	             FALSE, "example2.com", 0,
 	             TRUE, NULL,
 	             NULL,
 	             "eth0",
@@ -399,7 +401,7 @@ static void
 test_override_hostname (void)
 {
 	test_config (override_hostname_orig, override_hostname_expected,
-	             FALSE, "blahblah", FALSE,
+	             FALSE, "blahblah", 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -428,7 +430,7 @@ static void
 test_override_hostname6 (void)
 {
 	test_config (override_hostname6_orig, override_hostname6_expected,
-	             TRUE, "blahblah.local", TRUE,
+	             TRUE, "blahblah.local", 0, TRUE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -450,8 +452,8 @@ test_nonfqdn_hostname6 (void)
 {
 	/* Non-FQDN hostname can't be used with dhclient */
 	test_config (NULL, nonfqdn_hostname6_expected,
-	             TRUE, "blahblah",
-	             TRUE, NULL,
+	             TRUE, "blahblah", 0, TRUE,
+	             NULL,
 	             NULL,
 	             "eth0",
 	             NULL);
@@ -485,8 +487,7 @@ static void
 test_existing_alsoreq (void)
 {
 	test_config (existing_alsoreq_orig, existing_alsoreq_expected,
-	             FALSE, NULL,
-	             FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -524,8 +525,7 @@ static void
 test_existing_req (void)
 {
 	test_config (existing_req_orig, existing_req_expected,
-	             FALSE, NULL,
-	             FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -564,7 +564,7 @@ static void
 test_existing_multiline_alsoreq (void)
 {
 	test_config (existing_multiline_alsoreq_orig, existing_multiline_alsoreq_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -778,7 +778,7 @@ static void
 test_interface1 (void)
 {
 	test_config (interface1_orig, interface1_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",
@@ -823,7 +823,7 @@ static void
 test_interface2 (void)
 {
 	test_config (interface2_orig, interface2_expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth1",
@@ -877,7 +877,7 @@ test_config_req_intf (void)
 		"\n";
 
 	test_config (orig, expected,
-	             FALSE, NULL, FALSE,
+	             FALSE, NULL, 0, FALSE,
 	             NULL,
 	             NULL,
 	             "eth0",