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.