This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 3/3] posix: New Linux posix_spawn{p} implementation
- From: Paul Eggert <eggert at cs dot ucla dot edu>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, libc-alpha at sourceware dot org
- Date: Fri, 26 Feb 2016 12:18:33 -0800
- Subject: Re: [PATCH 3/3] posix: New Linux posix_spawn{p} implementation
- Authentication-results: sourceware.org; auth=none
- References: <1456495001-5298-1-git-send-email-adhemerval dot zanella at linaro dot org> <1456495001-5298-4-git-send-email-adhemerval dot zanella at linaro dot org>
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.