Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

should wslbridge2 return child exit status? #28

Open
iuliantatarascu opened this issue Oct 25, 2020 · 4 comments
Open

should wslbridge2 return child exit status? #28

iuliantatarascu opened this issue Oct 25, 2020 · 4 comments

Comments

@iuliantatarascu
Copy link

Now that mintty/wsltty#254 is fixed, should wslbridge2 propagate the backend/child exit status, as suggested in this comment mintty/wsltty#254 (comment)?

This would allow mintty --hold error from wsltty to close the window upon a successful command, and hold the window open if the command returns non-zero exit code.

I tried implementing it like this:

diff, CLICK ME
diff --git a/src/wslbridge2-backend.cpp b/src/wslbridge2-backend.cpp
index ad60a1f..13dceae 100644
--- a/src/wslbridge2-backend.cpp
+++ b/src/wslbridge2-backend.cpp
@@ -306,6 +306,7 @@ int main(int argc, char *argv[])
     act.sa_sigaction = [](int x, siginfo_t *y, void *z) { wait(NULL); };
     sigaction(SIGCHLD, &act, NULL);
 
+    int childExitStatus = -1;
     int mfd;
     char ptyname[16];
     const pid_t child = forkpty(&mfd, ptyname, NULL, &winp);
@@ -372,6 +373,18 @@ int main(int argc, char *argv[])
             /* Shutdown I/O sockets when child process terminates */
             if (fds[2].revents & (POLLERR | POLLHUP))
             {
+                int status;
+                if (wait(&status))
+                {
+                    if (WIFEXITED(status))
+                    {
+                        childExitStatus = WEXITSTATUS(status);
+                        printf("exit-status 0x%x 0x%x\n", childExitStatus, status);
+                    }
+                } else {
+                    perror("wait");
+                }
+
                 for (size_t i = 0; i < ARRAYSIZE(ioSockets.sock); i++)
                     shutdown(ioSockets.sock[i], SHUT_RDWR);
 
@@ -454,7 +467,7 @@ int main(int argc, char *argv[])
          * Do not use exit() because it performs clean-up
          * related to user-mode constructs in the library
          */
-        _exit(0);
+        _exit(-1);
     }
     else
         perror("fork");
@@ -463,5 +476,5 @@ int main(int argc, char *argv[])
     for (size_t i = 0; i < ARRAYSIZE(ioSockets.sock); i++)
         close(ioSockets.sock[i]);
 
-    return 0;
+    return childExitStatus;
 }
diff --git a/src/wslbridge2.cpp b/src/wslbridge2.cpp
index f0b49ae..595a22e 100644
--- a/src/wslbridge2.cpp
+++ b/src/wslbridge2.cpp
@@ -481,7 +481,8 @@ int main(int argc, char *argv[])
     if (!ret)
         fatal("SetHandleInformation: %s", GetErrorMessage(GetLastError()).c_str());
 
-    const auto watchdog = std::thread([&]() {
+    DWORD exitCode = 0;
+    auto watchdog = std::thread([&]() {
         WaitForSingleObject(pi.hProcess, INFINITE);
 
         std::vector<char> outVec = readAllFromHandle(outputPipe.rh);
@@ -501,6 +502,10 @@ int main(int argc, char *argv[])
             msg.append(err);
         }
 
+        if (!GetExitCodeProcess(pi.hProcess, &exitCode)) {
+            exitCode = -1;
+        }
+
         /* Exit with error if output/error message is not empty. */
         if (!msg.empty())
             termState.fatal("%s", msg.c_str());
@@ -546,6 +551,8 @@ int main(int argc, char *argv[])
     ret = pthread_create(&tidOutput, nullptr, receive_buffer, nullptr);
     assert(ret == 0);
 
+    watchdog.join();
+
     termState.enterRawMode();
 
     /*
@@ -562,5 +569,5 @@ int main(int argc, char *argv[])
     CloseHandle(pi.hProcess);
     CloseHandle(pi.hThread);
     WSACleanup();
-    termState.exitCleanly(0);
+    termState.exitCleanly(exitCode);
 }

But it doesn't seem to always work. Running ./bin/wslbridge2.exe /bin/false should always return 1. The proper exit code from wait() is not always captured, sometimes WEXITSTATUS wrongly gives 0, sometimes it correctly gives the real status code from the child.

wslbridge2 version e4b44dd236c587613e461d56011016292051d05d

@Biswa96
Copy link
Owner

Biswa96 commented Oct 27, 2020

I have added something for this. Would you like to check it? I have not properly though about the propagation of exit status to frontend.

@iuliantatarascu
Copy link
Author

I guess the exit status was unreliable because of a race condition with the previous wait

act.sa_sigaction = [](int x, siginfo_t *y, void *z) { wait(NULL); };

I have the full propagation working in my fork https://github.com/iuliantatarascu/wslbridge2/commit/626aa01765d408d365529c27e552760a886e9d02

I don't know what scenarios to test, to make sure I didn't break other things.

@iuliantatarascu
Copy link
Author

It returns the child's actual exit code on successful execution, and -1 if any other error occurs along the way.

I have only tested with WSL1, I don't know about WSL2, things might be different there.

@Biswa96
Copy link
Owner

Biswa96 commented Oct 31, 2020

BTW, in your same code the exit status is messed up. If GetExitCodeProcess() is added in the watchdog thread then it will capture the return value of wsl.exe process NOT the exit status of child process of wslbridge2-backend. The proper way is to send the exit status through socket. See wslbridge project, parent of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants