[PATCH 2/2] posix: New Linux posix_spawn{p} implementation

Paul Eggert eggert@cs.ucla.edu
Wed Jan 27 16:17:00 GMT 2016


Adhemerval Zanella wrote:
> +#define SIGALL_SET ((sigset_t *)(const unsigned long long [2]){ -1,-1 })

This shouldn't assume that _SIGSET_NWORDS is 2. And since the code is 
Linux-specific, you should be able to avoid casting to sigset_t *; just write a 
constant that evaluates to sigset_t * without a cast.

> +#ifdef O_CLOEXEC
> +# ifndef __ASSUME_PIPE2
> +  if (__have_pipe2 >= 0)
> +# endif
> +    {
> +      r = __pipe2 (pipe_fds, O_CLOEXEC);
> +# ifndef __ASSUME_PIPE2
> +      if (__have_pipe2 == 0)
...

This sort of code is hard to read. Instead, declare substitutes like this after 
you do your #includes:

   #ifndef __ASSUME_PIPE2
   # define __have_pipe2 1
   #endif
   #ifndef O_CLOEXEC
   # define O_CLOEXEC 0
   #endif

and let the rest of the code just use __have_pipe2 and O_CLOEXEC, without the 
forest of #ifdefs.

> +struct posix_spawn_args {
> +  int p[2];
> +  sigset_t oldmask;
> +  const char *path;

This is a file name, right? Not a path? If so, it should not be called "path".

> +  int (*exec)(const char *, char *const *, char *const *);

Spaces between )(. Also, please check the indenting overall; it seemed a bit 
inconsistent in this file.

> +      while (argv[argc++]);
...
> +    while (write_not_cancel (p, &ret, sizeof ret) < 0);

Use "continue".

> +      /* Construct an argument list for the shell.  */
> +      char *new_argv[argc];

This can overflow the stack.

> +		  /* Only signal errors for file descriptors out of range.  */

"Signal errors only for ..."






More information about the Libc-alpha mailing list