[Openvpn-devel,M] Change in openvpn[master]: t_server_null: multiple improvements and fixes

Message ID 4711ccb96b777449406ef34ee3d2e6a27aeaf9e8-HTML@gerrit.openvpn.net
State Superseded
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: t_server_null: multiple improvements and fixes | expand

Commit Message

flichtenheld (Code Review) June 20, 2024, 4:06 p.m. UTC
Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/669?usp=email

to review the following change.


Change subject: t_server_null: multiple improvements and fixes
......................................................................

t_server_null: multiple improvements and fixes

- exit after a timeout if unable to kill servers
- use sudo or equivalent only for server stop/start
- use /bin/sh directly instead of through /usr/bin/env
- simplify sudo call in the sample rc file
- remove misleading and outdated documentation

Change-Id: I2cce8ad4e0d262e1404ab1eb6ff673d8590b6b3a
Signed-off-by: Samuli Seppänen <samuli.seppanen@gmail.com>
---
M doc/t_server_null.rst
M tests/t_server_null.rc-sample
M tests/t_server_null.sh
M tests/t_server_null_client.sh
M tests/t_server_null_server.sh
M tests/t_server_null_stress.sh
6 files changed, 41 insertions(+), 32 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/69/669/1

Patch

diff --git a/doc/t_server_null.rst b/doc/t_server_null.rst
index e3a098a..7ce7a3f 100644
--- a/doc/t_server_null.rst
+++ b/doc/t_server_null.rst
@@ -73,13 +73,6 @@ 
 
       * Waits until servers have launched. Then launch all clients, wait for them to exit and then check test results by parsing the client log files. Each client kills itself after some delay using an "--up" script.
 
-Note that "make check" moves on once *t_server_null_client.sh* has exited. At
-that point *t_server_null_server.sh* is still running, because it exists only
-after waiting a few seconds for more client connections to potentially appear.
-This is a feature and not a bug, but means that launching "make check" runs too
-quickly might cause test failures or unexpected behavior such as leftover
-OpenVPN server processes.
-
 Configuration
 -------------
 
diff --git a/tests/t_server_null.rc-sample b/tests/t_server_null.rc-sample
index 28c3773..98d7869 100644
--- a/tests/t_server_null.rc-sample
+++ b/tests/t_server_null.rc-sample
@@ -1,6 +1,5 @@ 
 # Uncomment to run tests with sudo
-#SUDO_EXEC=`which sudo`
-#RUN_SUDO="${SUDO_EXEC} -E"
+#RUN_SUDO="sudo -E"
 
 TEST_RUN_LIST="1 2 3 10 11"
 
diff --git a/tests/t_server_null.sh b/tests/t_server_null.sh
index 0e53ba4..7627edf 100755
--- a/tests/t_server_null.sh
+++ b/tests/t_server_null.sh
@@ -1,4 +1,4 @@ 
-#!/usr/bin/env sh
+#!/bin/sh
 #
 TSERVER_NULL_SKIP_RC="${TSERVER_NULL_SKIP_RC:-77}"
 
@@ -57,12 +57,7 @@ 
 
 srcdir="${srcdir:-.}"
 
-if [ -z "${RUN_SUDO}" ]; then
-    "${srcdir}/t_server_null_server.sh" &
-else
-    $RUN_SUDO "${srcdir}/t_server_null_server.sh" &
-fi
-
+"${srcdir}/t_server_null_server.sh" &
 "${srcdir}/t_server_null_client.sh"
 retval=$?
 
diff --git a/tests/t_server_null_client.sh b/tests/t_server_null_client.sh
index 8890007..3b7dc6d 100755
--- a/tests/t_server_null_client.sh
+++ b/tests/t_server_null_client.sh
@@ -1,4 +1,4 @@ 
-#!/usr/bin/env sh
+#!/bin/sh
 
 launch_client() {
     test_name=$1
diff --git a/tests/t_server_null_server.sh b/tests/t_server_null_server.sh
index 9bc0c88..037baf7 100755
--- a/tests/t_server_null_server.sh
+++ b/tests/t_server_null_server.sh
@@ -1,4 +1,4 @@ 
-#!/usr/bin/env sh
+#!/bin/sh
 
 launch_server() {
     server_name=$1
@@ -8,16 +8,23 @@ 
     status="${server_name}.status"
     pid="${server_name}.pid"
 
-    # Ensure that old status, log and pid files are gone
-    rm -f "${status}" "${log}" "${pid}"
-
-    "${server_exec}" \
-        $server_conf \
-        --status "${status}" 1 \
-        --log "${log}" \
-        --writepid "${pid}" \
-        --explicit-exit-notify 3
-
+    if [ -z "${RUN_SUDO}" ]; then
+        rm -f "${status}" "${log}" "${pid}"
+        "${server_exec}" \
+         $server_conf \
+         --status "${status}" 1 \
+         --log "${log}" \
+         --writepid "${pid}" \
+         --explicit-exit-notify 3
+    else
+        $RUN_SUDO rm -f "${status}" "${log}" "${pid}"
+        $RUN_SUDO "${server_exec}" \
+                   $server_conf \
+                   --status "${status}" 1 \
+                   --log "${log}" \
+                   --writepid "${pid}" \
+                   --explicit-exit-notify 3
+    fi
 }
 
 # Load base/default configuration
@@ -64,15 +71,30 @@ 
 
 echo "All clients have disconnected from all servers"
 
+# Make sure that the server processes are truly dead before exiting.  If a
+# server process does not exit in 15 seconds assume it never will, move on and
+# hope for the best.
+echo "Waiting for servers to exit"
 for PID_FILE in $server_pid_files
 do
     SERVER_PID=$(cat "${PID_FILE}")
-    $KILL_EXEC "${SERVER_PID}"
 
-    # Make sure that the server processes are truly dead before exiting
-    while :
+    if [ -z "${RUN_SUDO}" ]; then
+        $KILL_EXEC "${SERVER_PID}"
+    else
+        $RUN_SUDO $KILL_EXEC "${SERVER_PID}"
+    fi
+
+    count=0
+    maxcount=75
+    while [ $count -le $maxcount ]
     do
         ps -p "${SERVER_PID}" > /dev/null || break
+        count=$(( count + 1))
         sleep 0.2
     done
+
+    if [ $count -ge $maxcount ]; then
+        echo "WARNING: could not kill server with pid ${SERVER_PID}!"
+    fi
 done
diff --git a/tests/t_server_null_stress.sh b/tests/t_server_null_stress.sh
index 1281397..0bb9452 100755
--- a/tests/t_server_null_stress.sh
+++ b/tests/t_server_null_stress.sh
@@ -1,4 +1,4 @@ 
-#!/usr/bin/env sh
+#!/bin/sh
 #
 # Run this stress test as root to avoid sudo authorization from timing out.