[libvirt] [PATCH] Avoid deadlock on logging mutex due to fork in virFileCreate/virDirCreate.
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'"), -- 1.6.6
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. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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.
participants (2)
-
Daniel Veillard -
Laine Stump