From b32cf7181463d10c646405596ceec4f83333edf3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 Jul 2019 15:46:03 +0200 Subject: [PATCH 1/4] dhcp: cleanup selecting GType from DHCP client factory Instead of returning a client-factory, return the GType right away. --- src/dhcp/nm-dhcp-manager.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 304a7b9961..06bfd8ec0c 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -89,10 +89,11 @@ _client_factory_available (const NMDhcpClientFactory *client_factory) return NULL; } -static const NMDhcpClientFactory * -_client_factory_get_effective (const NMDhcpClientFactory *client_factory, - int addr_family) +static GType +_client_factory_get_gtype (const NMDhcpClientFactory *client_factory, + int addr_family) { + GType gtype; nm_auto_unref_gtypeclass NMDhcpClientClass *klass = NULL; nm_assert (client_factory); @@ -118,23 +119,25 @@ _client_factory_get_effective (const NMDhcpClientFactory *client_factory, * to those plugins. But we don't intend to do so. The internal plugin is the way forward and * not extending other plugins. */ + gtype = client_factory->get_type (); + if (client_factory == &_nm_dhcp_client_factory_internal) { - /* already using internal plugin. Nothing to do. */ - return client_factory; + /* we are already using the internal plugin. Nothing to do. */ + return gtype; } - klass = g_type_class_ref (client_factory->get_type ()); + klass = g_type_class_ref (gtype); nm_assert (NM_IS_DHCP_CLIENT_CLASS (klass)); if (addr_family == AF_INET6) { return klass->ip6_start - ? client_factory - : &_nm_dhcp_client_factory_internal; + ? gtype + : _nm_dhcp_client_factory_internal.get_type (); } return klass->ip4_start - ? client_factory - : &_nm_dhcp_client_factory_internal; + ? gtype + : _nm_dhcp_client_factory_internal.get_type (); } /*****************************************************************************/ @@ -225,7 +228,6 @@ client_start (NMDhcpManager *self, NMDhcpClient *client; gboolean success = FALSE; gsize hwaddr_len; - const NMDhcpClientFactory *client_factory; g_return_val_if_fail (NM_IS_DHCP_MANAGER (self), NULL); g_return_val_if_fail (iface, NULL); @@ -255,8 +257,6 @@ client_start (NMDhcpManager *self, priv = NM_DHCP_MANAGER_GET_PRIVATE (self); - client_factory = _client_factory_get_effective (priv->client_factory, addr_family); - /* Kill any old client instance */ client = get_client_for_ifindex (self, addr_family, ifindex); if (client) { @@ -271,7 +271,7 @@ client_start (NMDhcpManager *self, g_object_unref (client); } - client = g_object_new (client_factory->get_type (), + client = g_object_new (_client_factory_get_gtype (priv->client_factory, addr_family), NM_DHCP_CLIENT_MULTI_IDX, multi_idx, NM_DHCP_CLIENT_ADDR_FAMILY, addr_family, NM_DHCP_CLIENT_INTERFACE, iface, From 8d8cc0da3d29b3a064a5cbd883365817a56f8e9d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 Jul 2019 16:12:41 +0200 Subject: [PATCH 2/4] dhcp: log effectively used DHCP plugin type --- src/dhcp/nm-dhcp-manager.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 06bfd8ec0c..507fc64359 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -123,7 +123,7 @@ _client_factory_get_gtype (const NMDhcpClientFactory *client_factory, if (client_factory == &_nm_dhcp_client_factory_internal) { /* we are already using the internal plugin. Nothing to do. */ - return gtype; + goto out; } klass = g_type_class_ref (gtype); @@ -131,13 +131,23 @@ _client_factory_get_gtype (const NMDhcpClientFactory *client_factory, nm_assert (NM_IS_DHCP_CLIENT_CLASS (klass)); if (addr_family == AF_INET6) { - return klass->ip6_start - ? gtype - : _nm_dhcp_client_factory_internal.get_type (); + if (!klass->ip6_start) + gtype = _client_factory_get_gtype (&_nm_dhcp_client_factory_internal, addr_family); + } else { + if (!klass->ip4_start) + gtype = _client_factory_get_gtype (&_nm_dhcp_client_factory_internal, addr_family); } - return klass->ip4_start - ? gtype - : _nm_dhcp_client_factory_internal.get_type (); + +out: + nm_assert (g_type_is_a (gtype, NM_TYPE_DHCP_CLIENT)); + nm_assert (({ + nm_auto_unref_gtypeclass NMDhcpClientClass *k = g_type_class_ref (gtype); + + (addr_family == AF_INET6 && k->ip6_start) + || (addr_family == AF_INET && k->ip4_start); + })); + + return gtype; } /*****************************************************************************/ @@ -228,6 +238,7 @@ client_start (NMDhcpManager *self, NMDhcpClient *client; gboolean success = FALSE; gsize hwaddr_len; + GType gtype; g_return_val_if_fail (NM_IS_DHCP_MANAGER (self), NULL); g_return_val_if_fail (iface, NULL); @@ -271,7 +282,14 @@ client_start (NMDhcpManager *self, g_object_unref (client); } - client = g_object_new (_client_factory_get_gtype (priv->client_factory, addr_family), + gtype = _client_factory_get_gtype (priv->client_factory, addr_family); + + nm_log_trace (LOGD_DHCP , "dhcp%c: creating IPv%c DHCP client of type %s", + nm_utils_addr_family_to_char (addr_family), + nm_utils_addr_family_to_char (addr_family), + g_type_name (gtype)); + + client = g_object_new (gtype, NM_DHCP_CLIENT_MULTI_IDX, multi_idx, NM_DHCP_CLIENT_ADDR_FAMILY, addr_family, NM_DHCP_CLIENT_INTERFACE, iface, From b53e261427c925034ada6b90278b7e9077e2ea43 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 Jul 2019 15:41:02 +0200 Subject: [PATCH 3/4] dhcp: make "systemd" DHCP plugin configurable We have the "internal" DHCP plugin. That's our preferred plugin, and eventually we may drop all other plugins. Currently, the "internal" plugin is based on code from systemd-networkd and implemented in "src/dhcp/nm-dhcp-systemd.c". As this code is forked we eventually want to switch to nettools' n-dhcp4 library (for IPv4). For that reason we already have "src/dhcp/nm-dhcp-nettools.c". Note that "nettools" can be configured as a DHCP plugin, but this configuration is only experimental and for testing. There is never supposed to be a "nettools" plugin, but eventually the "internal" plugin will switch implementation. We don't want to replace systemd-based implementation right away. Not until we are sure that nettools works well. For that reason we keep them both in parallel for a while. This commit makes "systemd" DHCP plugin explicitly configurable in NetworkManager.conf. Like "nettools" this is an undocumented option, only for testing. If you choose "internal" (the default), you get one of the implementations (currently the "systemd" one). But by selecting "systemd" or "nettools" explicitly, you can select the exact plugin. --- src/dhcp/nm-dhcp-client.h | 5 ++++- src/dhcp/nm-dhcp-dhclient.c | 2 +- src/dhcp/nm-dhcp-dhcpcanon.c | 2 +- src/dhcp/nm-dhcp-dhcpcd.c | 2 +- src/dhcp/nm-dhcp-listener.c | 3 ++- src/dhcp/nm-dhcp-manager.c | 12 ++++++++---- src/dhcp/nm-dhcp-manager.h | 2 +- src/dhcp/nm-dhcp-nettools.c | 6 +++--- src/dhcp/nm-dhcp-systemd.c | 23 +++++++++++++++++++---- src/nm-iface-helper.c | 5 +++-- 10 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 9eb76f33a8..551e2449a8 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -207,15 +207,18 @@ void nm_dhcp_client_set_client_id_bin (NMDhcpClient *self, *****************************************************************************/ typedef struct { - GType (*get_type)(void); + GType (*get_type) (void); + GType (*get_type_per_addr_family) (int addr_family); const char *name; const char *(*get_path) (void); + bool experimental:1; } NMDhcpClientFactory; extern const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcanon; extern const NMDhcpClientFactory _nm_dhcp_client_factory_dhclient; extern const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcd; extern const NMDhcpClientFactory _nm_dhcp_client_factory_internal; +extern const NMDhcpClientFactory _nm_dhcp_client_factory_systemd; extern const NMDhcpClientFactory _nm_dhcp_client_factory_nettools; #endif /* __NETWORKMANAGER_DHCP_CLIENT_H__ */ diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index 54b50479dd..72ea3d48f8 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -723,7 +723,7 @@ nm_dhcp_dhclient_class_init (NMDhcpDhclientClass *dhclient_class) } const NMDhcpClientFactory _nm_dhcp_client_factory_dhclient = { - .name = "dhclient", + .name = "dhclient", .get_type = nm_dhcp_dhclient_get_type, .get_path = nm_dhcp_dhclient_get_path, }; diff --git a/src/dhcp/nm-dhcp-dhcpcanon.c b/src/dhcp/nm-dhcp-dhcpcanon.c index 2d2113cc76..c193e36780 100644 --- a/src/dhcp/nm-dhcp-dhcpcanon.c +++ b/src/dhcp/nm-dhcp-dhcpcanon.c @@ -248,7 +248,7 @@ nm_dhcp_dhcpcanon_class_init (NMDhcpDhcpcanonClass *dhcpcanon_class) } const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcanon = { - .name = "dhcpcanon", + .name = "dhcpcanon", .get_type = nm_dhcp_dhcpcanon_get_type, .get_path = nm_dhcp_dhcpcanon_get_path, }; diff --git a/src/dhcp/nm-dhcp-dhcpcd.c b/src/dhcp/nm-dhcp-dhcpcd.c index c300bbe2f0..b07d5a1bba 100644 --- a/src/dhcp/nm-dhcp-dhcpcd.c +++ b/src/dhcp/nm-dhcp-dhcpcd.c @@ -242,7 +242,7 @@ nm_dhcp_dhcpcd_class_init (NMDhcpDhcpcdClass *dhcpcd_class) } const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcd = { - .name = "dhcpcd", + .name = "dhcpcd", .get_type = nm_dhcp_dhcpcd_get_type, .get_path = nm_dhcp_dhcpcd_get_path, }; diff --git a/src/dhcp/nm-dhcp-listener.c b/src/dhcp/nm-dhcp-listener.c index 88aafeb0b1..ce86d55622 100644 --- a/src/dhcp/nm-dhcp-listener.c +++ b/src/dhcp/nm-dhcp-listener.c @@ -38,7 +38,7 @@ /*****************************************************************************/ -const NMDhcpClientFactory *const _nm_dhcp_manager_factories[5] = { +const NMDhcpClientFactory *const _nm_dhcp_manager_factories[6] = { /* the order here matters, as we will try the plugins in this order to find * the first available plugin. */ @@ -52,6 +52,7 @@ const NMDhcpClientFactory *const _nm_dhcp_manager_factories[5] = { &_nm_dhcp_client_factory_dhcpcd, #endif &_nm_dhcp_client_factory_internal, + &_nm_dhcp_client_factory_systemd, &_nm_dhcp_client_factory_nettools, }; diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 507fc64359..3c78d7c2dc 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -119,7 +119,10 @@ _client_factory_get_gtype (const NMDhcpClientFactory *client_factory, * to those plugins. But we don't intend to do so. The internal plugin is the way forward and * not extending other plugins. */ - gtype = client_factory->get_type (); + if (client_factory->get_type_per_addr_family) + gtype = client_factory->get_type_per_addr_family (addr_family); + else + gtype = client_factory->get_type (); if (client_factory == &_nm_dhcp_client_factory_internal) { /* we are already using the internal plugin. Nothing to do. */ @@ -547,9 +550,10 @@ nm_dhcp_manager_init (NMDhcpManager *self) if (!f) continue; - nm_log_dbg (LOGD_DHCP, "dhcp-init: enabled DHCP client '%s' (%s)%s", - f->name, g_type_name (f->get_type ()), - _client_factory_available (f) ? "" : " (not available)"); + nm_log_dbg (LOGD_DHCP, "dhcp-init: enabled DHCP client '%s'%s%s", + f->name, + _client_factory_available (f) ? "" : " (not available)", + f->experimental ? " (undocumented internal plugin)" : ""); } /* Client-specific setup */ diff --git a/src/dhcp/nm-dhcp-manager.h b/src/dhcp/nm-dhcp-manager.h index ff0d6f5472..1e0a972b1f 100644 --- a/src/dhcp/nm-dhcp-manager.h +++ b/src/dhcp/nm-dhcp-manager.h @@ -86,7 +86,7 @@ NMDhcpClient * nm_dhcp_manager_start_ip6 (NMDhcpManager *manager, /* For testing only */ extern const char* nm_dhcp_helper_path; -extern const NMDhcpClientFactory *const _nm_dhcp_manager_factories[5]; +extern const NMDhcpClientFactory *const _nm_dhcp_manager_factories[6]; void nmtst_dhcp_manager_unget (gpointer singleton_instance); diff --git a/src/dhcp/nm-dhcp-nettools.c b/src/dhcp/nm-dhcp-nettools.c index 3aa921807b..292cfecc01 100644 --- a/src/dhcp/nm-dhcp-nettools.c +++ b/src/dhcp/nm-dhcp-nettools.c @@ -1272,7 +1272,7 @@ nm_dhcp_nettools_class_init (NMDhcpNettoolsClass *class) } const NMDhcpClientFactory _nm_dhcp_client_factory_nettools = { - .name = "nettools", - .get_type = nm_dhcp_nettools_get_type, - .get_path = NULL, + .name = "nettools", + .get_type = nm_dhcp_nettools_get_type, + .experimental = TRUE, }; diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 073a6da051..0e30ab3883 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -1173,8 +1173,23 @@ nm_dhcp_systemd_class_init (NMDhcpSystemdClass *sdhcp_class) client_class->stop = stop; } -const NMDhcpClientFactory _nm_dhcp_client_factory_internal = { - .name = "internal", - .get_type = nm_dhcp_systemd_get_type, - .get_path = NULL, +const NMDhcpClientFactory _nm_dhcp_client_factory_systemd = { + .name = "systemd", + .get_type = nm_dhcp_systemd_get_type, + .experimental = TRUE, +}; + +/*****************************************************************************/ + +static GType +_get_type_per_addr_family (int addr_family) +{ + nm_assert_addr_family (addr_family); + + return nm_dhcp_systemd_get_type (); +} + +const NMDhcpClientFactory _nm_dhcp_client_factory_internal = { + .name = "internal", + .get_type_per_addr_family = _get_type_per_addr_family, }; diff --git a/src/nm-iface-helper.c b/src/nm-iface-helper.c index dd5bb327a0..239b46a140 100644 --- a/src/nm-iface-helper.c +++ b/src/nm-iface-helper.c @@ -596,9 +596,10 @@ main (int argc, char *argv[]) /*****************************************************************************/ -const NMDhcpClientFactory *const _nm_dhcp_manager_factories[5] = { +const NMDhcpClientFactory *const _nm_dhcp_manager_factories[6] = { + /* For nm-iface-helper there is no option to choose a DHCP plugin. + * It just uses the "internal" one. */ &_nm_dhcp_client_factory_internal, - &_nm_dhcp_client_factory_nettools, }; /*****************************************************************************/ From 75503c85545110b8941562f3cd47ac602c5db454 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Aug 2019 15:13:27 +0200 Subject: [PATCH 4/4] dhcp: minor refactoring to switch default IPv4 DHCP plugin to "nettools" with one-line change Minor refactoring so that there is only a one-line change necessary to flip the implementation of the "internal" DHCP plugin for IPv4 from "systemd" to "nettools". We don't do that yet, because there are still some issues (e.g. the lease is not persisted for nettools plugin). Eventually we want to switch, so prepare the code to be almost there. --- src/dhcp/nm-dhcp-client.h | 2 ++ src/dhcp/nm-dhcp-nettools.c | 2 -- src/dhcp/nm-dhcp-systemd.c | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 551e2449a8..29f83259f8 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -214,6 +214,8 @@ typedef struct { bool experimental:1; } NMDhcpClientFactory; +GType nm_dhcp_nettools_get_type (void); + extern const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcanon; extern const NMDhcpClientFactory _nm_dhcp_client_factory_dhclient; extern const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcd; diff --git a/src/dhcp/nm-dhcp-nettools.c b/src/dhcp/nm-dhcp-nettools.c index 292cfecc01..5191f5a4e9 100644 --- a/src/dhcp/nm-dhcp-nettools.c +++ b/src/dhcp/nm-dhcp-nettools.c @@ -55,8 +55,6 @@ typedef struct _NMDhcpNettools NMDhcpNettools; typedef struct _NMDhcpNettoolsClass NMDhcpNettoolsClass; -static GType nm_dhcp_nettools_get_type (void); - /*****************************************************************************/ typedef struct { diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 0e30ab3883..f0084f1b86 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -1186,6 +1186,8 @@ _get_type_per_addr_family (int addr_family) { nm_assert_addr_family (addr_family); + if (FALSE && addr_family == AF_INET) + return nm_dhcp_nettools_get_type (); return nm_dhcp_systemd_get_type (); }