[libvirt PATCH 1/2] util: Fix error return for virProcessKillPainfullyDelay()
Commit 93af79fb removed a cleanup label in favor of returning error values directly in certain cases. But the final return value was changed from -1 to 0. If we get to the end of the function, that means that we've waited for the process to exit but it still exists. So we should return -1. The error message was still being set correctly, but we were returning a success status (0). Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/util/virprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 31b833e5c8..6ce5ef99a9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -436,7 +436,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo _("Failed to terminate process %1$lld with SIG%2$s"), (long long)pid, signame); - return 0; + return -1; } -- 2.41.0
virProcessKillPainfullyDelay() currently almost always returns 1 or -1, even though the documentation indicates that it should return 0 if the process was terminated gracefully. The only case that it currently returns 0 is when it is called with the pid of a process that does not exist. This function initially sends a SIGTERM signal to the process. If the signal was sent successfully (i.e. virProcessKill() returns 0) we immediately sleep for 200ms before iterating the loop. On the second iteration of the loop, signum will be set to 0 and we call virProcessKill() again to check whether the process still exists. If the process does not exist (virProcessKill() returns -1 and errno is ESRCH), we consider the process killed successfully. But in order to determine what value to return from the function, we compare the value of signum to SIGTERM. But signum has already been set to 0; it will only ever be SIGTERM in the very first loop iteration. So it always indicates a forcible killing even when it is killed gracefully. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/util/virprocess.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 6ce5ef99a9..42b3b76eaa 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -377,6 +377,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo /* This is in 1/5th seconds since polling is on a 0.2s interval */ unsigned int polldelay = (force ? 200 : 75) + (extradelay*5); const char *signame = "TERM"; + int forced = 0; VIR_DEBUG("vpid=%lld force=%d extradelay=%u group=%d", (long long)pid, force, extradelay, group); @@ -399,6 +400,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo if (i == 0) { signum = SIGTERM; /* kindly suggest it should exit */ } else if (i == 50 && force) { + forced = 1; VIR_DEBUG("Timed out waiting after SIGTERM to process %lld, " "sending SIGKILL", (long long)pid); /* No SIGKILL kill on Win32 ! Use SIGABRT instead which our @@ -426,7 +428,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo (long long)pid, signame); return -1; } - return signum == SIGTERM ? 0 : 1; + return forced; } g_usleep(200 * 1000); -- 2.41.0
On a Friday in 2023, Jonathon Jongsma wrote:
virProcessKillPainfullyDelay() currently almost always returns 1 or -1, even though the documentation indicates that it should return 0 if the process was terminated gracefully. The only case that it currently returns 0 is when it is called with the pid of a process that does not exist.
All of the callers either check if the return value is less than zero, ignore it completely, or pass it up higher where one of the first two options happens. The return value was only used by virBhyveProcessStop until 2016: commit c35c2fe78eb05f9becf848393912e0d7ded70da2 bhyve: drop virProcessKillPainfully() from destroy I think we can just return 0 and drop the distinction from the function documentation. Jano
This function initially sends a SIGTERM signal to the process. If the signal was sent successfully (i.e. virProcessKill() returns 0) we immediately sleep for 200ms before iterating the loop. On the second iteration of the loop, signum will be set to 0 and we call virProcessKill() again to check whether the process still exists. If the process does not exist (virProcessKill() returns -1 and errno is ESRCH), we consider the process killed successfully. But in order to determine what value to return from the function, we compare the value of signum to SIGTERM. But signum has already been set to 0; it will only ever be SIGTERM in the very first loop iteration. So it always indicates a forcible killing even when it is killed gracefully.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/util/virprocess.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
On a Friday in 2023, Jonathon Jongsma wrote:
Commit 93af79fb removed a cleanup label in favor of returning error values directly in certain cases. But the final return value was changed from -1 to 0. If we get to the end of the function, that means that we've waited for the process to exit but it still exists. So we should return -1. The error message was still being set correctly, but we were returning a success status (0).
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/util/virprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jonathon Jongsma -
Ján Tomko