bug-make
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Refactor and merge child_execute_job() code


From: Eli Zaretskii
Subject: Re: [PATCH] Refactor and merge child_execute_job() code
Date: Thu, 30 Jan 2014 19:29:35 +0200

> Date: Thu, 30 Jan 2014 01:16:34 +0400
> From: Pavel Fedin <address@hidden>
> 
>  Hello!  This is my long-promised refactor. After this it's much easier
> to  apply  runtime  selection  between  spawn()  and fork() on Cygwin,
> because  all  differences  are  now  consolidated  in  two  functions:
> child_execute_process() and exec_command().

Thanks.

>  In    some   critical   POSIX-specific   places   i   have   expanded
> CLOSE_ON_EXEC()   macro   in   order to provide reliable build problem
> tracking.  As  far  as  i  understand,  even  very  old  POSIX systems
> implement this flag, so this should be OK.

This will break the native Windows build, so please don't.  (See job.h
for how this macro is defined on Windows.)  And even if the places
where you made those expansions are not compiled on Windows, having a
macro in some places and its expansion in others is confusing.

Also, I don't understand this part:

> --- a/job.h
> +++ b/job.h
> @@ -126,13 +126,8 @@ int child_execute_job (char *argv, struct child *child);
>  # define FD_STDIN       (fileno (stdin))
>  # define FD_STDOUT      (fileno (stdout))
>  # define FD_STDERR      (fileno (stderr))
> -# if defined(__EMX__)
> -int child_execute_job (int stdin_fd, int stdout_fd, int stderr_fd,
> +pid_t child_execute_job (int stdin_fd, int stdout_fd, int stderr_fd,
>                         char **argv, char **envp);
> -# else
> -void child_execute_job (int stdin_fd, int stdout_fd, int stderr_fd,
> -                        char **argv, char **envp) __attribute__ ((noreturn));
> -# endif

Now systems other than EMX that use 'fork/exec' will not declare
child_execute_job as a function that does not return, which is needed,
since the call to 'exec_command' doesn't return.

And what about this FIXME comment:

> +      /* undo FD_CLOEXEC after the child process has been started
> +         CHECKME: Is this really needed ? These flags should not harm. */
>        if (!(flags & COMMANDS_RECURSE) && job_fds[0] >= 0)
>          {

This is based on a first cursory look; I will review the patch some
more in a day or two.  (And I hope Paul will as well.)

Thanks again for working on this.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]