[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Refactor and merge child_execute_job() code
From: |
Pavel Fedin |
Subject: |
Re: [PATCH] Refactor and merge child_execute_job() code |
Date: |
Wed, 26 Feb 2014 21:21:12 +0400 |
Hello, Paul! Sorry for so long delay, i'm really quite busy, however i
have found some time to get back to this. Please review the new
version.
Monday, February 3, 2014, 0:30:26 you wrote:
> I prefer the macro form as well; please keep it that way.
Done.
> Also, I'm not sure I like the current child_execute_job() where there
> are two completely different implementations in the same function,
> handled by ifdef. But I guess we do the same thing in other places.
Yes. This function does very OS-specific things, and personally i
don't see how it could be done in some other way. Well, except if we
completely split it up and introduce some new OS-specific .c files
which will hold implementations of OS-specific functions.
> Can you push down the environ pointer resetting into exec_command(),
> rather than having the save/restore in start_job_command()? We don't
> need this on POSIX systems so it would be nice if it was not done there.
Done. After some code review i have noticed that we actually don't
need this at all when using fork(), because execvp() is executed in
child context. So only spawn() code actually uses it now.
> However it seems we might be able to simplify some things, especially on
> POSIX systems, by pushing the handling of it down into
> child_execute_job(). To do that we'd need to pass the flags argument to
> child_execute_job(), or at least a boolean value specifying whether
> COMMANDS_RECURSE is set. But then on POSIX systems, at least, we could
> do the close-on-exec in the child's fork and we wouldn't have to undo
> it.
Yes, we can do also this. However perhaps this would not look too
good because we call child_execute_job() from two different places,
and we need to deal with these fd's only in one place. I think it's
not a big loss to reset the flag.
--
С уважением,
Pavel mailto:address@hidden
make-merge-child_execute_job-code.diff
Description: Binary data