dhcp: stop tracking NMDhcpClient instances from NMDhcpManager

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 18062350492a ('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.

3 files changed, 6 insertions, 89 deletions

diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c
index 972bc5f583..0985e386e1 100644
--- a/src/core/dhcp/nm-dhcp-client.c
+++ b/src/core/dhcp/nm-dhcp-client.c
@@ -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);
 
diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h
index 49deb3305a..2e7e021650 100644
--- a/src/core/dhcp/nm-dhcp-client.h
+++ b/src/core/dhcp/nm-dhcp-client.h
@@ -84,7 +84,6 @@ struct _NMDhcpClientPrivate;
 typedef struct {
     GObject                      parent;
     struct _NMDhcpClientPrivate *_priv;
-    CList                        dhcp_client_lst;
 } NMDhcpClient;
 
 typedef enum _nm_packed {
diff --git a/src/core/dhcp/nm-dhcp-manager.c b/src/core/dhcp/nm-dhcp-manager.c
index 0bcc6e1959..3cb893932d 100644
--- a/src/core/dhcp/nm-dhcp-manager.c
+++ b/src/core/dhcp/nm-dhcp-manager.c
@@ -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";
 
@@ -136,53 +130,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,
              NMDedupMultiIndex *       multi_idx,
@@ -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);