some renaming, prettyfying, etc...

src/netns/protocol.py

@@ -71,8 +71,8 @@ class Server(object):
         self.closed = False
         # Print debug info
         self.debug = debug
-        # Dictionary to keep track of started processes
-        self._children = dict()
+        # Set to keep track of started processes
+        self._children = set()
         # Buffer and flag for PROC mode
         self._proc = None
 
@@ -246,8 +246,8 @@ class Server(object):
         self.reply(221, "Sayounara.");
         self.closed = True
 
-    def do_PROC_CRTE(self, cmdname, user, file, *argv):
-        self._proc = { 'user': user, 'file': file, 'argv': argv }
+    def do_PROC_CRTE(self, cmdname, user, executable, *argv):
+        self._proc = { 'user': user, 'executable': executable, 'argv': argv }
         self.commands = _proc_commands
         self.reply(200, "Entering PROC mode.")
 
@@ -295,7 +295,7 @@ class Server(object):
 
     def do_PROC_RUN(self, cmdname):
         try:
-            chld = netns.subprocess.Popen(**self._proc)
+            chld = netns.subprocess.spawn(**self._proc)
         except BaseException, e: # FIXME
             self.reply(500, "Failure starting process: %s" % str(e))
             self._proc = None
@@ -317,12 +317,12 @@ class Server(object):
             self.reply(500, "Process does not exist.")
             return
         if cmdname == 'PROC POLL':
-            ret = self._children[pid].poll()
+            ret = netns.subprocess.poll(pid)
         else:
-            ret = self._children[pid].wait()
+            ret = netns.subprocess.wait(pid)
 
         if ret != None:
-            del self._children[pid]
+            self._children.remove(pid)
             self.reply(200, "%d exitcode." % ret)
         else:
             self.reply(450, "Not finished yet.")
@@ -330,14 +330,14 @@ class Server(object):
     # Same code for the two commands
     do_PROC_WAIT = do_PROC_POLL
 
-    def do_PROC_KILL(self, cmdname, pid, signal):
+    def do_PROC_KILL(self, cmdname, pid, sig):
         if pid not in self._children:
             self.reply(500, "Process does not exist.")
             return
         if signal:
-            self._children[pid].kill(signal)
+            os.kill(pid, sig)
         else:
-            self._children[pid].kill()
+            os.kill(pid, signal.SIGTERM)
         self.reply(200, "Process signalled.")
 
 #    def do_IF_LIST(self, cmdname, ifnr = None):
@@ -416,14 +416,15 @@ class Client(object):
         passfd.sendfd(self._fd, fd, "PROC " + type)
         self._read_and_check_reply()
 
