fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2026-01-05 13:50:15 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

dhcp: stop tracking NMDhcpClient instances from NMDhcpManager

Browse source 
NMDhcpManager was tracking DHCP clients. During start, it would check
whether an instance for the same ifindex is running, and stop it.

That seems unnecessary and wrong. Clearly, we cannot have multiple users
(like two `NMDevice`s) run DHCP on the same interface. But its up to
them to coordinate that. They also cannot configure IP addresses at the
same interface, and if they do, then there is a big problem already.

This comes from commit 1806235049 ('dhcp: convert dhcp backends to
classes'). Maybe back then there was also the idea that NetworkManager
could quit and leave dhclient running. That idea is also flawed. When
NetworkManager stops, it leaves the interface (possibly) up, so that
restart works without disruption. That does not mean that the DHCP
client needs to keep running. What works is to restart NetworkManager in
a timely manner, then NetworkManager will start a new DHCP client after
restart. What does not work is stop NetworkManager, do nothing (like
taking over the interface by running your own manager) and expect that
DHCP keeps working indefinitely. And of course, with the internal client
this cannot possibly work either. Don't stop NetworkManager for good, if
you expect NetworkManager to run DHCP on an interface.

A different things is that when NetworkManager crashes, that after
restart it kills the left over dhclient instances. That may require a
solution, for example systemd killing all processes or checking for
left-over PID files and kill the processes. But what was implemented in
NMDhcpManager was not a solution for that.

As such, it's not clear what conflicting instance we want to kill, or
why NMDhcpManager should even track NMDhcpClient instances.
This commit is contained in:
Thomas Haller 2021-08-06 09:08:56 +02:00
parent dbdd8303fc
commit 359d207d95
No known key found for this signature in database
GPG key ID: 29C2366E4DFC5728
3 changed files with 6 additions and 89 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/core/dhcp/nm-dhcp-client.c
View file

@ -1191,8 +1191,6 @@ nm_dhcp_client_init(NMDhcpClient *self)
priv = G_TYPE_INSTANCE_GET_PRIVATE(self, NM_TYPE_DHCP_CLIENT, NMDhcpClientPrivate);
self->_priv = priv;
c_list_init(&self->dhcp_client_lst);
priv->pid = -1;
}
@ -1229,8 +1227,6 @@ dispose(GObject *object)
* the DHCP client.
*/
nm_assert(c_list_is_empty(&self->dhcp_client_lst));
watch_cleanup(self);
timeout_cleanup(self);

1
src/core/dhcp/nm-dhcp-client.h
View file

