This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 3/3] posix: New Linux posix_spawn{p} implementation


Similar issues with int vs ptrdiff_t, argc + 1, etc.

This patch creates new lines with trailing white space; please avoid that. (Doesn't git complain about that to you? It does to me.)

/* To avoid impose hard limits on posix_spawn{p} total number of arguments

impose -> imposing
total -> the total

     valus.  */

values

  /* Add a slack area to child own stack. */

child -> the child's

   clobber due stack spilling). The remaning issue are:

Misspelling of "remaining".

   1. That no signal handlers must run in child context, to avoid corrupt

corrupt -> corrupting

   a pipe or waitpid (in case or error). The pipe has the advantage of
   allowing the child the signal an exec error.  */

the signal -> to signal (or better yet, reword to avoid "signal" since this doesn't use signals)

  /* Then apply FD_CLOCEXEC if it is supported in the pipe call case.  */

Misspelled "FD_CLOEXEC".

  /* Try pipe2 even if __ASSUME_PIPE2 is not define and returning an error
     iff the call returns ENOSYS.  */
define and returning -> defined and return

      if (__have_pipe2 > 0)
    return r;

This code is unnecessary and can be removed.

  if (__have_pipe2 < 0)
    if (__pipe (pipe_fds) < 0)
      return -1;

  /* Then apply FD_CLOCEXEC if it is supported in the pipe call case.  */
  if (__have_pipe2 < 0)
    {
      if ((r = __fcntl (pipe_fds[0], F_SETFD, FD_CLOEXEC)) == -1
      || (r = __fcntl (pipe_fds[1], F_SETFD, FD_CLOEXEC)) == -1)
    {
      close_not_cancel (pipe_fds[0]);
      close_not_cancel (pipe_fds[1]);
      return r;
    }
    }

This tests __have_pipe2 twice, whereas it should test it just once.

   to be called explicity using /bin/sh (_PATH_BSHELL).  */

Misspelled "explicitly".

{
  if (xflags & SPAWN_XFLAGS_USE_PATH)
return __spawnix (pid, file, acts, attrp, argv, envp, xflags, __execvpe);
  return __spawnix (pid, file, acts, attrp, argv, envp, xflags, __execve);
}

Put the if-then-else inside the __spawnix call, to avoid repetition. Something like:

   return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);

  const int prot = (PROT_READ | PROT_WRITE
            | ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0));

  /* Add a slack area to child own stack.  */
  const size_t argv_size = (argc * sizeof (void *)) + 512;
  const size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));

There is typically no need for locals to be declared "const". We can easily see that they are not changed by reading the code, and the "const" makes the declaration a bit harder to read.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]