Commit 1cb4832f authored by Francesco Giudici's avatar Francesco Giudici
Browse files

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: fa467360

https://bugzilla.redhat.com/show_bug.cgi?id=1491243
parent 8198a3ab
......@@ -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");
......
......@@ -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,
......
......@@ -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;
......
......@@ -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",
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment