setsid() exception in xend

[Re: forking / setsid'ing patch]

Quoted 1 line · Horms

[Re: forking / setsid'ing patch]

Hi Horms,

How are you running Xend? There's a call to fork in fork_pid, and no-one's had a problem with this or setsid before.

Quoted 8 lines · Ewan Mellor

Quoted 1 line · Horms

[Re: forking / setsid'ing patch]

Hi Horms,

How are you running Xend? There's a call to fork in fork_pid, and no-one's had a problem with this or setsid before.

Hi Ewan,

at the time that I noticed the problem, my machine was doing some very strange things that I won't go into. I can't actually reproduce the exception now that I fixed the box up.

However, I do think that there is a minor problem. My previous patch seemed to solve it, but I think it was slightly wrong, as you point out by the time daemonize() is called, the code is already running as a child.

The thing that I think is missing is that after calling setsid(), fork() needs to be called again. This allows the session leader (the process that called setsid()) to exit, and this its children have no way of regaining the terminal.

Here is a slightly revised patch that puts a second fork() after setsid() (rather than before as the previous incarnation did). If you look at the output of ps you should with and without this patch, and see that the assosiation with the terminal disapears.

As for why the previous patch worked, I'm not entirely sure.

# HG changeset patch
# User Horms <[email protected]>
# Node ID 86339c0ea955b837c3185d500d4ebbb3a5f448c3 # Parent ddd958718cde22f20371a58359e173fd21c5da2e [xend] Detach from terminal

* For setsid to effectivley detach a process from the terminal, the following needs to occur:

fork () Return control to the shell
setsid () New session with no controlling terminal
fork () The session leader (parent) exits and thus
the resulting child process can never regain the terminal

This patch adds the second fork

* The call to self.daemonize(), which now forks, is moved to run before self.tracing(), as not that it actually disconnects from the terminal, and thus the prevailing process, the trace looses the processes created in self.run().

diff -r ddd958718cde -r 86339c0ea955 tools/python/xen/xend/server/SrvDaemon.py

tools/python/xen/xend/server/SrvDaemon.py +30 -4

@@ -121,10 +121,34 @@
 
         return self.child
 
-    def daemonize(self):
+    def daemonize(self, status):
         if not XEND_DAEMONIZE: return
+ 
         # Detach from TTY.
+
+	# Become the group leader (already a child process)
         os.setsid()
+ 
+        # Fork, this allows the group leader to exit,
+	# which means the child can never again regain control of the
+	# terminal
+        r, w = os.pipe()
+        if self.fork_pid(XEND_PID_FILE):
+            os.close(w)
+            r = os.fdopen(r, 'r')
+            try:
+                s = r.read()
+            finally:
+                r.close()
+                if len(s):
+                    status.write(s)
+                    status.close()
+                self.exit()
+ 
+        # Child
+        os.close(r);
+        status.close();
+        status = os.fdopen(w, 'w')
 
         # Detach from standard file descriptors, and redirect them to
         # /dev/null or the log as appropriate.

@@ -140,6 +164,8 @@
             os.dup(0)
             os.open(XEND_DEBUG_LOG, os.O_WRONLY|os.O_CREAT)
 
+	return status
+
 
     def start(self, trace=0):
         """Attempts to start the daemons.

@@ -164,7 +190,7 @@
         # we can avoid a race condition during startup
 
         r,w = os.pipe()
-        if self.fork_pid(XEND_PID_FILE):
+        if os.fork():
             os.close(w)
             r = os.fdopen(r, 'r')
             try:

@@ -178,8 +204,9 @@
         else:
             os.close(r)
             # Child
+            status = self.daemonize(os.fdopen(w, 'w'))
             self.tracing(trace)
-            self.run(os.fdopen(w, 'w'))
+            self.run(status)
 
         return ret
 

@@ -274,7 +301,6 @@
 
             relocate.listenRelocation()
             servers = SrvServer.create()
-            self.daemonize()
             servers.start(status)
         except Exception, ex:
             print >>sys.stderr, 'Exception starting xend:', ex

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 26 lines · Horms

Quoted 8 lines · Ewan Mellor

Quoted 1 line · Horms

[Re: forking / setsid'ing patch]

Hi Horms,

How are you running Xend? There's a call to fork in fork_pid, and no-one's had a problem with this or setsid before.

Hi Ewan,

at the time that I noticed the problem, my machine was doing some very strange things that I won't go into. I can't actually reproduce the exception now that I fixed the box up.

However, I do think that there is a minor problem. My previous patch seemed to solve it, but I think it was slightly wrong, as you point out by the time daemonize() is called, the code is already running as a child.

The thing that I think is missing is that after calling setsid(), fork() needs to be called again. This allows the session leader (the process that called setsid()) to exit, and this its children have no way of regaining the terminal.

Quoted 4 lines

Here is a slightly revised patch that puts a second fork() after setsid() (rather than before as the previous incarnation did). If you look at the output of ps you should with and without this patch, and see that the assosiation with the terminal disapears.

I agree that the second fork is required, so you're on the right track, but the rest of the patch seems problematic to me. Do we really need to have the grandchild write to the child, just so that the child can write to the parent (hunk 1 of your patch)? Why not just pass the descriptor from grandparent to grandchild directly? I think that this would mean that the daemonize process could keep it's original interface, and just perform the extra fork, with no other changes.

Even if the intermediate pipe is required, closing one descriptor called "status" and then opening a new one and assigning that to status just for it to be returned from the function is unreasonably complicated.

[Snip!]

Sure, I can eliminate the intermediate file descriptor, I'll send a fresh patch tomorrow.

On a related issue, can you clarify what the race is that it is there to avoid? It seems cumbersome as you point out.

Quoted 4 lines · Horms

[Snip!]

Sure, I can eliminate the intermediate file descriptor, I'll send a fresh patch tomorrow.

That's great, thank you.

Quoted 2 lines

On a related issue, can you clarify what the race is that it is there to avoid? It seems cumbersome as you point out.

It means that the xend wrapper, i.e. tools/misc/xend aka /usr/sbin/xend does not exit until the daemon is ready to receive commands. If you were using the init.d scripts to start Xend and then start some guest domains, you would have a race in that case between daemon startup and the subsequent script that would be trying to execute commands at the same time.

Quoted 4 lines · Horms

[Snip!]

Sure, I can eliminate the intermediate file descriptor, I'll send a fresh patch tomorrow.

That's great, thank you.

Quoted 2 lines

On a related issue, can you clarify what the race is that it is there to avoid? It seems cumbersome as you point out.

It means that the xend wrapper, i.e. tools/misc/xend aka /usr/sbin/xend does not exit until the daemon is ready to receive commands. If you were using the init.d scripts to start Xend and then start some guest domains, you would have a race in that case between daemon startup and the subsequent script that would be trying to execute commands at the same time.