[PATCH] setsid() exception in xend

[PATCH] setsid() exception in xend

[Horms]

# HG changeset patch
# User Horms <horms@verge.net.au>
# Node ID 37d3e34dfdac009eac2bb040ff79ae711b2d50f9
# Parent  b9181b1c576fb39bb4d3b088ac3378d77163b4cc
For some reason that I can't quite put a finger on,
xend is a session leader well before the code in
SrvDaemon is called. This causes os.setsid() to
throw an exception.

This patch should be safe because the only exception
setsid should throw is if the current process is already
a session leader. So the process will either become 
a session leader, or will already be one.

Signed-Off-By: Horms <horms@verge.net.au>

diff -r b9181b1c576f -r 37d3e34dfdac tools/python/xen/xend/server/SrvDaemon.py
--- a/tools/python/xen/xend/server/SrvDaemon.py	Sat Nov 26 01:21:55 2005
+++ b/tools/python/xen/xend/server/SrvDaemon.py	Sat Nov 26 11:37:18 2005
@@ -123,8 +123,12 @@
 
     def daemonize(self):
         if not XEND_DAEMONIZE: return
-        # Detach from TTY.
-        os.setsid()
+	# Detach from TTY
+	try:
+        	os.setsid()
+	except:
+		# Already a session leader
+		pass
 
         # Detach from standard file descriptors, and redirect them to
         # /dev/null or the log as appropriate.

Re: [PATCH] setsid() exception in xend

[Horms]

The previous patch was bogus, please ignore it.
This should be a bit better:

# HG changeset patch
# User Horms <horms@verge.net.au>
# Node ID 81daa5bcf2ee0a463755e7662606bb6650038f69
# Parent  ed749e5935bd8dc283852f4064569415118deeaf
[xend] Detach from terminal

* For setsid to effectivley detach a process from the terminal,
  the process must have forked.

  Setsid is run in the child process. 
  
  The parent process manages the status fd, as per its behaviour in 
  self.start(), and exits when the fd handling is complete.

  The output of ps axf verifies that xend and its subsequenbtly
  created child processes are detached from the terminal.

* 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 ed749e5935bd -r 81daa5bcf2ee tools/python/xen/xend/server/SrvDaemon.py
--- a/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 04:06:14 2005
+++ b/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 04:16:03 2005
@@ -121,8 +121,28 @@
 
         return self.child
 
-    def daemonize(self):
+    def daemonize(self, status):
         if not XEND_DAEMONIZE: return
+ 
+        # Fork to allow disconnection from TTY
+        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 TTY.
         os.setsid()
 
@@ -140,6 +160,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 +186,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 +200,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 +297,6 @@
 
             relocate.listenRelocation()
             servers = SrvServer.create()
-            self.daemonize()
             servers.start(status)
         except Exception, ex:
             print >>sys.stderr, 'Exception starting xend:', ex

Re: Re: [PATCH] setsid() exception in xend

[Ewan Mellor]

On Mon, Nov 28, 2005 at 04:29:04AM +0000, Horms wrote:

