On 09/09/11 05:08, Bram Moolenaar wrote:
> Tim Starling wrote:
>> 1. Fork, call gtk_init() in the child, and have the parent wait to see
>> if the child successfully starts the GUI before exiting. If the child
>> fails to start the GUI, the child would exit and the parent would
>> continue.
> Going for the first option would be best. It's not that easy to
> implement, but once done it should avoid problems in the future.
> Note that with ":gui" you can start the GUI any time, also with a
> modified buffer. That's why option 2 is not acceptable.
Please find attached a Mercurial changeset which fixes the
GtkFileChooser problem with a solution along these lines.
It turns out that for vim -g, gtk_init() is called long before
gui_start() or termcapinit(). As the RedHat bug suggested, the more
serious problem was the fact that Vim was creating a window before
forking, not that it was calling gtk_init() before forking.
In fact termcapinit() calls gui_mch_init(), which on GTK/X11 returns
OK unconditionally. There are a few platforms where it is possible for
gui_mch_init() to return FAIL, so I tested that error case by
temporarily patching gui_mch_init() to fail unconditionally.
It would be better if opening the X display could be delayed until
after the fork, but it looks like that would take longer to implement
than the amount of time I have to offer at the moment. The current
code works well enough for the common cases, and for the apparently
unreachable error cases, it fails without segfaulting or destroying
data. Specifically:
* "DISPLAY=foo vim -g": gives error "E233" as it always has
* "DISPLAY=foo vim" then type ":gui" : gives the new error "E852: The
child process failed to start the GUI". The terminal is cleared and
the error message not immediately visible, but it is in :messages.
* "vim -g" with gui_mch_init patched to return FAIL: gives "E852" and
a press-enter prompt.
* "vim" then ":gui" with gui_mch_init patched to return FAIL: Terminal
modes are corrupted due to an unexpected longjmp(). However, vim is
still responsive, and "E852" is visible in :messages.
The attached changeset starts the GUI after the fork is done. The
pre-existing pipe() code was extended to allow the child to signal to
the parent whether or not the GUI was started successfully. The first
two commits are refactoring only, the third commit is the main part of
it, and the fourth is cleanup.
-- Tim Starling
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
# HG changeset patch
# User tstarl...@wikimedia.org
# Date 1315551475 -36000
# Node ID 85606be689a869520ddbe8f427dc234bec8207dc
# Parent 1d5e7413d728619f55fe574cad436a1de2686cae
Split gui_do_fork() out of gui_start(), for readability.
diff -r 1d5e7413d728 -r 85606be689a8 src/gui.c
--- a/src/gui.c Thu Sep 08 23:24:14 2011 +0200
+++ b/src/gui.c Fri Sep 09 16:57:55 2011 +1000
@@ -37,6 +37,12 @@
static void gui_set_bg_color __ARGS((char_u *name));
static win_T *xy2win __ARGS((int x, int y));
+#if defined(UNIX) && !defined(__BEOS__) && !defined(MACOS_X) \
+ && !defined(__APPLE__)
+# define MAY_FORK
+static void gui_do_fork __ARGS((void));
+#endif
+
static int can_update_cursor = TRUE; /* can display the cursor */
/*
@@ -59,9 +65,7 @@
gui_start()
{
char_u *old_term;
-#if defined(UNIX) && !defined(__BEOS__) && !defined(MACOS_X) \
- && !defined(__APPLE__)
-# define MAY_FORK
+#ifdef MAY_FORK
int dofork = TRUE;
#endif
static int recursive = 0;
@@ -124,7 +128,7 @@
}
#endif
-#if defined(MAY_FORK) && !defined(__QNXNTO__)
+#if defined(MAY_FORK)
/*
* Quit the current process and continue in the child.
* Makes "gvim file" disconnect from the shell it was started in.
@@ -133,73 +137,8 @@
*/
if (gui.in_use && dofork)
{
- int pipefd[2]; /* pipe between parent and child */
- int pipe_error;
- char dummy;
- pid_t pid = -1;
-
- /* Setup a pipe between the child and the parent, so that the parent
- * knows when the child has done the setsid() call and is allowed to
- * exit. */
- pipe_error = (pipe(pipefd) < 0);
- pid = fork();
- if (pid > 0) /* Parent */
- {
- /* Give the child some time to do the setsid(), otherwise the
- * exit() may kill the child too (when starting gvim from inside a
- * gvim). */
- if (pipe_error)
- ui_delay(300L, TRUE);
- else
- {
- /* The read returns when the child closes the pipe (or when
- * the child dies for some reason). */
- close(pipefd[1]);
- ignored = (int)read(pipefd[0], &dummy, (size_t)1);
- close(pipefd[0]);
- }
-
- /* When swapping screens we may need to go to the next line, e.g.,
- * after a hit-enter prompt and using ":gui". */
- if (newline_on_exit)
- mch_errmsg("\r\n");
-
- /*
- * The parent must skip the normal exit() processing, the child
- * will do it. For example, GTK messes up signals when exiting.
- */
- _exit(0);
- }
-
-# if defined(HAVE_SETSID) || defined(HAVE_SETPGID)
- /*
- * Change our process group. On some systems/shells a CTRL-C in the
- * shell where Vim was started would otherwise kill gvim!
- */
- if (pid == 0) /* child */
-# if defined(HAVE_SETSID)
- (void)setsid();
-# else
- (void)setpgid(0, 0);
-# endif
-# endif
- if (!pipe_error)
- {
- close(pipefd[0]);
- close(pipefd[1]);
- }
-
-# if defined(FEAT_GUI_GNOME) && defined(FEAT_SESSION)
- /* Tell the session manager our new PID */
- gui_mch_forked();
-# endif
+ gui_do_fork();
}
-#else
-# if defined(__QNXNTO__)
- if (gui.in_use && dofork)
- procmgr_daemon(0, PROCMGR_DAEMON_KEEPUMASK | PROCMGR_DAEMON_NOCHDIR |
- PROCMGR_DAEMON_NOCLOSE | PROCMGR_DAEMON_NODEVNULL);
-# endif
#endif
#ifdef FEAT_AUTOCMD
@@ -213,6 +152,76 @@
--recursive;
}
+static void gui_do_fork()
+{
+#ifdef __QNXNTO__
+ procmgr_daemon(0, PROCMGR_DAEMON_KEEPUMASK | PROCMGR_DAEMON_NOCHDIR |
+ PROCMGR_DAEMON_NOCLOSE | PROCMGR_DAEMON_NODEVNULL);
+ return;
+#else
+ int pipefd[2]; /* pipe between parent and child */
+ int pipe_error;
+ char dummy;
+ pid_t pid = -1;
+
+ /* Setup a pipe between the child and the parent, so that the parent
+ * knows when the child has done the setsid() call and is allowed to
+ * exit. */
+ pipe_error = (pipe(pipefd) < 0);
+ pid = fork();
+ if (pid > 0) /* Parent */
+ {
+ /* Give the child some time to do the setsid(), otherwise the
+ * exit() may kill the child too (when starting gvim from inside a
+ * gvim). */
+ if (pipe_error)
+ ui_delay(300L, TRUE);
+ else
+ {
+ /* The read returns when the child closes the pipe (or when
+ * the child dies for some reason). */
+ close(pipefd[1]);
+ ignored = (int)read(pipefd[0], &dummy, (size_t)1);
+ close(pipefd[0]);
+ }
+
+ /* When swapping screens we may need to go to the next line, e.g.,
+ * after a hit-enter prompt and using ":gui". */
+ if (newline_on_exit)
+ mch_errmsg("\r\n");
+
+ /*
+ * The parent must skip the normal exit() processing, the child
+ * will do it. For example, GTK messes up signals when exiting.
+ */
+ _exit(0);
+ }
+
+# if defined(HAVE_SETSID) || defined(HAVE_SETPGID)
+ /*
+ * Change our process group. On some systems/shells a CTRL-C in the
+ * shell where Vim was started would otherwise kill gvim!
+ */
+ if (pid == 0) /* child */
+# if defined(HAVE_SETSID)
+ (void)setsid();
+# else
+ (void)setpgid(0, 0);
+# endif
+# endif
+ if (!pipe_error)
+ {
+ close(pipefd[0]);
+ close(pipefd[1]);
+ }
+
+# if defined(FEAT_GUI_GNOME) && defined(FEAT_SESSION)
+ /* Tell the session manager our new PID */
+ gui_mch_forked();
+# endif
+#endif
+}
+
/*
* Call this when vim starts up, whether or not the GUI is started
*/
# HG changeset patch
# User tstarl...@wikimedia.org
# Date 1315553496 -36000
# Node ID 59a60c369292cf19f8635ab721c520bb9221ceac
# Parent 85606be689a869520ddbe8f427dc234bec8207dc
Split the part of gui_start() that actually starts the GUI, as opposed to forking and restoring terminal settings and what not, out to gui_attempt_start(). Should have the same overall function, since I'm only delaying the assignment of gui.starting very slightly, and moving an "if (gui.in_use)" above an "if (!gui.in_use)" block.
diff -r 85606be689a8 -r 59a60c369292 src/gui.c
--- a/src/gui.c Fri Sep 09 16:57:55 2011 +1000
+++ b/src/gui.c Fri Sep 09 17:31:36 2011 +1000
@@ -43,6 +43,8 @@
static void gui_do_fork __ARGS((void));
#endif
+static void gui_attempt_start __ARGS((void));
+
static int can_update_cursor = TRUE; /* can display the cursor */
/*
@@ -72,34 +74,18 @@
old_term = vim_strsave(T_NAME);
- /*
- * Set_termname() will call gui_init() to start the GUI.
- * Set the "starting" flag, to indicate that the GUI will start.
- *
- * We don't want to open the GUI shell until after we've read .gvimrc,
- * otherwise we don't know what font we will use, and hence we don't know
- * what size the shell should be. So if there are errors in the .gvimrc
- * file, they will have to go to the terminal: Set full_screen to FALSE.
- * full_screen will be set to TRUE again by a successful termcapinit().
- */
settmode(TMODE_COOK); /* stop RAW mode */
if (full_screen)
cursor_on(); /* needed for ":gui" in .vimrc */
- gui.starting = TRUE;
full_screen = FALSE;
-#ifdef FEAT_GUI_GTK
- gui.event_time = GDK_CURRENT_TIME;
-#endif
-
#ifdef MAY_FORK
if (!gui.dofork || vim_strchr(p_go, GO_FORG) || recursive)
dofork = FALSE;
#endif
++recursive;
- termcapinit((char_u *)"builtin_gui");
- gui.starting = recursive - 1;
+ gui_attempt_start();
if (!gui.in_use) /* failed to start GUI */
{
@@ -112,21 +98,6 @@
vim_free(old_term);
-#if defined(FEAT_GUI_GTK) || defined(FEAT_GUI_X11)
- if (gui.in_use)
- {
-# ifdef FEAT_EVAL
- Window x11_window;
- Display *x11_display;
-
- if (gui_get_x11_windis(&x11_window, &x11_display) == OK)
- set_vim_var_nr(VV_WINDOWID, (long)x11_window);
-# endif
-
- /* Display error messages in a dialog now. */
- display_errors();
- }
-#endif
#if defined(MAY_FORK)
/*
@@ -148,7 +119,48 @@
apply_autocmds(gui.in_use ? EVENT_GUIENTER : EVENT_GUIFAILED,
NULL, NULL, FALSE, curbuf);
#endif
-
+ --recursive;
+}
+
+/*
+ * Set_termname() will call gui_init() to start the GUI.
+ * Set the "starting" flag, to indicate that the GUI will start.
+ *
+ * We don't want to open the GUI shell until after we've read .gvimrc,
+ * otherwise we don't know what font we will use, and hence we don't know
+ * what size the shell should be. So if there are errors in the .gvimrc
+ * file, they will have to go to the terminal: Set full_screen to FALSE.
+ * full_screen will be set to TRUE again by a successful termcapinit().
+ */
+static void gui_attempt_start()
+{
+ static int recursive = 0;
+
+ ++recursive;
+ gui.starting = TRUE;
+
+#ifdef FEAT_GUI_GTK
+ gui.event_time = GDK_CURRENT_TIME;
+#endif
+
+ termcapinit((char_u *)"builtin_gui");
+ gui.starting = recursive - 1;
+
+#if defined(FEAT_GUI_GTK) || defined(FEAT_GUI_X11)
+ if (gui.in_use)
+ {
+# ifdef FEAT_EVAL
+ Window x11_window;
+ Display *x11_display;
+
+ if (gui_get_x11_windis(&x11_window, &x11_display) == OK)
+ set_vim_var_nr(VV_WINDOWID, (long)x11_window);
+# endif
+
+ /* Display error messages in a dialog now. */
+ display_errors();
+ }
+#endif
--recursive;
}
# HG changeset patch
# User tstarl...@wikimedia.org
# Date 1315739148 -36000
# Node ID ca1d2f48fbd3d46bc0575d9621ede6627c82db5f
# Parent 59a60c369292cf19f8635ab721c520bb9221ceac
* Do the fork before starting the GUI, to avoid breaking GTK+. If the child fails to start the GUI, communicate the problem to the parent via a pipe, so that the parent can continue in terminal mode.
* Modified gui_do_fork() to actually start the GUI as well as doing the fork. Documented gui_do_fork().
* Added "#ifdef MAY_FORK" around fork-related function definitions
diff -r 59a60c369292 -r ca1d2f48fbd3 src/gui.c
--- a/src/gui.c Fri Sep 09 17:31:36 2011 +1000
+++ b/src/gui.c Sun Sep 11 21:05:48 2011 +1000
@@ -41,7 +41,17 @@
&& !defined(__APPLE__)
# define MAY_FORK
static void gui_do_fork __ARGS((void));
-#endif
+
+static int gui_read_child_pipe __ARGS((int fd));
+
+/* Return values for gui_read_child_pipe */
+enum {
+ GUI_CHILD_IO_ERROR,
+ GUI_CHILD_OK,
+ GUI_CHILD_FAILED
+};
+
+#endif /* MAY_FORK */
static void gui_attempt_start __ARGS((void));
@@ -67,9 +77,6 @@
gui_start()
{
char_u *old_term;
-#ifdef MAY_FORK
- int dofork = TRUE;
-#endif
static int recursive = 0;
old_term = vim_strsave(T_NAME);
@@ -79,13 +86,24 @@
cursor_on(); /* needed for ":gui" in .vimrc */
full_screen = FALSE;
+ ++recursive;
+
#ifdef MAY_FORK
- if (!gui.dofork || vim_strchr(p_go, GO_FORG) || recursive)
- dofork = FALSE;
-#endif
- ++recursive;
-
- gui_attempt_start();
+ /*
+ * Quit the current process and continue in the child.
+ * Makes "gvim file" disconnect from the shell it was started in.
+ * Don't do this when Vim was started with "-f" or the 'f' flag is present
+ * in 'guioptions'.
+ */
+ if (gui.dofork && !vim_strchr(p_go, GO_FORG) && recursive <= 1)
+ {
+ gui_do_fork();
+ }
+ else
+#endif
+ {
+ gui_attempt_start();
+ }
if (!gui.in_use) /* failed to start GUI */
{
@@ -98,20 +116,6 @@
vim_free(old_term);
-
-#if defined(MAY_FORK)
- /*
- * Quit the current process and continue in the child.
- * Makes "gvim file" disconnect from the shell it was started in.
- * Don't do this when Vim was started with "-f" or the 'f' flag is present
- * in 'guioptions'.
- */
- if (gui.in_use && dofork)
- {
- gui_do_fork();
- }
-#endif
-
#ifdef FEAT_AUTOCMD
/* If the GUI started successfully, trigger the GUIEnter event, otherwise
* the GUIFailed event. */
@@ -132,7 +136,8 @@
* file, they will have to go to the terminal: Set full_screen to FALSE.
* full_screen will be set to TRUE again by a successful termcapinit().
*/
-static void gui_attempt_start()
+ static void
+gui_attempt_start()
{
static int recursive = 0;
@@ -164,38 +169,72 @@
--recursive;
}
-static void gui_do_fork()
+#ifdef MAY_FORK
+
+/*
+ * Create a new process, by forking. In the child, start the GUI, and in
+ * the parent, exit.
+ *
+ * If something goes wrong, this will return with gui.in_use still set
+ * to FALSE, in which case the caller should continue execution without
+ * the GUI.
+ *
+ * If the child fails to start the GUI, then the child will exit and the
+ * parent will return. If the child succeeds, then the parent will exit
+ * and the child will return.
+ */
+ static void
+gui_do_fork()
{
#ifdef __QNXNTO__
procmgr_daemon(0, PROCMGR_DAEMON_KEEPUMASK | PROCMGR_DAEMON_NOCHDIR |
PROCMGR_DAEMON_NOCLOSE | PROCMGR_DAEMON_NODEVNULL);
+ gui_attempt_start();
return;
#else
int pipefd[2]; /* pipe between parent and child */
int pipe_error;
- char dummy;
pid_t pid = -1;
+ FILE *parent_file;
/* Setup a pipe between the child and the parent, so that the parent
* knows when the child has done the setsid() call and is allowed to
* exit. */
pipe_error = (pipe(pipefd) < 0);
pid = fork();
- if (pid > 0) /* Parent */
+ if (pid < 0) /* Fork error */
+ {
+ EMSG(_("E851: Failed to create a new process for the GUI"));
+ return;
+ }
+ else if (pid > 0) /* Parent */
{
/* Give the child some time to do the setsid(), otherwise the
* exit() may kill the child too (when starting gvim from inside a
* gvim). */
- if (pipe_error)
- ui_delay(300L, TRUE);
- else
+ if (!pipe_error)
{
/* The read returns when the child closes the pipe (or when
* the child dies for some reason). */
close(pipefd[1]);
- ignored = (int)read(pipefd[0], &dummy, (size_t)1);
- close(pipefd[0]);
+ int status = gui_read_child_pipe(pipefd[0]);
+ if (status == GUI_CHILD_FAILED)
+ {
+ /* The child failed to start the GUI, so the caller must
+ * continue. There may be more error information written
+ * to stderr by the child. */
+ EMSG(_("E852: The child process failed to start the GUI"));
+ return;
+ }
+ else if (status == GUI_CHILD_IO_ERROR)
+ {
+ pipe_error = TRUE;
+ }
+ /* else GUI_CHILD_OK: parent exit */
}
+
+ if (pipe_error)
+ ui_delay(300L, TRUE);
/* When swapping screens we may need to go to the next line, e.g.,
* after a hit-enter prompt and using ":gui". */
@@ -208,33 +247,83 @@
*/
_exit(0);
}
+ /* Child */
# if defined(HAVE_SETSID) || defined(HAVE_SETPGID)
/*
* Change our process group. On some systems/shells a CTRL-C in the
* shell where Vim was started would otherwise kill gvim!
*/
- if (pid == 0) /* child */
# if defined(HAVE_SETSID)
- (void)setsid();
+ (void)setsid();
# else
- (void)setpgid(0, 0);
+ (void)setpgid(0, 0);
# endif
# endif
if (!pipe_error)
{
close(pipefd[0]);
- close(pipefd[1]);
}
# if defined(FEAT_GUI_GNOME) && defined(FEAT_SESSION)
/* Tell the session manager our new PID */
gui_mch_forked();
# endif
+
+ if (!pipe_error) {
+ parent_file = fdopen(pipefd[1], "w");
+ } else {
+ parent_file = NULL;
+ }
+
+ /* Try to start the GUI */
+ gui_attempt_start();
+
+ /* Notify the parent */
+ if (parent_file)
+ {
+ fputs(gui.in_use ? "ok" : "fail", parent_file);
+ fclose(parent_file);
+ }
+
+ /* If we failed to start the GUI, exit now. */
+ if (!gui.in_use) {
+ exit(1);
+ }
#endif
}
/*
+ * Read from a pipe assumed to be connected to the child process (this
+ * function is called from the parent. Return GUI_CHILD_OK if the child
+ * successfully started the GUI, GUY_CHILD_FAILED if the child failed, or
+ * GUI_CHILD_IO_ERROR if there was some other error.
+ *
+ * The file descriptor will be closed before the function returns.
+ */
+ static int
+gui_read_child_pipe(int fd)
+{
+ size_t bytes_read;
+ FILE *file;
+ char buffer[10];
+
+ file = fdopen(fd, "r");
+ if (!file)
+ return GUI_CHILD_IO_ERROR;
+
+ bytes_read = fread(buffer, sizeof(char), sizeof(buffer)-1, file);
+ buffer[bytes_read] = '\0';
+ fclose(file);
+ if (strcmp(buffer, "ok") == 0)
+ return GUI_CHILD_OK;
+ else
+ return GUI_CHILD_FAILED;
+}
+
+#endif /* MAY_FORK */
+
+/*
* Call this when vim starts up, whether or not the GUI is started
*/
void
# HG changeset patch
# User tstarl...@wikimedia.org
# Date 1315748882 -36000
# Node ID 1641001bc97c51db1c6bbe34e7dcda77a114a1ce
# Parent ca1d2f48fbd3d46bc0575d9621ede6627c82db5f
* Wait for failed child, don't leave zombies.
* Add caveat discovered during testing.
diff -r ca1d2f48fbd3 -r 1641001bc97c src/gui.c
--- a/src/gui.c Sun Sep 11 21:05:48 2011 +1000
+++ b/src/gui.c Sun Sep 11 23:48:02 2011 +1000
@@ -107,7 +107,17 @@
if (!gui.in_use) /* failed to start GUI */
{
- termcapinit(old_term); /* back to old term settings */
+ /* Back to old term settings
+ *
+ * FIXME: If we got here because a child process failed and flagged to
+ * the parent to resume, and X11 is enabled with FEAT_TITLE, this will
+ * hit an X11 I/O error and do a longjmp(), leaving recursive
+ * permanently set to 1. This is probably not as big a problem as it
+ * sounds, because gui_mch_init() in both gui_x11.c and gui_gtk_x11.c
+ * return "OK" unconditionally, so it would be very difficult to
+ * actually hit this case.
+ */
+ termcapinit(old_term);
settmode(TMODE_RAW); /* restart RAW mode */
#ifdef FEAT_TITLE
set_title_defaults(); /* set 'title' and 'icon' again */
@@ -194,6 +204,7 @@
#else
int pipefd[2]; /* pipe between parent and child */
int pipe_error;
+ int exit_status;
pid_t pid = -1;
FILE *parent_file;
@@ -223,6 +234,7 @@
/* The child failed to start the GUI, so the caller must
* continue. There may be more error information written
* to stderr by the child. */
+ waitpid(pid, &exit_status, 0);
EMSG(_("E852: The child process failed to start the GUI"));
return;
}
