[libvirt] [PATCH 1/2] Add virFork() function to utils.

[Daniel P. Berrange]

On Wed, Feb 17, 2010 at 02:29:27PM -0500, Laine Stump wrote:
> virFork() contains bookkeeping that must be done any time a process
> forks. Currently this includes:
> 
> 1) Call virLogLock() prior to fork() and virLogUnlock() just after,
>    to avoid a deadlock if some other thread happens to hold that lock
>    during the fork.
> 
> 2) Reset the logging hooks and send all child process log messages to
>    stderr.
> 
> 3) Block all signals prior to fork(), then either a) reset the signal
>    mask for the parent process, or b) clear the signal mask for the
>    child process.
> 
> util.c, util.h: add virFork() function, based on what is currently
>                 done in __virExec().
> ---
>  src/util/util.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/util.h |    1 +
>  2 files changed, 116 insertions(+), 0 deletions(-)
> 
> diff --git a/src/util/util.c b/src/util/util.c
> index cdab300..f508f6b 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -296,6 +296,121 @@ static int virClearCapabilities(void)
>  }
>  #endif
>  
> +
> +/* virFork() - fork a new process while avoiding various race/deadlock conditions
> +
> +   @pid - a pointer to a pid_t that will receive the return value from
> +          fork()
> +
> +   on return from virFork(), if *pid < 0, the fork failed and there is
> +   no new process. Otherwise, just like fork(), if *pid == 0, it is the
> +   child process returning, and if *pid > 0, it is the parent.
> +
> +   Even if *pid >= 0, if the return value from virFork() is < 0, it
> +   indicates a failure that occurred in the parent or child process
> +   after the fork. In this case, the child process should call
> +   _exit(1) after doing any additional error reporting.
> +
> + */
> +int virFork(pid_t *pid) {
> +    sigset_t oldmask, newmask;
> +    struct sigaction sig_action;
> +    int saved_errno, ret = -1;
> +
> +    *pid = -1;
> +
> +    /*
> +     * Need to block signals now, so that child process can safely
> +     * kill off caller's signal handlers without a race.
> +     */
> +    sigfillset(&newmask);
> +    if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) {
> +        saved_errno = errno;
> +        virReportSystemError(errno,
> +                             "%s", _("cannot block signals"));
> +        goto cleanup;
> +    }
> +
> +    /* Ensure we hold the logging lock, to protect child processes
> +     * from deadlocking on another thread's inherited mutex state */
> +    virLogLock();
> +
> +    *pid = fork();
> +    saved_errno = errno; /* save for caller */
> +
> +    /* Unlock for both parent and child process */
> +    virLogUnlock();
> +
> +    if (*pid < 0) {
> +        virReportSystemError(saved_errno,
> +                             "%s", _("cannot fork child process"));
> +        goto cleanup;
> +    }

Tiny bug here, in that we forget to unblock the parent's signals in 
this error path.

> +
> +    if (*pid) {
> +
> +        /* parent process */
> +
> +        /* Restore our original signal mask now that the child is
> +           safely running */
> +        if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) {
> +            saved_errno = errno; /* save for caller */
> +            virReportSystemError(errno, "%s", _("cannot unblock signals"));
> +            goto cleanup;
> +        }
> +        ret = 0;
> +
> +    } else {
> +
> +        /* child process */
> +
> +        int logprio;
> +        int i;
> +
> +        /* Remove any error callback so errors in child now
> +           get sent to stderr where they stand a fighting chance
> +           of being seen / logged */
> +        virSetErrorFunc(NULL, NULL);
> +
> +        /* Make sure any hook logging is sent to stderr, since child
> +         * process may close the logfile FDs */
> +        logprio = virLogGetDefaultPriority();
> +        virLogReset();
> +        virLogSetDefaultPriority(logprio);
> +
> +        /* Clear out all signal handlers from parent so nothing
> +           unexpected can happen in our child once we unblock
> +           signals */
> +        sig_action.sa_handler = SIG_DFL;
> +        sig_action.sa_flags = 0;
> +        sigemptyset(&sig_action.sa_mask);
> +
> +        for (i = 1; i < NSIG; i++) {
> +            /* Only possible errors are EFAULT or EINVAL
> +               The former wont happen, the latter we
> +               expect, so no need to check return value */
> +
> +            sigaction(i, &sig_action, NULL);
> +        }
> +
> +        /* Unmask all signals in child, since we've no idea
> +           what the caller's done with their signal mask
> +           and don't want to propagate that to children */
> +        sigemptyset(&newmask);
> +        if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) {
> +            saved_errno = errno; /* save for caller */
> +            virReportSystemError(errno, "%s", _("cannot unblock signals"));
> +            goto cleanup;
> +        }
> +        ret = 0;
> +    }
> +
> +cleanup:
> +    if (ret < 0)
> +        errno = saved_errno;
> +    return ret;
> +}
> +
>  /*
>   * @argv argv to exec
>   * @envp optional environment to use for exec
> diff --git a/src/util/util.h b/src/util/util.h
> index 4207508..8460100 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -81,6 +81,7 @@ int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK;
>  int virRunWithHook(const char *const*argv,
>                     virExecHook hook, void *data,
>                     int *status) ATTRIBUTE_RETURN_CHECK;
> +int virFork(pid_t *pid);
>  
>  int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
>  


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|