From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40127 invoked by alias); 22 Sep 2016 20:54:07 -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 40107 invoked by uid 89); 22 Sep 2016 20:54:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=*restrict, UD:string.h, string.h, 79,7 X-HELO: mail-yw0-f179.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=d4cetbOmp5G6qK0YFqmvTvEG+v2+r1iIv9I4mJXp5VU=; b=HHceTvhfFtfaWZ+WhoyGBd5ok00JpxLyuE0AHEAXS1m/B7sjGf88LtmD1uCqePKlzW z9eILdyBB9l5MnlmSqTWyVpnxPUx92hmbxyVM6Z7Jenc+s3W+h3QtpA/JWfgqXZbI4Y3 KlNKposQiePJ5EQy8TbUZMd8pxWCEkEwCTGZxYZ2DQTg6GTVhYFfezynGahXtyG0+IJJ hj/7V/Wht4Y9SYD7CsrpTUldsDvcSZ5LtGpiZpgYuu19MJ/VSEiGhsx71kLPESr4SM+L QHL73WZ+7XQO/8XXn8OJyGhGUYoRnbn5P0ho3ubj+1CS7Mn3qAfyaiWHzp9NcwdSOQIu 1WwQ== X-Gm-Message-State: AE9vXwPn9MWW9xRioI9ZmR182lX5IG5tFGfCFs9OAAWZON/3Y4RhStsw3+f+2ZEYnE9RtVhU X-Received: by 10.129.178.130 with SMTP id q124mr3071060ywh.328.1474577634628; Thu, 22 Sep 2016 13:53:54 -0700 (PDT) Subject: Re: [PATCH] linux: spawni.c: simplify error reporting to parent To: libc-alpha@sourceware.org References: <877fa6nwsl.fsf@mid.deneb.enyo.de> <1474405260-16657-1-git-send-email-rv@rasmusvillemoes.dk> From: Adhemerval Zanella Message-ID: <0b37fd86-671e-9b79-17e7-0efe8345178b@linaro.org> Date: Thu, 22 Sep 2016 20:54:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1474405260-16657-1-git-send-email-rv@rasmusvillemoes.dk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-09/txt/msg00470.txt.bz2 LGTM, I think it addressed all the comments. On 20/09/2016 18:01, Rasmus Villemoes wrote: > Using VFORK already ensures that the parent does not run until the > child has either exec'ed succesfully or called _exit. Hence we don't > need to read from a CLOEXEC pipe to ensure proper synchronization - we > just make explicit use of the fact the the child and parent run in the > same VM, so the child can write an error code to a field of the > posix_spawn_args struct instead of sending it through a pipe. > > To ensure that this mechanism really works, the parent initializes the > field to -1 and the child writes 0 before execing. > > This eliminates some annoying bookkeeping that is necessary to avoid > the file actions from clobbering the write end of the pipe, and > getting rid of the pipe creation in the first place means fewer system > calls (four in the parent, usually one in the child) and fewer > chanches for the spawn to fail (e.g. if we're close to EMFILE). > > Signed-off-by: Rasmus Villemoes > --- > sysdeps/unix/sysv/linux/spawni.c | 71 ++++++++++++++-------------------------- > 1 file changed, 25 insertions(+), 46 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c > index bb3eecf..c0e15ac 100644 > --- a/sysdeps/unix/sysv/linux/spawni.c > +++ b/sysdeps/unix/sysv/linux/spawni.c > @@ -17,6 +17,7 @@ > . */ > > #include > +#include > #include > #include > #include > @@ -44,11 +45,12 @@ > 3. Child must synchronize with parent to enforce 2. and to possible > return execv issues. > > - The first issue is solved by blocking all signals in child, even the > - NPTL-internal ones (SIGCANCEL and SIGSETXID). The second and third issue > - is done by a stack allocation in parent and a synchronization with using > - a pipe or waitpid (in case or error). The pipe has the advantage of > - allowing the child the communicate an exec error. */ > + The first issue is solved by blocking all signals in child, even > + the NPTL-internal ones (SIGCANCEL and SIGSETXID). The second and > + third issue is done by a stack allocation in parent, and by using a > + field in struct spawn_args where the child can write an error > + code. CLONE_VFORK ensures that the parent does not run until the > + child has either exec'ed successfully or exited. */ > > > /* The Unix standard contains a long explanation of the way to signal > @@ -79,7 +81,6 @@ > > struct posix_spawn_args > { > - int pipe[2]; > sigset_t oldmask; > const char *file; > int (*exec) (const char *, char *const *, char *const *); > @@ -89,6 +90,7 @@ struct posix_spawn_args > ptrdiff_t argc; > char *const *envp; > int xflags; > + int err; > }; > > /* Older version requires that shell script without shebang definition > @@ -125,11 +127,8 @@ __spawni_child (void *arguments) > struct posix_spawn_args *args = arguments; > const posix_spawnattr_t *restrict attr = args->attr; > const posix_spawn_file_actions_t *file_actions = args->fa; > - int p = args->pipe[1]; > int ret; > > - close_not_cancel (args->pipe[0]); > - > /* The child must ensure that no signal handler are enabled because it shared > memory with parent, so the signal disposition must be either SIG_DFL or > SIG_IGN. It does by iterating over all signals and although it could > @@ -203,17 +202,6 @@ __spawni_child (void *arguments) > { > struct __spawn_action *action = &file_actions->__actions[cnt]; > > - /* Dup the pipe fd onto an unoccupied one to avoid any file > - operation to clobber it. */ > - if ((action->action.close_action.fd == p) > - || (action->action.open_action.fd == p) > - || (action->action.dup2_action.fd == p)) > - { > - if ((ret = __dup (p)) < 0) > - goto fail; > - p = ret; > - } > - > switch (action->tag) > { > case spawn_do_close: > @@ -273,6 +261,7 @@ __spawni_child (void *arguments) > __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK) > ? &attr->__ss : &args->oldmask, 0); > > + args->err = 0; > args->exec (args->file, args->argv, args->envp); > > /* This is compatibility function required to enable posix_spawn run > @@ -280,14 +269,13 @@ __spawni_child (void *arguments) > (2.15). */ > maybe_script_execute (args); > > - ret = -errno; > - > fail: > - /* Since sizeof errno < PIPE_BUF, the write is atomic. */ > - ret = -ret; > - if (ret) > - while (write_not_cancel (p, &ret, sizeof ret) < 0) > - continue; > + /* errno should have an appropriate non-zero value; otherwise, > + there's a bug in glibc or the kernel. For lack of an error code > + (EINTERNALBUG) describing that, use ECHILD. Another option would > + be to set args->err to some negative sentinel and have the parent > + abort(), but that seems needlessly harsh. */ > + args->err = errno ? : ECHILD; > _exit (SPAWN_ERROR); > } > > @@ -304,9 +292,6 @@ __spawnix (pid_t * pid, const char *file, > struct posix_spawn_args args; > int ec; > > - if (__pipe2 (args.pipe, O_CLOEXEC)) > - return errno; > - > /* To avoid imposing hard limits on posix_spawn{p} the total number of > arguments is first calculated to allocate a mmap to hold all possible > values. */ > @@ -333,15 +318,14 @@ __spawnix (pid_t * pid, const char *file, > void *stack = __mmap (NULL, stack_size, prot, > MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); > if (__glibc_unlikely (stack == MAP_FAILED)) > - { > - close_not_cancel (args.pipe[0]); > - close_not_cancel (args.pipe[1]); > - return errno; > - } > + return errno; > > /* Disable asynchronous cancellation. */ > int cs = LIBC_CANCEL_ASYNC (); > > + /* Child must set args.err to something non-negative - we rely on > + the parent and child sharing VM. */ > + args.err = -1; > args.file = file; > args.exec = exec; > args.fa = file_actions; > @@ -355,9 +339,8 @@ __spawnix (pid_t * pid, const char *file, > > /* The clone flags used will create a new child that will run in the same > memory space (CLONE_VM) and the execution of calling thread will be > - suspend until the child calls execve or _exit. These condition as > - signal below either by pipe write (_exit with SPAWN_ERROR) or > - a successful execve. > + suspend until the child calls execve or _exit. > + > Also since the calling thread execution will be suspend, there is not > need for CLONE_SETTLS. Although parent and child share the same TLS > namespace, there will be no concurrent access for TLS variables (errno > @@ -365,22 +348,18 @@ __spawnix (pid_t * pid, const char *file, > new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size, > CLONE_VM | CLONE_VFORK | SIGCHLD, &args); > > - close_not_cancel (args.pipe[1]); > - > if (new_pid > 0) > { > - if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) > - ec = 0; > - else > - __waitpid (new_pid, NULL, 0); > + ec = args.err; > + assert (ec >= 0); > + if (ec != 0) > + __waitpid (new_pid, NULL, 0); > } > else > ec = -new_pid; > > __munmap (stack, stack_size); > > - close_not_cancel (args.pipe[0]); > - > if ((ec == 0) && (pid != NULL)) > *pid = new_pid; > >