-    def popen(self, user, file, argv = None, cwd = None, env = None,
+    def spawn(self, user, executable, argv = None, cwd = None, env = None,
             stdin = None, stdout = None, stderr = None):
         """Start a subprocess in the slave; the interface resembles
         subprocess.Popen, but with less functionality. In particular
-        stdin/stdout/stderr can only be None or a open file descriptor."""
+        stdin/stdout/stderr can only be None or a open file descriptor.
+        See netns.subprocess.spawn for details."""
 
         params = ["PROC", "CRTE", base64.b64encode(user),
-                base64.b64encode(file)]
+                base64.b64encode(executable)]
         if argv != None:
             for i in argv:
                 params.append(base64.b64encode(i))

src/netns/subprocess.py

@@ -3,125 +3,145 @@
 
 import fcntl, grp, os, pickle, pwd, signal, sys, traceback
 
-class Popen(object):
-    """Class that attempts to provide low-leven popen-like behaviour, with the
-    extra feature of being able to switch user before executing the command."""
-    def __init__(self, user, file, argv, cwd = None, env = None,
-            stdin = None, stdout = None, stderr = None):
-        """Check Python's subprocess.Popen for the intended behaviour. The
-        extra `user` argument, if not None, specifies a username to run the
-        command as, including its primary and secondary groups. If a numerical
-        UID is given, a reverse lookup is performed to find the user name and
-        then set correctly the groups.  Note that `stdin`, `stdout`, and
-        `stderr` can only be integers representing file descriptors, and that
-        they are not closed by this class; piping should be handled
-        externally."""
-
-        userfd = [stdin, stdout, stderr]
-        sysfd = [x.fileno() for x in sys.stdin, sys.stdout, sys.stderr]
-        # Verify there is no clash
-        assert not (set(filter(None, userfd)) & set(filter(None, sysfd)))
-
-        if user != None:
-            if str(user).isdigit():
-                uid = int(user)
-                try:
-                    user = pwd.getpwuid(uid)[0]
-                except:
-                    raise ValueError("UID %d does not exist" % int(user))
-            else:
-                try:
-                    uid = pwd.getpwnam(str(user))[2]
-                except:
-                    raise ValueError("User %s does not exist" % str(user))
-
-            gid = pwd.getpwuid(uid)[3]
-            groups = [x[2] for x in grp.getgrall() if user in x[3]]
-
-        (r, w) = os.pipe()
-        pid = os.fork()
-        if pid == 0:
+def spawn(user, executable, argv, cwd = None, env = None,
+        stdin = None, stdout = None, stderr = None):
+    """Forks and execs a program, with stdio redirection and user switching.
+    The program is specified by `executable', if it does not contain any slash,
+    the PATH environment variable is used to search for the file.
+
+    The `user` parameter, if not None, specifies a user name to run the
+    command as, after setting its primary and secondary groups. If a numerical
+    UID is given, a reverse lookup is performed to find the user name and
+    then set correctly the groups.
+
+    To run the program in a different directory than the current one, it should
+    be set in `cwd'.
+
+    If specified, `env' replaces the caller's environment with the dictionary
+    provided.
+
+    The standard input, output, and error of the created process will be
+    redirected to the file descriptors specified by `stdin`, `stdout`, and
+    `stderr`, respectively. These parameters must be integers or None, in which
+    case, no redirection will occur. If the value is negative, the respective
+    file descriptor is closed in the executed program.
+
+    Note that the original descriptors are not closed, and that piping should
+    be handled externally.
+    
+    Exceptions occurred while trying to set up the environment or executing the
+    program are propagated to the parent."""
+
+    userfd = [stdin, stdout, stderr]
+    filtered_userfd = filter(lambda x: x != None and x >= 0, userfd)
+    sysfd = [x.fileno() for x in sys.stdin, sys.stdout, sys.stderr]
+    # Verify there is no clash
+    assert not (set(sysfd) & set(filtered_userfd))
+
+    if user != None:
+        if str(user).isdigit():
+            uid = int(user)
+            try:
+                user = pwd.getpwuid(uid)[0]
+            except:
+                raise ValueError("UID %d does not exist" % int(user))
+        else:
+            try:
+                uid = pwd.getpwnam(str(user))[2]
+            except:
+                raise ValueError("User %s does not exist" % str(user))
+
+        gid = pwd.getpwuid(uid)[3]
+        groups = [x[2] for x in grp.getgrall() if user in x[3]]
+
+    (r, w) = os.pipe()
+    pid = os.fork()
+    if pid == 0:
+        try:
+            # Set up stdio piping
+            for i in range(3):
+                if userfd[i] != None and userfd[i] >= 0:
+                    os.dup2(userfd[i], sysfd[i])
+                    os.close(userfd[i]) # only in child!
+                if userfd[i] != None and userfd[i] < 0:
+                    os.close(sysfd[i])
+            # Set up special control pipe
+            os.close(r)
+            fcntl.fcntl(w, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
+            if user != None:
+                # Change user
+                os.setgid(gid)
+                os.setgroups(groups)
+                os.setuid(uid)
+            if cwd != None:
+                os.chdir(cwd)
+            if not argv:
+                argv = [ file ]
+            if '/' in file: # Should not search in PATH
+                if env != None:
+                    os.execve(file, argv, env)
+                else:
+                    os.execv(file, argv)
+            else: # use PATH
+                if env != None:
+                    os.execvpe(file, argv, env)
+                else:
+                    os.execvp(file, argv)
+            raise RuntimeError("Unreachable reached!")
+        except:
             try:
-                # Set up stdio piping
-                for i in range(3):
-                    if userfd[i] != None:
-                        os.dup2(userfd[i], sysfd[i])
-                        os.close(userfd[i])
-                # Set up special control pipe
-                os.close(r)
-                fcntl.fcntl(w, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
-                if user != None:
-                    # Change user
-                    os.setgid(gid)
-                    os.setgroups(groups)
-                    os.setuid(uid)
-                if cwd != None:
-                    os.chdir(cwd)
-                if not argv:
-                    argv = [ file ]
-                if '/' in file: # Should not search in PATH
-                    if env != None:
-                        os.execve(file, argv, env)
-                    else:
-                        os.execv(file, argv)
-                else: # use PATH
-                    if env != None:
-                        os.execvpe(file, argv, env)
-                    else:
-                        os.execvp(file, argv)
-                raise RuntimeError("Unreachable reached!")
+                (t, v, tb) = sys.exc_info()
+                # Got the child_traceback attribute trick from Python's
+                # subprocess.py
+                v.child_traceback = "".join(
+                        traceback.format_exception(t, v, tb))
+                os.write(w, pickle.dumps(v))
+                os.close(w)
             except:
-                try:
-                    (t, v, tb) = sys.exc_info()
-                    # Got the child_traceback attribute trick from Python's
-                    # subprocess.py
-                    v.child_traceback = "".join(
-                            traceback.format_exception(t, v, tb))
-                    os.write(w, pickle.dumps(v))
-                    os.close(w)
-                except:
-                    traceback.print_exc()
-                os._exit(1)
-
-        os.close(w)
-
-        # read EOF for success, or a string as error info
-        s = ""
-        while True:
-            s1 = os.read(r, 4096)
-            if s1 == "":
-                break
-            s += s1
-        os.close(r)
-
-        if s == "":
-            self._pid = pid
-            return
-
-        # It was an error
-        os.waitpid(pid, 0)
-        raise pickle.loads(s)
-
-    @property
-    def pid(self):
-        return self._pid
-
-    def poll(self):
-        """Check if the process already died. Returns the exit code or None if
-        the process is still alive."""
-        r = os.waitpid(self._pid, os.WNOHANG)
-        if r[0]:
-            del self._pid
-            return r[1]
-        return None
-
-    def wait(self):
-        """Wait for process to die and return the exit code."""
-        r = os.waitpid(self._pid, 0)[1]
-        del self._pid
-        return r
-
-    def kill(self, sig = signal.SIGTERM):
-        """Kill the process with the specified signal. Note that the process
-        still needs to be waited for to avoid zombies."""
-        os.kill(self._pid, sig)
+                traceback.print_exc()
+            os._exit(1)
+
+    os.close(w)
+
+    # read EOF for success, or a string as error info
+    s = ""
+    while True:
+        s1 = os.read(r, 4096)
+        if s1 == "":
+            break
+        s += s1
+    os.close(r)
+
+    if s == "":
+        return pid
+
+    # It was an error
+    os.waitpid(pid, 0)
+    exc = pickle.loads(s)
+    # XXX: sys.excepthook
+    raise exc
+
+# Used to print extra info in nested exceptions
+def _custom_hook(t, v, tb):
+    sys.stderr.write("wee\n")
+    if hasattr(v, "child_traceback"):
+        sys.stderr.write("Nested exception, original traceback " +
+                "(most recent call last):\n")
+        sys.stderr.write(v.child_traceback + ("-" * 70) + "\n")
+    sys.__excepthook__(t, v, tb)
+
+# XXX: somebody kill me, I deserve it :)
+sys.excepthook = _custom_hook
+
+def poll(pid):
+    """Check if the process already died. Returns the exit code or None if
+    the process is still alive."""
+    r = os.waitpid(pid, os.WNOHANG)
+    if r[0]:
+        return r[1]
+    return None
+
+def wait(pid):
+    """Wait for process to die and return the exit code."""
+    return os.waitpid(pid, 0)[1]
+

t/test_subprocess.py

@@ -56,32 +56,32 @@ class TestSubprocess(unittest.TestCase):
     @test_util.skipUnless(os.getuid() == 0, "Test requires root privileges")
     def test_popen_chuser(self):
         user = 'nobody'
-        p = netns.subprocess.Popen(user, '/bin/sleep', ['/bin/sleep', '1000'])
-        self._check_ownership(user, p.pid)
-        p.kill()
-        self.assertEquals(p.wait(), signal.SIGTERM)
+        pid = netns.subprocess.spawn(user, '/bin/sleep', ['/bin/sleep', '100'])
+        self._check_ownership(user, pid)
+        os.kill(pid, signal.SIGTERM)
+        self.assertEquals(netns.subprocess.wait(pid), signal.SIGTERM)
 
     def test_popen_basic(self):
         # User does not exist
-        self.assertRaises(ValueError, netns.subprocess.Popen,
+        self.assertRaises(ValueError, netns.subprocess.spawn,
                 self.nouser, '/bin/sleep', ['/bin/sleep', '1000'])
-        self.assertRaises(ValueError, netns.subprocess.Popen,
+        self.assertRaises(ValueError, netns.subprocess.spawn,
                 self.nouid, '/bin/sleep', ['/bin/sleep', '1000'])
         # Invalid CWD: it is a file
-        self.assertRaises(OSError, netns.subprocess.Popen,
+        self.assertRaises(OSError, netns.subprocess.spawn,
                 None, '/bin/sleep', None, cwd = '/bin/sleep')
         # Invalid CWD: does not exist
-        self.assertRaises(OSError, netns.subprocess.Popen,
+        self.assertRaises(OSError, netns.subprocess.spawn,
                 None, '/bin/sleep', None, cwd = self.nofile)
         # Exec failure
-        self.assertRaises(OSError, netns.subprocess.Popen,
+        self.assertRaises(OSError, netns.subprocess.spawn,
                 None, self.nofile, None)
         # Test that the environment is cleared: sleep should not be found
         # XXX: This should be a python bug: if I don't set PATH explicitly, it
         # uses a default search path
-        self.assertRaises(OSError, netns.subprocess.Popen,
+        self.assertRaises(OSError, netns.subprocess.spawn,
                 None, 'sleep', None, env = {'PATH': ''})
-        #p = netns.subprocess.Popen(None, '/bin/sleep', ['/bin/sleep', '1000'],
+        #p = netns.subprocess.spawn(None, '/bin/sleep', ['/bin/sleep', '1000'],
         #        cwd = '/', env = [])
         # FIXME: tests fds