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: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Paul Eggert <eggert at cs dot ucla dot edu>, libc-alpha at sourceware dot org
- Date: Fri, 26 Feb 2016 19:26:49 -0300
- 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> <56D0B319 dot 3060005 at cs dot ucla dot edu>
Thank you very much for the review (and the others too)!
On 26-02-2016 17:18, Paul Eggert wrote:
> 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. */
>
Ack.
> values
>
>> /* Add a slack area to child own stack. */
>
> child -> the child's
>
Ack.
>> clobber due stack spilling). The remaning issue are:
>
> Misspelling of "remaining".
Ack.
>
>> 1. That no signal handlers must run in child context, to avoid corrupt
>
> corrupt -> corrupting
Ack.
>
>> 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)
Ack, I will change to communicate.
>
>> /* 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.
Yes, I already did after Joseph's comments.
>
>> 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.
I remove this code.
>
> to be called explicity using /bin/sh (_PATH_BSHELL). */
>
> Misspelled "explicitly".
Ack.
>
>> {
>> 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);
I will change it.
>
>> 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.
I disagree with this, I see using const on complex expression might help compiler
get logic errors (where the variable is set to a different value) and states
explicit that it should be a constant value. Also, imho I do not see it as
harder to read.