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

[Laine Stump]

On 02/18/2010 07:50 AM, Daniel P. Berrange wrote:
> 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.
>    

Yow! That actually seems potentially catastrophic to me - if fork() ever 
fails, libvirtd would ignore all signals until it was kill -9'd.

My only defense is that I was replicating existing behavior ;-)

Fixed patch coming up.