From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34691 invoked by alias); 26 Feb 2016 20:18:38 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 34677 invoked by uid 89); 26 Feb 2016 20:18:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=white X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH 3/3] posix: New Linux posix_spawn{p} implementation To: Adhemerval Zanella , libc-alpha@sourceware.org References: <1456495001-5298-1-git-send-email-adhemerval.zanella@linaro.org> <1456495001-5298-4-git-send-email-adhemerval.zanella@linaro.org> From: Paul Eggert Message-ID: <56D0B319.3060005@cs.ucla.edu> Date: Fri, 26 Feb 2016 20:49:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1456495001-5298-4-git-send-email-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2016-02/txt/msg00849.txt.bz2 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.