utils: remove timeout argument from nm_utils_term_child_async()lr/simplify-child-kill

This simplifies things a little -- 5 seconds should be enough for
everybody.

It also increases the timeout it all cases. This is done so that the
timeout is also good enough for pppd, which is generally the slowest
thing to shut down.

On orderly shutdown, pp likes to tell the other side. This seems to take
at least a second even when no real network latency is at play, on busy
systems 1.5 seconds easily ends up being inadequate.

A violent pppd shutdown is generally okay apart from that it can leave
garbage (port lock) behind and the other side potentially confused for a
while.

As it happens, this interacts badly with modemu.pl which is used for
testing: the pseudo terminal in PPP line discipline mode has no idea that
the remote disconnected and while ModemManager is learning that something
wrong the hard way (AT command timing out, because the remote still expects
to talk PPP), the test times out.

Fixes-test: @gsm_sim_mtu
https://bugzilla.redhat.com/show_bug.cgi?id=2049596

7 files changed, 12 insertions, 24 deletions

diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c
index b5531799a0..c6309722f2 100644
--- a/src/core/devices/nm-device.c
+++ b/src/core/devices/nm-device.c
@@ -13373,7 +13373,6 @@ ip_check_gw_ping_cleanup(NMDevice *self)
         nm_utils_term_child_async(priv->gw_ping.pid,
                                   priv->gw_ping.log_domain,
                                   "ping",
-                                  1000,
                                   NULL,
                                   NULL);
         priv->gw_ping.pid = 0;
diff --git a/src/core/devices/team/nm-device-team.c b/src/core/devices/team/nm-device-team.c
index 2b8005d089..88fabf3a39 100644
--- a/src/core/devices/team/nm-device-team.c
+++ b/src/core/devices/team/nm-device-team.c
@@ -290,7 +290,6 @@ teamd_cleanup(NMDeviceTeam *self, gboolean free_tdc)
         nm_utils_term_child_async(priv->teamd_pid,
                                   LOGD_TEAM,
                                   "teamd",
-                                  2000,
                                   teamd_kill_cb,
                                   g_object_ref(self));
         priv->teamd_pid = 0;
diff --git a/src/core/dnsmasq/nm-dnsmasq-manager.c b/src/core/dnsmasq/nm-dnsmasq-manager.c
index 88f8fc7261..9026b0400c 100644
--- a/src/core/dnsmasq/nm-dnsmasq-manager.c
+++ b/src/core/dnsmasq/nm-dnsmasq-manager.c
@@ -299,7 +299,7 @@ nm_dnsmasq_manager_stop(NMDnsMasqManager *manager)
     nm_clear_g_source(&priv->dm_watch_id);
 
     if (priv->pid) {
-        nm_utils_term_child_async(priv->pid, LOGD_SHARING, "dnsmasq", 2000, NULL, NULL);
+        nm_utils_term_child_async(priv->pid, LOGD_SHARING, "dnsmasq", NULL, NULL);
         priv->pid = 0;
     }
 
