Re: posix_spawn

[Neal H Walfield]

> Now that people are able to build libc, I'd like somebody to try out
> my posix_spawn implementation.  Plop this into sysdeps/mach/hurd/spawni.c,
> rebuild libc, and then try the `tst-spawn' test in posix.  Then you get to
> debug it for me.  As described in the comments, I will probably rewrite
> parts of this to do something more hairy (and more robust).  But I'd like
> to get this code tested and debugged first. 


Roland, it is a pleasure debugging your code: I always learn so much.

Anyway, three small bugs:

 o We need <spin-lock.h> for the declaration of spin_lock_locked.

 o Anything allocated with alloca is freed when the stack frame is
   destroy, i.e. we cannot expand local arrays using a nested
   function; I have changed this to a macro in the included patch.

 o You forgot to clear the corresponding dtable_cell when setting
   dtable to a raw port in the spawn_do_open action.  This was causing
   a core dump in the clean up code.

The code with this patch passes Ulrich's test plus a few personal
additions.


2001-12-16  Neal H Walfield  <neal@cs.uml.edu>

        * sysdeps/mach/hurd/spawni.c: Include <spin-lock.h> for the
        spin_lock_locked declaration.

        (__spawni:expand_dtable): Change this from a function . . .
        (EXPAND_DTABLE): . . . to a macro.
        (GROW_TABLE): New macro.

        (__spawni): When setting an element of DTABLE to a raw port,
        be sure to clear the corresponding element in DTABLE_CELLS.


--- libc/sysdeps/mach/hurd/spawni.c     Sun Dec 16 22:31:41 2001
+++ libc/sysdeps/mach/hurd/spawni.c.new Sun Dec 16 22:27:16 2001
@@ -32,6 +32,7 @@
 #include <hurd/resource.h>
 #include <assert.h>
 #include <argz.h>
+#include <spin-lock.h>
 #include "spawn_int.h"
 
 /* Spawn a new process executing PATH with the attributes describes in *ATTRP.
@@ -386,24 +387,28 @@ __spawni (pid_t *pid, const char *file,
          }
 
        /* Make sure the dtable can hold NEWFD.  */
-       error_t expand_dtable (int newfd)
-         {
-           if ((unsigned int)newfd >= dtablesize
-               && newfd < _hurd_rlimits[RLIMIT_OFILE].rlim_cur)
-             {
-               /* We need to expand the dtable for the child.  */
-#define NEW(x) \
-  do { __typeof (x) new_##x = __alloca ((newfd + 1) * sizeof (x[0]));        \
-  memcpy (new_##x, x, dtablesize * sizeof (x[0]));                           \
-  memset (&new_##x[dtablesize], 0, (newfd + 1 - dtablesize) * sizeof (x[0])); \
-  x = new_##x; } while (0)
-               NEW (dtable);
-               NEW (ulink_dtable);
-               NEW (dtable_cells);
-#undef NEW
-             }
-           return ((unsigned int)newfd < dtablesize ? 0 : EMFILE);
-         }
+#define GROW_TABLE(x, newfd) \
+       do                                                                    \
+         {                                                                   \
+           __typeof (x) new_##x = __alloca ((newfd + 1) * sizeof (x[0]));    \
+           memcpy (new_##x, x, dtablesize * sizeof (x[0]));                  \
+           memset (&new_##x[dtablesize], 0,                                  \
+                   (newfd + 1 - dtablesize) * sizeof (x[0]));                \
+           x = new_##x;                                                      \
+         }                                                                   \
+       while (0)
+#define EXPAND_DTABLE(newfd) \
+       ({ if ((unsigned int)newfd >= dtablesize                              \
+              && newfd < _hurd_rlimits[RLIMIT_OFILE].rlim_cur)               \
+            {                                                                \
+              /* We need to expand the dtable for the child.  */             \
+              GROW_TABLE (dtable, newfd);                                    \
+              GROW_TABLE (ulink_dtable, newfd);                              \
+              GROW_TABLE (dtable_cells, newfd);                              \
+              dtablesize = newfd + 1;                                        \
+            }                                                                \
+          ((unsigned int)newfd < dtablesize ? 0 : EMFILE);                   \
+        })
 
        struct __spawn_action *action = &file_actions->__actions[i];
 
@@ -425,7 +430,7 @@ __spawni (pid_t *pid, const char *file,
                if (fd == newfd)
                  // Same is same as same was.
                  break;
-               err = expand_dtable (newfd);
+               err = EXPAND_DTABLE (newfd);
                if (!err)
                  {
                    /* Close the old NEWFD and replace it with FD's
@@ -464,7 +469,7 @@ __spawni (pid_t *pid, const char *file,
              do_close (fd);
              if (fd < orig_dtablesize)
                dtable_cloexec[fd] = 0;
-             err = expand_dtable (fd);
+             err = EXPAND_DTABLE (fd);
              if (err)
                break;
 
@@ -472,6 +477,7 @@ __spawni (pid_t *pid, const char *file,
                                  action->action.open_action.oflag,
                                  action->action.open_action.mode,
                                  &dtable[fd]);
+             dtable_cells[fd] = NULL;
              break;
            }
          }