Current problems with git master

[gmane@xxxxxxxxxxxxxx (Colin Guthrie)]

'Twas brillig, and Tanu Kaskinen at 25/03/11 05:38 did gyre and gimble:
> On Thu, 2011-03-24 at 21:30 +0000, Colin Guthrie wrote:
>> If someone could double check, I'd appreciate it (seeing as I'd
>> rather any bugs in my commit last less than a year and a half!!)
> 
> Problems found:
> 
> The first process: daemon_pipe is not closed if the first fork()
> call fails. Even if it doesn't fail, the first process never closes 
> daemon_pipe[0].
> 
> The second process: daemon_pipe[1] is not closed if anything fails 
> between the first and the second fork() call.

Ahh yes. A simple call to:
        pa_close_pipe(daemon_pipe);
inside finish should deal with these issues and should be a noop if it's
already closed properly (which should be the case if all goes well). And
an extra call if to pa_close_pipe(daemon_pipe) if the fork fails ensures
nothing is written to the pipe when no-one is listening anyway.


> Also, if the second fork fails, then the finish section writes to
> daemon_pipe2[1], even though only the third process should do that.
> Also, if anything fails between the first and the second fork, then
> the second process never writes anything to daemon_pipe[1]. I don't
> know what happens in the first process in this case - does it get an
> error or does pa_loop_read() get stuck.

Fair point.

So if we get to finish and daemon_pipe[1] is still valid, then this
means that we must have failed at some point after the first fork but
before the second. Therefore only write to it.

If it's not still valid but daemon_pipe2[1] is valid, then it's failed
at some point after second fork so we write to it, which will ultimately
write to the first one too.

This is getting like Inception :p

> The third process: No problems :)

But time passes much more slowly in the third process... Thankfully I've
got a spinning top so I know it's not real....


This patch should fix up these issues right?

diff --git a/src/daemon/main.c b/src/daemon/main.c
index b77a8b6..f939313 100644
--- a/src/daemon/main.c
+++ b/src/daemon/main.c
@@ -729,6 +729,7 @@ int main(int argc, char *argv[]) {

         if ((child = fork()) < 0) {
             pa_log(_("fork() failed: %s"), pa_cstrerror(errno));
+            pa_close_pipe(daemon_pipe);
             goto finish;
         }

@@ -793,6 +794,7 @@ int main(int argc, char *argv[]) {

         if ((child = fork()) < 0) {
             pa_log(_("fork() failed: %s"), pa_cstrerror(errno));
+            pa_close_pipe(daemon_pipe2);
             goto finish;
         }

@@ -1128,10 +1130,15 @@ finish:
     pa_signal_done();

 #ifdef HAVE_FORK
-    if (daemon_pipe2[1] >= 0)
+    /* If we have daemon_pipe[1] still open, this means we've failed after
+     * the first fork, but before the second. Therefore just write to
it. */
+    if (daemon_pipe[1] >= 0)
+        pa_loop_write(daemon_pipe[1], &retval, sizeof(retval), NULL);
+    else if (daemon_pipe2[1] >= 0)
         pa_loop_write(daemon_pipe2[1], &retval, sizeof(retval), NULL);

     pa_close_pipe(daemon_pipe2);
+    pa_close_pipe(daemon_pipe);
 #endif

     if (mainloop)







-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]