GNOME Bugzilla – Bug 123591
vte_terminal_fork_command succeeds even when it does not
Last modified: 2007-01-23 10:13:14 UTC
vte_terminal_fork_command seems to return successfully, even when it could not exec the command. To reproduce: create a `Problematic' profile in gnome-terminal, and set the command to be executed to a non existent one, say, /usr/bin/this-does-not-exist; then say gnome-terminal --disable-factory --window-with-profile=Problematic Nothing will happen... gdb'ing shows that vte_terminal_fork_command returns PID of something which no longer exists in terminal-widget-vte.c(terminal_widget_fork_command)
The problem lies in the pty.c(_vte_pty_run_on_pty). Custom commands are executed without checking for return values or error codes: if (argv != NULL) { for (i = 0; (argv[i] != NULL); i++) ; args = g_malloc0(sizeof(char*) * (i + 1)); for (i = 0; (argv[i] != NULL); i++) { args[i] = g_strdup(argv[i]); } execvp(command, args); } else { arg = g_strdup(command); execlp(command, arg, NULL); } /* Avoid calling any atexit() code. */ _exit(0); g_assert_not_reached();
Created attachment 50620 [details] [review] possible patch Any chance that this might help?
Ping?
Would be nice to get the exit status of programs in general.
Chris Wilson pointed out that the patch doesn't work. What needs to be done is to communicate the failure from the forked process to the parent. To not reinvent the wheel, we should be using g_spawn_async_with_pipes().
Created attachment 80838 [details] [review] Use g_spawn to execute the Throw out a lot of code and replace it with g_spawn_async_with_pipes. s/pid_t/GPid/. If the API wasn't involatile then we could propagate the GError up to caller...
The addition of the g_warning("Failed to spawn") is for testing purposes only...
Quite a lot of black magic code removed. Does g_spawn do all those ioctl stuff?
Not directly, but provides a child_setup callback into which we put the process leader and terminal ioctls - nicely consolidating it into one place. g-t is happily running shells which I take as a good sign that the terminal emulation is working and pops up a dialog box if you try to execute /this/command/does/not/exist. Wow!
Commit then!
Loïc, please open a separate enhancement request if you want a child-exited-with-status style signal. r1493: 2007-01-22 Chris Wilson <chris@chris-wilson.co.uk> Bug 123591 – vte_terminal_fork_command succeeds even when it does not * python/vte.defs: * python/vte.override: * src/reaper.c: (vte_reaper_child_watch_cb): * src/vte-private.h: * src/vte.c: (vte_terminal_catch_child_exited), (_vte_terminal_fork_basic), (vte_terminal_forkpty): * src/vte.h: * src/vteapp.c: s/pid_t/GPid/ portable variant, no API/ABI implications * src/pty.c: (vte_pty_child_setup), (_vte_pty_run_on_pty), (_vte_pty_fork_on_pty_name), (_vte_pty_fork_on_pty_fd), (_vte_pty_open_unix98), (_vte_pty_pipe_open), (n_read), (n_write), (_vte_pty_start_helper), (_vte_pty_open_with_helper), (_vte_pty_open), (main): Replace custom fork() and pipe based syncrohonisation scheme with g_spawn_async_with_pipes() as it actually handles all failure modes correctly, and gives us overtures of portability.
Mariano points out that he can no longer open a tab [in g-t] to a non-existent starting directory. He also, kindly, pointed out that the old method used to failover to the pwd if the requested directory was non-existent.
r1510: 2007-01-23 Chris Wilson <chris@chris-wilson.co.uk> Regression on Bug 123591: mkdir /tmp/aa; cd /tmp/aa; bash; cd ..; rmdir aa Open a new tab in g-t -> Failure. * src/pty.c: (_vte_pty_run_on_pty): Check the error return from g_spawn and try again with the pwd if it reports that the child could not change directory.