diff --git a/src/core/nm-core-utils.c b/src/core/nm-core-utils.c
index cb62f61c34..06ebc52e0a 100644
--- a/src/core/nm-core-utils.c
+++ b/src/core/nm-core-utils.c
@@ -569,12 +569,17 @@ _kill_child_async(pid_t                   pid,
  * @pid: the process id of the process to kill
  * @log_domain: the logging domain used for logging (LOGD_NONE to suppress logging)
  * @log_name: for logging, the name of the processes to kill
- * @wait_before_kill_msec: Waittime in milliseconds before sending %SIGKILL signal. Set this value
  * to zero, not to send %SIGKILL. If @sig is already %SIGKILL, this parameter is ignored.
  * @callback: (allow-none): callback after the child terminated. This function will always
  *   be invoked asynchronously.
  * @user_data: passed on to callback
  *
+ * Sends a SIGTERM to a process, then waits 5 seconds and unless the process
+ * has terminated it forces exit with SIGKILL.
+ *
+ * The value of 5 seconds is chosen to give even the sluggiest programs (pppd)
+ * a fair chance to clean up. Change with caution!
+ *
  * Uses g_child_watch_add(), so note the glib comment: if you obtain pid from g_spawn_async() or
  * g_spawn_async_with_pipes() you will need to pass %G_SPAWN_DO_NOT_REAP_CHILD as flag to the spawn
  * function for the child watching to work.
@@ -585,11 +590,10 @@ void
 nm_utils_term_child_async(pid_t                   pid,
                           NMLogDomain             log_domain,
                           const char             *log_name,
-                          guint32                 wait_before_kill_msec,
                           NMUtilsKillChildAsyncCb callback,
                           void                   *user_data)
 {
-    _kill_child_async(pid, SIGTERM, log_domain, log_name, wait_before_kill_msec, callback, user_data);
+    _kill_child_async(pid, SIGTERM, log_domain, log_name, 5000, callback, user_data);
 }
 
 static gulong
diff --git a/src/core/nm-core-utils.h b/src/core/nm-core-utils.h
index 305544c89a..c20c1e2ec5 100644
--- a/src/core/nm-core-utils.h
+++ b/src/core/nm-core-utils.h
@@ -167,7 +167,6 @@ typedef void (*NMUtilsKillChildAsyncCb)(pid_t    pid,
 void     nm_utils_term_child_async(pid_t                   pid,
                                    guint64                 log_domain,
                                    const char             *log_name,
-                                   guint32                 wait_before_kill_msec,
                                    NMUtilsKillChildAsyncCb callback,
                                    void                   *user_data);
 gboolean nm_utils_kill_child_sync(pid_t       pid,
diff --git a/src/core/ppp/nm-ppp-manager.c b/src/core/ppp/nm-ppp-manager.c
index d762030869..57c32a61df 100644
--- a/src/core/ppp/nm-ppp-manager.c
+++ b/src/core/ppp/nm-ppp-manager.c
@@ -1245,7 +1245,6 @@ _ppp_manager_stop(NMPPPManager            *self,
     nm_utils_term_child_async(nm_steal_int(&priv->pid),
                               LOGD_PPP,
                               "pppd",
-                              NM_SHUTDOWN_TIMEOUT_MS,
                               _stop_child_cb,
                               handle);
 
diff --git a/src/core/tests/test-core-with-expect.c b/src/core/tests/test-core-with-expect.c
index 09a6f3744d..af342ea639 100644
--- a/src/core/tests/test-core-with-expect.c
+++ b/src/core/tests/test-core-with-expect.c
@@ -95,7 +95,6 @@ test_nm_utils_term_child_async_fail_cb(void *user_data)
 static void
 test_nm_utils_term_child_async_do(const char *name,
                                   pid_t       pid,
-                                  guint32     wait_before_kill_msec,
                                   gboolean    expected_success,
                                   const int  *expected_child_status)
 {
@@ -110,7 +109,6 @@ test_nm_utils_term_child_async_do(const char *name,
     nm_utils_term_child_async(pid,
                               LOGD_CORE,
                               name,
-                              wait_before_kill_msec,
                               test_nm_utils_term_child_async_cb,
                               &data);
     g_assert(!data.called);
@@ -256,7 +254,7 @@ do_test_nm_utils_kill_child(void)
         "trap \"while true; do :; done\" TERM; while true; do :; done; #" TEST_TOKEN,
         NULL,
     };
-    pid_t pid1a_1, pid2a, pid4a;
+    pid_t pid1a_1, pid2a;
     pid_t pid1s_1, pid1s_2, pid1s_3, pid2s, pid3s, pid4s;
 
     const int expected_exit_47     = 12032; /* exit with status 47 */
@@ -274,7 +272,6 @@ do_test_nm_utils_kill_child(void)
 
     pid1a_1 = test_nm_utils_kill_child_spawn(argv1, TRUE);
     pid2a   = test_nm_utils_kill_child_spawn(argv2, TRUE);
-    pid4a   = test_nm_utils_kill_child_spawn(argv4, TRUE);
 
     /* give processes time to start (and potentially block signals) ... */
     g_usleep(G_USEC_PER_SEC / 10);
@@ -341,12 +338,11 @@ do_test_nm_utils_kill_child(void)
     test_nm_utils_kill_child_sync_do("test-s-4", pid4s, SIGTERM, 1, TRUE, &expected_signal_KILL);
 
     NMTST_EXPECT_NM_DEBUG("kill child process 'test-a-1-1' (*): wait for process to terminate "
-                          "after sending SIGTERM (15) (send SIGKILL in 3000 milliseconds)...");
+                          "after sending SIGTERM (15) (send SIGKILL in 5000 milliseconds)...");
     NMTST_EXPECT_NM_DEBUG(
         "kill child process 'test-a-1-1' (*): terminated by signal 15 (* usec elapsed)");
     test_nm_utils_term_child_async_do("test-a-1-1",
                                       pid1a_1,
-                                      3000,
                                       TRUE,
                                       &expected_signal_TERM);
 
@@ -354,7 +350,7 @@ do_test_nm_utils_kill_child(void)
         "kill child process 'test-a-2' (*): process * already terminated normally with status 47");
     NMTST_EXPECT_NM_DEBUG(
         "kill child process 'test-a-2' (*): invoke callback: terminated normally with status 47");
-    test_nm_utils_term_child_async_do("test-a-2", pid2a, 3000, TRUE, &expected_exit_47);
+    test_nm_utils_term_child_async_do("test-a-2", pid2a, TRUE, &expected_exit_47);
 
     /* pid3s should not be a valid process, hence the call should fail. Note, that there
      * is a race here. */
@@ -363,15 +359,7 @@ do_test_nm_utils_kill_child(void)
         "(No child process*, 10) after sending SIGTERM (15)");
     NMTST_EXPECT_NM_DEBUG(
         "kill child process 'test-a-3-2' (*): invoke callback: killing child failed");
-    test_nm_utils_term_child_async_do("test-a-3-2", pid3s, 0, FALSE, NULL);
-
-    NMTST_EXPECT_NM_DEBUG("kill child process 'test-a-4' (*): wait for process to terminate after "
-                          "sending SIGTERM (15) (send SIGKILL in 1 milliseconds)...");
-    NMTST_EXPECT_NM_DEBUG("kill child process 'test-a-4' (*): process not terminated after * usec. "
-                          "Sending SIGKILL signal");
-    NMTST_EXPECT_NM_DEBUG(
-        "kill child process 'test-a-4' (*): terminated by signal 9 (* usec elapsed)");
-    test_nm_utils_term_child_async_do("test-a-4", pid4a, 1, TRUE, &expected_signal_KILL);
+    test_nm_utils_term_child_async_do("test-a-3-2", pid3s, FALSE, NULL);
 
     g_log_set_always_fatal(fatal_mask);