@ -84,7 +84,6 @@ struct _NMDhcpClientPrivate;
typedef struct {
GObject parent;
struct _NMDhcpClientPrivate *_priv;
CList dhcp_client_lst;
} NMDhcpClient;
typedef enum _nm_packed {

90
src/core/dhcp/nm-dhcp-manager.c
View file

@ -27,7 +27,6 @@
typedef struct {
const NMDhcpClientFactory *client_factory;
char * default_hostname;
CList dhcp_client_lst_head;
} NMDhcpManagerPrivate;
struct _NMDhcpManager {
@ -45,11 +44,6 @@ G_DEFINE_TYPE(NMDhcpManager, nm_dhcp_manager, G_TYPE_OBJECT)
/*****************************************************************************/
static void
client_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDhcpManager *self);
/*****************************************************************************/
/* default to installed helper, but can be modified for testing */
const char *nm_dhcp_helper_path = LIBEXECDIR "/nm-dhcp-helper";
@ -135,53 +129,6 @@ _client_factory_get_gtype(const NMDhcpClientFactory *client_factory, int addr_fa
/*****************************************************************************/
static NMDhcpClient *
get_client_for_ifindex(NMDhcpManager *manager, int addr_family, int ifindex)
{
NMDhcpManagerPrivate *priv;
NMDhcpClient * client;
g_return_val_if_fail(NM_IS_DHCP_MANAGER(manager), NULL);
g_return_val_if_fail(ifindex > 0, NULL);
priv = NM_DHCP_MANAGER_GET_PRIVATE(manager);
c_list_for_each_entry (client, &priv->dhcp_client_lst_head, dhcp_client_lst) {
if (nm_dhcp_client_get_ifindex(client) == ifindex
&& nm_dhcp_client_get_addr_family(client) == addr_family)
return client;
}
return NULL;
}
static void
remove_client(NMDhcpManager *self, NMDhcpClient *client)
{
g_signal_handlers_disconnect_by_func(client, client_notify, self);
c_list_unlink(&client->dhcp_client_lst);
/* Stopping the client is left up to the controlling device
* explicitly since we may want to quit NetworkManager but not terminate
* the DHCP client.
*/
}
static void
remove_client_unref(NMDhcpManager *self, NMDhcpClient *client)
{
remove_client(self, client);
g_object_unref(client);
}
static void
client_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDhcpManager *self)
{
if (notify_data->notify_type == NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED
&& notify_data->state_changed.dhcp_state >= NM_DHCP_STATE_TIMEOUT)
remove_client_unref(self, client);
}
static NMDhcpClient *
client_start(NMDhcpManager * self,
int addr_family,
@ -212,10 +159,10 @@ client_start(NMDhcpManager * self,
GError ** error)
{
NMDhcpManagerPrivate *priv;
NMDhcpClient * client;
gboolean success = FALSE;
gsize hwaddr_len;
GType gtype;
gs_unref_object NMDhcpClient *client = NULL;
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);
@ -264,20 +211,6 @@ client_start(NMDhcpManager * self,
priv = NM_DHCP_MANAGER_GET_PRIVATE(self);
/* Kill any old client instance */
client = get_client_for_ifindex(self, addr_family, ifindex);
if (client) {
/* FIXME: we cannot just call synchronously "stop()" and forget about the client.
* We need to wait for the client to be fully stopped because most/all clients
* cannot quit right away.
*
* FIXME(shutdown): also fix this during shutdown, to wait for all DHCP clients
* to be fully stopped. */
remove_client(self, client);
nm_dhcp_client_stop(client, FALSE);
g_object_unref(client);
}
gtype = _client_factory_get_gtype(priv->client_factory, addr_family);
nm_log_trace(LOGD_DHCP,
@ -326,9 +259,6 @@ client_start(NMDhcpManager * self,
NM_DHCP_CLIENT_ANYCAST_ADDRESS,
anycast_address,
NULL);
nm_assert(client && c_list_is_empty(&client->dhcp_client_lst));
c_list_link_tail(&priv->dhcp_client_lst_head, &client->dhcp_client_lst);
g_signal_connect(client, NM_DHCP_CLIENT_NOTIFY, G_CALLBACK(client_notify), self);
/* unfortunately, our implementations work differently per address-family regarding client-id/DUID.
*
@ -368,12 +298,10 @@ client_start(NMDhcpManager * self,
error);
}
if (!success) {
remove_client_unref(self, client);
if (!success)
return NULL;
}
return g_object_ref(client);
return g_steal_pointer(&client);
}
/* Caller owns a reference to the NMDhcpClient on return */
@ -579,8 +507,6 @@ nm_dhcp_manager_init(NMDhcpManager *self)
int i;
const NMDhcpClientFactory *client_factory = NULL;
c_list_init(&priv->dhcp_client_lst_head);
for (i = 0; i < (int) G_N_ELEMENTS(_nm_dhcp_manager_factories); i++) {
const NMDhcpClientFactory *f = _nm_dhcp_manager_factories[i];
@ -651,10 +577,6 @@ dispose(GObject *object)
{
NMDhcpManager * self = NM_DHCP_MANAGER(object);
NMDhcpManagerPrivate *priv = NM_DHCP_MANAGER_GET_PRIVATE(self);
NMDhcpClient * client, *client_safe;
c_list_for_each_entry_safe (client, client_safe, &priv->dhcp_client_lst_head, dhcp_client_lst)
remove_client_unref(self, client);
G_OBJECT_CLASS(nm_dhcp_manager_parent_class)->dispose(object);