[PATCH 1/2] Remove surprises from X startup.

Vratislav Podzimek vpodzime at redhat.com
Thu Jun 26 05:50:44 UTC 2014


On Wed, 2014-06-25 at 15:07 -0400, David Shea wrote:
> (this version doesn't try to mutate the dict it's iterating)
> 
> As was noted in the comment, assuming that any SIGCHLD means that X
> exited makes the startup process fragile, so stop doing that. Install a
> global SIGCHLD handler to track exits from Xorg and metacity. metacity
> failures will now throw an exception instead of being silently,
> confusingly ignored. Add a fork wrapper to remove the race between
> process startup and process monitoring.
> 
> Removed a comment about the importance of the window manager being the
> first X connection. This is no longer true, which is fortunate as the
> period between metacity starting and metacity connecting to the X server
> presents an intractible race.
> ---
>  anaconda | 212 +++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 165 insertions(+), 47 deletions(-)
> 
> diff --git a/anaconda b/anaconda
> index cbbedd6..9bb270b 100755
> --- a/anaconda
> +++ b/anaconda
> @@ -44,9 +44,77 @@ if ("debug=1" in proc_cmdline) or ("debug" in proc_cmdline):
>      cov.start()
>  
> 
> -import atexit, sys, os, time, subprocess
> -# keep up with process ID of the window manager if we start it
> -wm_pid = None
> +import atexit, sys, os, time, subprocess, signal, errno
> +
> +# Install a global SIGCHLD handler to keep track of things that should be
> +# running for as long as anaconda does. The dictionary is of the form
> +# {pid: name, ...}. The handler will raise OSError, so if not caught
> +# a SIGCHLD from a watched process will halt anaconda.
> +forever_pids = {}
> +
> +def sigchld_handler(num, frame):
> +    # Check whether anything in the list of processes being watched has
> +    # exited. We don't want to call waitpid(-1), since that would break
> +    # anything else using wait/waitpid (like the subprocess module).
> +    exited_pids = []
> +    exn_message = []
> +
> +    for child_pid in forever_pids:
> +        try:
> +            pid_result, status = os.waitpid(child_pid, os.WNOHANG)
> +        except OSError as e:
> +            if e.errno == errno.ECHILD:
> +                continue
> +
> +        if pid_result:
> +            proc_name = forever_pids[child_pid]
> +            exited_pids.append(child_pid)
> +
> +            if os.WIFEXITED(status):
> +                status_str = "with status %s" % os.WEXITSTATUS(status)
> +            elif os.WIFSIGNALED(status):
> +                status_str = "on signal %s" % os.WTERMSIG(status)
> +            else:
> +                status_str = "with unknown status code %s" % status
> +
> +            exn_message.append("%s exited %s" % (proc_name, status_str))
> +
> +    for child_pid in exited_pids:
> +        del forever_pids[child_pid]
> +
> +    if exn_message:
> +        raise OSError(0, ", ".join(exn_message))
> +
> +signal.signal(signal.SIGCHLD, sigchld_handler)
> +
> +# Fork a new process and add it to forever_pids. The return values are the
> +# the same as os.fork, but neither the parent nor the child will return
> +# until the process is watched.
> +def start_watched_pid(proc_name):
> +    readpipe, writepipe = os.pipe()
> +    childpid = os.fork()

> +    if not childpid:
Don't know about the others, but I find 'if childpid != 0' much better
readable and obvious than 'if not childpid'.

Other than that this looks good to me. Nice change!

-- 
Vratislav Podzimek

Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic




More information about the anaconda-patches mailing list