From 61a84e2b0009ca907984b1e33a806c7ed21247ec Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 24 Oct 2018 10:52:42 +0200
Subject: [PATCH 1/3] core/tests: allow to reset singleton instantiations for
testing
Most singletons can only be instantiated once (unless NM_DEFINE_SINGLETON_ALLOW_MULTIPLE
is defined). Otherwise, an assertion will be triggered if the singleton is destroyed
and another instance is requested.
For testing, we want to create multiple singleton instances and being able to reset
the singleton getter. Add a function for that.
(cherry picked from commit 5f4d8ffa79d574c86b108a83735e58e6f1bb6ab5)
(cherry picked from commit 9672ea128e3f9bbe7345dd12c111fdcdd211cd1f)
---
src/nm-core-utils.h | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h
index 30d1360a1..8b8043f19 100644
--- a/src/nm-core-utils.h
+++ b/src/nm-core-utils.h
@@ -61,20 +61,35 @@ void _nm_singleton_instance_register_destruction (GObject *instance);
#define NM_DEFINE_SINGLETON_GETTER(TYPE, GETTER, GTYPE, ...) \
NM_DEFINE_SINGLETON_INSTANCE (TYPE); \
NM_DEFINE_SINGLETON_REGISTER (TYPE); \
+static char _already_created_##GETTER = FALSE; \
TYPE * \
GETTER (void) \
{ \
if (G_UNLIKELY (!singleton_instance)) { \
- static char _already_created = FALSE; \
-\
- g_assert (!_already_created || (NM_DEFINE_SINGLETON_ALLOW_MULTIPLE)); \
- _already_created = TRUE;\
+ g_assert (!(_already_created_##GETTER) || (NM_DEFINE_SINGLETON_ALLOW_MULTIPLE)); \
+ (_already_created_##GETTER) = TRUE;\
singleton_instance = (g_object_new (GTYPE, ##__VA_ARGS__, NULL)); \
g_assert (singleton_instance); \
nm_singleton_instance_register (); \
nm_log_dbg (LOGD_CORE, "create %s singleton (%p)", G_STRINGIFY (TYPE), singleton_instance); \
} \
return singleton_instance; \
+} \
+_nm_unused static void \
+_nmtst_##GETTER##_reset (TYPE *instance) \
+{ \
+ /* usually, the singleton can only be created once (and further instantiations
+ * are guarded by an assert). For testing, we need to reset the singleton to
+ * allow multiple instantiations. */ \
+ g_assert (G_IS_OBJECT (instance)); \
+ g_assert (instance == singleton_instance); \
+ g_assert (_already_created_##GETTER); \
+ g_object_unref (instance); \
+ \
+ /* require that the last unref also destroyed the singleton. If this fails,
+ * somebody still keeps a reference. Fix your test! */ \
+ g_assert (!singleton_instance); \
+ _already_created_##GETTER = FALSE; \
}
/* attach @instance to the data or @owner. @owner owns a reference
--
2.19.1
From b8cfcfeffe24ff72478c459433f5cd342fb39957 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 24 Oct 2018 11:51:57 +0200
Subject: [PATCH 2/3] core/tests: allow temporarily suppressing logging during
tests
Often, during tests we want to assert against the logged messages.
In fact, most tests enable assertions for all logging and enforce
them with g_test_assert_expected_messages(). So, this is common.
However, sometimes it can be cumbersome to understand which logging
lines will be produced. For example, the next commits will call
nm_dhcp_manager_get() during the tests, which initializes NMDhcpManager
and logs a message which plugin was selected (or an additional warning,
if the selected plugin was not found). The availability of the DHCP plugin
depends on searching the path for "/usr/bin/dhclient", so from testing code
it's hard to determine what will be logged.
Instead, add a way to temporarily disable logging during testing.
(cherry picked from commit 35cecd32fd9c27f67d3151247692d0d38185f149)
(cherry picked from commit e2d777ff13acbb63ec9d39fec510fa55052dd1fb)
---
shared/nm-utils/nm-test-utils.h | 36 +++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h
index b575382e4..e09525e6f 100644
--- a/shared/nm-utils/nm-test-utils.h
+++ b/shared/nm-utils/nm-test-utils.h
@@ -1337,6 +1337,42 @@ _nmtst_assert_resolve_relative_path_equals (const char *f1, const char *f2, cons
/*****************************************************************************/
+#ifdef __NETWORKMANAGER_LOGGING_H__
+static inline gpointer
+nmtst_logging_disable (gboolean always)
+{
+ gpointer p;
+
+ g_assert (nmtst_initialized ());
+ if (!always && __nmtst_internal.no_expect_message) {
+ /* The caller does not want to @always suppress logging. Instead,
+ * the caller wants to suppress unexpected log messages that would
+ * fail assertions (since we possibly assert against all unexpected
+ * log messages).
+ *
+ * If the test is run with no-expect-message, then don't suppress
+ * the loggings, because they also wouldn't fail assertions. */
+ return NULL;
+ }
+
+ p = g_memdup (_nm_logging_enabled_state, sizeof (_nm_logging_enabled_state));
+ memset (_nm_logging_enabled_state, 0, sizeof (_nm_logging_enabled_state));
+ return p;
+}
+
+static inline void
+nmtst_logging_reenable (gpointer old_state)
+{
+ g_assert (nmtst_initialized ());
+ if (old_state) {
+ memcpy (_nm_logging_enabled_state, old_state, sizeof (_nm_logging_enabled_state));
+ g_free (old_state);
+ }
+}
+#endif
+
+/*****************************************************************************/
+
#ifdef NM_SETTING_IP_CONFIG_H
static inline void
nmtst_setting_ip_config_add_address (NMSettingIPConfig *s_ip,
--
2.19.1
From 472782c69be453c2f2d9a1db77b4d219c045341b Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 24 Oct 2018 08:43:45 +0200
Subject: [PATCH 3/3] 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)
(cherry picked from commit 9ac4bdb5015fb6c43071f800132b11ca6dc0641f)
---
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 87cf00162..25b70bc6e 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.
</para></listitem>
</varlistentry>
+ <varlistentry>
+ <term>dhcp-plugin:DHCP</term>
+ <listitem><para>Match the configured DHCP plugin "<literal>main.dhcp</literal>".
+ </para></listitem>
+ </varlistentry>
<varlistentry>
<term>except:SPEC</term>
<listitem><para>Negative match of a device. <literal>SPEC</literal> must be explicitly qualified with
diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c
index 1404854d7..e3766809d 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 b26d08bdc..efbd9037c 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 f8c99d488..51751ecf3 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -15693,6 +15693,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 6c71af9d0..10b6133c8 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 1d9e5c21d..f8f39e53e 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 8d84e74ab..0259f0017 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 b52ddcc4f..545d9a875 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 ca8c95267..2e25340bf 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 8b8043f19..42f59cfcb 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 75fef4fad..20a05df13 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 1e7329d13..7cc4b982c 100644
--- a/src/tests/test-general.c
+++ b/src/tests/test-general.c
@@ -1093,7 +1093,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;
@@ -1103,9 +1103,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
--
2.19.1