From 9ac4bdb5015fb6c43071f800132b11ca6dc0641f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 08:43:45 +0200 Subject: [PATCH] device: add "dhcp-plugin" match spec for device The need for this is the following: "ipv4.dhcp-client-id" can be specified via global connection defaults. In absence of any configuration in NetworkManager, the default depends on the DHCP client plugin. In case of "dhclient", the default further depends on /etc/dhcp. For "internal" plugin, we may very well want to change the default client-id to "mac" by universally installing a configuration snippet [connection-use-mac-client-id] ipv4.dhcp-client-id=mac However, if we the user happens to enable "dhclient" plugin, this also forces the client-id and overrules configuration from /etc/dhcp. The real problem is, that dhclient can be configured via means outside of NetworkManager, so our defaults shall not overwrite defaults from /etc/dhcp. With the new device spec, we can avoid this issue: [connection-dhcp-client-id] match-device=except:dhcp-plugin:dhclient ipv4.dhcp-client-id=mac This will be part of the solution for rh#1640494. Note that merely dropping a configuration snippet is not yet enough. More fixes for DHCP will follow. Also, bug rh#1640494 may have alternative solutions as well. The nice part of this new feature is that it is generally useful for configuring connection defaults and not specifically for the client-id issue. Note that this match spec is per-device, although the plugin is selected globally. That makes some sense, because in the future we may or may not configure the DHCP plugin per-device or per address family. https://bugzilla.redhat.com/show_bug.cgi?id=1640494 (cherry picked from commit b9eb264efe6dec856d5e30f0c48a62017bad1466) --- man/NetworkManager.conf.xml | 5 +++++ src/NetworkManagerUtils.c | 4 +++- src/NetworkManagerUtils.h | 1 + src/devices/nm-device.c | 1 + src/dhcp/nm-dhcp-manager.c | 10 ++++++++++ src/dhcp/nm-dhcp-manager.h | 2 ++ src/nm-config-data.c | 6 +++++- src/nm-config-data.h | 9 +++++++++ src/nm-core-utils.c | 9 ++++++++- src/nm-core-utils.h | 3 ++- src/tests/config/test-config.c | 19 +++++++++++++++++++ src/tests/test-general.c | 6 +++--- 12 files changed, 68 insertions(+), 7 deletions(-) diff --git a/man/NetworkManager.conf.xml b/man/NetworkManager.conf.xml index 87cf001621..25b70bc6e1 100644 --- a/man/NetworkManager.conf.xml +++ b/man/NetworkManager.conf.xml @@ -1343,6 +1343,11 @@ enable=nm-version-min:1.3,nm-version-min:1.2.6,nm-version-min:1.0.16 Optionally, a driver version may be specified separated by '/'. Globbing is supported for the version. + + dhcp-plugin:DHCP + Match the configured DHCP plugin "main.dhcp". + + except:SPEC Negative match of a device. SPEC must be explicitly qualified with diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 1404854d71..e3766809d5 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -881,6 +881,7 @@ nm_utils_match_connection (NMConnection *const*connections, int nm_match_spec_device_by_pllink (const NMPlatformLink *pllink, const char *match_device_type, + const char *match_dhcp_plugin, const GSList *specs, int no_match_value) { @@ -897,7 +898,8 @@ nm_match_spec_device_by_pllink (const NMPlatformLink *pllink, pllink ? pllink->driver : NULL, NULL, NULL, - NULL); + NULL, + match_dhcp_plugin); switch (m) { case NM_MATCH_SPEC_MATCH: diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index b26d08bdce..efbd9037c5 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -50,6 +50,7 @@ NMConnection *nm_utils_match_connection (NMConnection *const*connections, int nm_match_spec_device_by_pllink (const NMPlatformLink *pllink, const char *match_device_type, + const char *match_dhcp_plugin, const GSList *specs, int no_match_value); diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index f2dbe6594d..a6074a8a7e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -15728,6 +15728,7 @@ nm_device_spec_match_list_full (NMDevice *self, const GSList *specs, int no_matc nm_device_get_driver (self), nm_device_get_driver_version (self), nm_device_get_permanent_hw_address (self), + nm_dhcp_manager_get_config (nm_dhcp_manager_get ()), klass->get_s390_subchannels ? klass->get_s390_subchannels (self) : NULL); switch (m) { diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 5ae16d721a..372f241d47 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -384,6 +384,12 @@ nm_dhcp_manager_get_config (NMDhcpManager *self) NM_DEFINE_SINGLETON_GETTER (NMDhcpManager, nm_dhcp_manager_get, NM_TYPE_DHCP_MANAGER); +void +nmtst_dhcp_manager_unget (gpointer self) +{ + _nmtst_nm_dhcp_manager_get_reset (self); +} + static void nm_dhcp_manager_init (NMDhcpManager *self) { @@ -446,6 +452,10 @@ nm_dhcp_manager_init (NMDhcpManager *self) nm_log_info (LOGD_DHCP, "dhcp-init: Using DHCP client '%s'", client_factory->name); + /* NOTE: currently the DHCP plugin is chosen once at start. It's not + * possible to reload that configuration. If that ever becomes possible, + * beware that the "dhcp-plugin" device spec made decisions based on + * the previous plugin and may need reevaluation. */ priv->client_factory = client_factory; } diff --git a/src/dhcp/nm-dhcp-manager.h b/src/dhcp/nm-dhcp-manager.h index 1d9e5c21dc..f8f39e53e5 100644 --- a/src/dhcp/nm-dhcp-manager.h +++ b/src/dhcp/nm-dhcp-manager.h @@ -87,4 +87,6 @@ extern const char* nm_dhcp_helper_path; extern const NMDhcpClientFactory *const _nm_dhcp_manager_factories[4]; +void nmtst_dhcp_manager_unget (gpointer singleton_instance); + #endif /* __NETWORKMANAGER_DHCP_MANAGER_H__ */ diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 8d84e74abc..0259f00179 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -1241,9 +1241,13 @@ _match_section_infos_lookup (const MatchSectionInfo *match_section_infos, const char *match_device_type, char **out_value) { + const char *match_dhcp_plugin; + if (!match_section_infos) return NULL; + match_dhcp_plugin = nm_dhcp_manager_get_config (nm_dhcp_manager_get ()); + for (; match_section_infos->group_name; match_section_infos++) { char *value = NULL; gboolean match; @@ -1263,7 +1267,7 @@ _match_section_infos_lookup (const MatchSectionInfo *match_section_infos, if (device) match = nm_device_spec_match_list (device, match_section_infos->match_device.spec); else if (pllink) - match = nm_match_spec_device_by_pllink (pllink, match_device_type, match_section_infos->match_device.spec, FALSE); + match = nm_match_spec_device_by_pllink (pllink, match_device_type, match_dhcp_plugin, match_section_infos->match_device.spec, FALSE); else match = FALSE; } else diff --git a/src/nm-config-data.h b/src/nm-config-data.h index b52ddcc4f8..545d9a8758 100644 --- a/src/nm-config-data.h +++ b/src/nm-config-data.h @@ -232,5 +232,14 @@ GKeyFile *_nm_config_data_get_keyfile (const NMConfigData *self); GKeyFile *_nm_config_data_get_keyfile_user (const NMConfigData *self); GKeyFile *_nm_config_data_get_keyfile_intern (const NMConfigData *self); +/*****************************************************************************/ + +/* nm-config-data.c requires getting the DHCP manager's configuration. That is a bit + * ugly, and optimally, NMConfig* is independent of NMDhcpManager. Instead of + * including the header, forward declare the two functions that we need. */ +struct _NMDhcpManager; +struct _NMDhcpManager *nm_dhcp_manager_get (void); +const char *nm_dhcp_manager_get_config (struct _NMDhcpManager *self); + #endif /* NM_CONFIG_DATA_H */ diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index ca8c952671..2e25340bf9 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -1133,6 +1133,7 @@ nm_utils_read_link_absolute (const char *link_file, GError **error) #define DEVICE_TYPE_TAG "type:" #define DRIVER_TAG "driver:" #define SUBCHAN_TAG "s390-subchannels:" +#define DHCP_PLUGIN_TAG "dhcp-plugin:" #define EXCEPT_TAG "except:" #define MATCH_TAG_CONFIG_NM_VERSION "nm-version:" #define MATCH_TAG_CONFIG_NM_VERSION_MIN "nm-version-min:" @@ -1144,6 +1145,7 @@ typedef struct { const char *device_type; const char *driver; const char *driver_version; + const char *dhcp_plugin; struct { const char *value; gboolean is_parsed; @@ -1361,6 +1363,9 @@ match_device_eval (const char *spec_str, if (_MATCH_CHECK (spec_str, SUBCHAN_TAG)) return match_data_s390_subchannels_eval (spec_str, match_data); + if (_MATCH_CHECK (spec_str, DHCP_PLUGIN_TAG)) + return nm_streq0 (spec_str, match_data->dhcp_plugin); + if (allow_fuzzy) { if (match_device_hwaddr_eval (spec_str, match_data)) return TRUE; @@ -1379,7 +1384,8 @@ nm_match_spec_device (const GSList *specs, const char *driver, const char *driver_version, const char *hwaddr, - const char *s390_subchannels) + const char *s390_subchannels, + const char *dhcp_plugin) { const GSList *iter; NMMatchSpecMatchType match; @@ -1390,6 +1396,7 @@ nm_match_spec_device (const GSList *specs, .device_type = nm_str_not_empty (device_type), .driver = nm_str_not_empty (driver), .driver_version = nm_str_not_empty (driver_version), + .dhcp_plugin = nm_str_not_empty (dhcp_plugin), .hwaddr = { .value = hwaddr, }, diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 8b8043f194..42f59cfcb5 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -223,7 +223,8 @@ NMMatchSpecMatchType nm_match_spec_device (const GSList *specs, const char *driver, const char *driver_version, const char *hwaddr, - const char *s390_subchannels); + const char *s390_subchannels, + const char *dhcp_plugin); NMMatchSpecMatchType nm_match_spec_config (const GSList *specs, guint nm_version, const char *env); diff --git a/src/tests/config/test-config.c b/src/tests/config/test-config.c index 75fef4fad1..20a05df137 100644 --- a/src/tests/config/test-config.c +++ b/src/tests/config/test-config.c @@ -25,6 +25,7 @@ #include "nm-config.h" #include "nm-test-device.h" #include "platform/nm-fake-platform.h" +#include "dhcp/nm-dhcp-manager.h" #include "nm-dbus-manager.h" #include "nm-connectivity.h" @@ -123,6 +124,24 @@ setup_config (GError **error, const char *config_file, const char *intern_config g_assert_no_error (local_error); } nm_config_cmd_line_options_free (cli); + + if (config) { + NMDhcpManager *dhcp_manager; + gpointer logging_old_state; + + logging_old_state = nmtst_logging_disable (FALSE); + + dhcp_manager = nm_dhcp_manager_get (); + g_test_assert_expected_messages (); + + nmtst_logging_reenable (logging_old_state); + + g_object_set_data_full (G_OBJECT (config), + "nmtst-config-keep-dhcp-manager-alive", + dhcp_manager, + nmtst_dhcp_manager_unget); + } + return config; } diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 01d964322c..91cbe09d84 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -1095,7 +1095,7 @@ static NMMatchSpecMatchType _test_match_spec_device (const GSList *specs, const char *match_str) { if (match_str && g_str_has_prefix (match_str, MATCH_S390)) - return nm_match_spec_device (specs, NULL, NULL, NULL, NULL, NULL, &match_str[NM_STRLEN (MATCH_S390)]); + return nm_match_spec_device (specs, NULL, NULL, NULL, NULL, NULL, &match_str[NM_STRLEN (MATCH_S390)], NULL); if (match_str && g_str_has_prefix (match_str, MATCH_DRIVER)) { gs_free char *s = g_strdup (&match_str[NM_STRLEN (MATCH_DRIVER)]); char *t; @@ -1105,9 +1105,9 @@ _test_match_spec_device (const GSList *specs, const char *match_str) t[0] = '\0'; t++; } - return nm_match_spec_device (specs, NULL, NULL, s, t, NULL, NULL); + return nm_match_spec_device (specs, NULL, NULL, s, t, NULL, NULL, NULL); } - return nm_match_spec_device (specs, match_str, NULL, NULL, NULL, NULL, NULL); + return nm_match_spec_device (specs, match_str, NULL, NULL, NULL, NULL, NULL, NULL); } static void