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


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.


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