[libvirt] [PATCH] Avoid deadlock on logging mutex due to fork in virFileCreate/virDirCreate.

[Laine Stump]

On 02/03/2010 11:22 AM, Daniel Veillard wrote:
> On Wed, Feb 03, 2010 at 10:27:32AM -0500, Laine Stump wrote:
>    
>> These functions have the same potential problem that Cole found in
>> virExec - if the log mutex is currently held at the time of the fork,
>> the child process' logging mutex will be locked, but nobody in the
>> process to unlock it. If the child process tries to log anything, it
>> will wait forever for this lock. The solution is to have the parent
>> acquire the lock before fork, then both parent and child release the
>> lock right after.
>>
>> Note that this patch requires Cole's patch to be applied first.
>> ---
>>   src/util/util.c |   12 ++++++++++--
>>   1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/util.c b/src/util/util.c
>> index 901c0d2..d8a2394 100644
>> --- a/src/util/util.c
>> +++ b/src/util/util.c
>> @@ -1263,7 +1263,11 @@ int virFileCreate(const char *path, mode_t mode,
>>        * following dance avoids problems caused by root-squashing
>>        * NFS servers. */
>>
>> -    if ((pid = fork())<  0) {
>> +    virLogLock();
>> +    pid = fork();
>> +    virLogUnlock();
>> +
>> +    if (pid<  0) {
>>           ret = errno;
>>           virReportSystemError(NULL, errno,
>>                                _("cannot fork o create file '%s'"), path);
>> @@ -1369,7 +1373,11 @@ int virDirCreate(const char *path, mode_t mode,
>>           return virDirCreateSimple(path, mode, uid, gid, flags);
>>       }
>>
>> -    if ((pid = fork())<  0) {
>> +    virLogLock();
>> +    pid = fork();
>> +    virLogUnlock();
>> +
>> +    if (pid<  0) {
>>           ret = errno;
>>           virReportSystemError(NULL, errno,
>>                                _("cannot fork to create directory '%s'"),
>>      
>    Okay, makes sense, ACK, and applied !
> Maybe at some point a wrapper around fork() may be nicer.
>    

My thoughts exactly! (didn't want to do it right now though - too much 
potential for disaster). This wrapper should also take the signal race 
problem into account.