Re: [PATCH v2 5/5] posix: Use posix_spawn for wordexp

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
> index 10a0768a6b..ef780b0a65 100644
> --- a/posix/wordexp-test.c
> +++ b/posix/wordexp-test.c

> -/* For each fork increment the fork count.  */
> -static void
> -register_fork (void)
> -{
> -  registered_forks++;
> -}

It's a bit sad to see this testing go away.  It was originally added to
catch command execution with WRDE_NOCMD.

On Linux, could you enter a PID namespace instead and check that the
next PID has the expected value?

Carlos, you added this testing.  Do you have an opinion here?

> diff --git a/posix/wordexp.c b/posix/wordexp.c
> index 22c6d18a9c..e1aafcaceb 100644
> --- a/posix/wordexp.c
> +++ b/posix/wordexp.c

> +static char **
> +create_environment (void)
> +{
> +  size_t s = 0;
> +
> +  /* Calculate total environment size, including 'IFS' if is present.  */
> +  for (char **ep = __environ; *ep != NULL; ep++, s++);

I would put s++ into the body of the for loop, for clarity.  Or give ep
a wider scope and use s = ep -- __environ.

> +  /* Include final NULL pointer.  */
> +  char **newenviron = malloc (s * sizeof (char*));
> +  if (newenviron == NULL)
> +    return NULL;

char* should be char *.  I don't see how this includes the final NULL?

Should we do all this work only if IFS= is actually present?  That is,
skip all this for getenv ("IFS) == NULL?

> +  /* Copy current environment excluding 'IFS', to make sure the subshell
> +     doesn't field-split on our behalf. */

That comment should apply to the entire function, I think.

> @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length,
>    size_t maxnewlines = 0;
>    char buffer[bufsize];
>    pid_t pid;
> -  int noexec = 0;
> +  bool noexec = false;
>  
>    /* Do nothing if command substitution should not succeed.  */
>    if (flags & WRDE_NOCMD)
>      return WRDE_CMDSUB;
>  
> -  /* Don't fork() unless necessary */
> +  /* Don't posix_spawn() unless necessary */

GNU style doesn't use () after function names.

Thanks,
Florian