> [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.

Ewan.

Re: [PATCH] setsid() exception in xend

[Horms]

Ewan Mellor <ewan@xensource.com> wrote:
> On Mon, Nov 28, 2005 at 04:29:04AM +0000, Horms wrote:
> 
>> [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 <horms@verge.net.au>
# 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
--- a/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 12:35:11 2005
+++ b/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 12:45:57 2005
@@ -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

Re: Re: [PATCH] setsid() exception in xend

[Ewan Mellor]

On Mon, Nov 28, 2005 at 12:53:17PM +0000, Horms wrote:

> Ewan Mellor <ewan@xensource.com> wrote:
> > On Mon, Nov 28, 2005 at 04:29:04AM +0000, Horms wrote:
> > 
> >> [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.

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.

Ewan.

> 
> As for why the previous patch worked, I'm not entirely sure.
> 
> # HG changeset patch
> # User Horms <horms@verge.net.au>
> # 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
> --- a/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 12:35:11 2005
> +++ b/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 12:45:57 2005
> @@ -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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: Re: [PATCH] setsid() exception in xend

[Horms]

On Mon, Nov 28, 2005 at 01:37:13PM +0000, Ewan Mellor wrote:
> On Mon, Nov 28, 2005 at 12:53:17PM +0000, Horms wrote:
> 
> > Ewan Mellor <ewan@xensource.com> wrote:
> > > On Mon, Nov 28, 2005 at 04:29:04AM +0000, Horms wrote:
> > > 
> > >> [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.
> 
> 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.

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.

-- 
Horms

Re: Re: [PATCH] setsid() exception in xend

[Ewan Mellor]

On Mon, Nov 28, 2005 at 10:58:09PM +0900, Horms wrote:

> [Snip!]
>
> Sure, I can eliminate the intermediate file descriptor,
> I'll send a fresh patch tomorrow. 

That's great, thank you.

> 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.

Ewan.

Re: Re: [PATCH] setsid() exception in xend

[Horms]

On Mon, Nov 28, 2005 at 03:57:01PM +0000, Ewan Mellor wrote:
> On Mon, Nov 28, 2005 at 10:58:09PM +0900, Horms wrote:
> 
> > [Snip!]
> >
> > Sure, I can eliminate the intermediate file descriptor,
> > I'll send a fresh patch tomorrow. 
> 
> That's great, thank you.
> 
> > 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.

Thanks for the clarification.

Below is a patch that removes the unecessary fd handling in
the intermediate parent that was present in my previous patch.
It just calls self.exit(). I did consider doing some fd closing,
but exit() shold be sufficient. I tested this patch and it seems
to work just fine.


# HG changeset patch
# User Horms <horms@verge.net.au>
# Node ID 4c45fd3fd1e0916a34b614c95ca28b7d44cd8e35
# 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 4c45fd3fd1e0 tools/python/xen/xend/server/SrvDaemon.py
--- a/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 12:35:11 2005
+++ b/tools/python/xen/xend/server/SrvDaemon.py	Tue Nov 29 01:48:14 2005
@@ -123,9 +123,18 @@
 
     def daemonize(self):
         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
+        if self.fork_pid(XEND_PID_FILE):
+            self.exit()
+ 
         # Detach from standard file descriptors, and redirect them to
         # /dev/null or the log as appropriate.
         os.close(0)
@@ -164,7 +173,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,6 +187,7 @@
         else:
             os.close(r)
             # Child
+            self.daemonize()
             self.tracing(trace)
             self.run(os.fdopen(w, 'w'))
 
@@ -274,7 +284,6 @@
 
             relocate.listenRelocation()
             servers = SrvServer.create()
-            self.daemonize()
             servers.start(status)
         except Exception, ex:
             print >>sys.stderr, 'Exception starting xend:', ex

Re: [PATCH] setsid() exception in xend

[Horms]

Sorry if this is a duplicate, I am sure I sent it but
it doesn't seem to have shown up on the list.

On Mon, Nov 28, 2005 at 03:57:01PM +0000, Ewan Mellor wrote:
> On Mon, Nov 28, 2005 at 10:58:09PM +0900, Horms wrote:
> 
> > [Snip!]
> >
> > Sure, I can eliminate the intermediate file descriptor,
> > I'll send a fresh patch tomorrow. 
> 
> That's great, thank you.
> 
> > 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.

Thanks for the clarification.

Below is a patch that removes the unecessary fd handling in
the intermediate parent that was present in my previous patch.
It just calls self.exit(). I did consider doing some fd closing,
but exit() shold be sufficient. I tested this patch and it seems
to work just fine.


# HG changeset patch
# User Horms <horms@verge.net.au>
# Node ID 4c45fd3fd1e0916a34b614c95ca28b7d44cd8e35
# 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 4c45fd3fd1e0 tools/python/xen/xend/server/SrvDaemon.py
--- a/tools/python/xen/xend/server/SrvDaemon.py	Mon Nov 28 12:35:11 2005
+++ b/tools/python/xen/xend/server/SrvDaemon.py	Tue Nov 29 01:48:14 2005
@@ -123,9 +123,18 @@
 
     def daemonize(self):
         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
+        if self.fork_pid(XEND_PID_FILE):
+            self.exit()
+ 
         # Detach from standard file descriptors, and redirect them to
         # /dev/null or the log as appropriate.
         os.close(0)
@@ -164,7 +173,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,6 +187,7 @@
         else:
             os.close(r)
             # Child
+            self.daemonize()
             self.tracing(trace)
             self.run(os.fdopen(w, 'w'))
 
@@ -274,7 +284,6 @@
 
             relocate.listenRelocation()
             servers = SrvServer.create()
-            self.daemonize()
             servers.start(status)
         except Exception, ex:
             print >>sys.stderr, 'Exception starting xend:', ex