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

[Pavel Fedin]

 Hello!

> 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.

 Ok, i actually can leave it as a macro. I decided to expand it for better
build reliability. I imagined that in some weird situations (broken include
files for example, broken compiler, whatever) there will be no compile
error, but the resulting executable will leak fd's. So i decided to make
sure that FD_CLOEXEC is really set. Yes, this part of code really does not
compile on Windows.
 But if you dislike it, of course it's possible to keep a macro.

> 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.

 But child_execute_job() does return, because now fork() is moved inside it.
It returns in parent context. To be more clear, here is algorithmic
difference:
 Old version: fork(), then (in child's context) manually close() unneeded
fd's, then jump to child_execute_job() which does not return.
 New version: set CLOEXEC flag for unneeded fd's then call
child_execute_job(). It fork()s inside, and returns in parent context.
 As a result, we now have this fork() in a single place (instead of two),
and it is much easier to introduce a runtime switch between fork() and
spawn() now.

> 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)
> >          {

 It's there to attract other's attention (it did the job :)) and gather some
opinions. What do you think, may be this is really redundant to reset
CLOEXEC after forking ? I don't see any situations where these fd's should
be passed on to child process. Every time we run a child, we use only
redirection of stdin/stdout/stderr. We don't run any special children which
would expect more fd's to be there.

> Thanks again for working on this.

 Thank you for your appreciation.
 By the way, i have two systems of Amiga family (MorphOS and AROS), are you
interested in testing and some face-lifting to Amiga-specific code ?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia