Thread: refactoring fork() and EXEC_BACKEND
While going through the usual motions needed to fork a child process of
the postmaster, it occurred to me that there's a fair bit of duplicated
code involved. There are also #ifdef for various situations (BeOS,
LINUX_PROFILE, and EXEC_BACKEND), which makes the code yet more ugly. I
think we could make this a lot cleaner.
I'd like to define an API like so:
pid_t fork_process(int proc_type);
pid_t fork_backend(Port *port);
If the process needs to add a lot of private information to the argv in
the case of EXEC_BACKEND, they could invoke a third variant:
#ifdef EXEC_BACKEND
pid_t forkexec_process(int proc_type, int argc, char **argv);
#endif
(Or possibly using varargs, if that is cleaner for most call-sites).
Hopefully most call sites could just use fork_process().
These functions would then take care of all the necessary
platform-specific judo:
- flush stdout, stderr
- invoke BeOS hooks as necessary
- save and restore profiling timer, if necessary
- if EXEC_BACKEND, use proc_type to lay out the argv for the new process
and then invoke internal_forkexec()
- otherwise, just invoke fork()
- return result to client
So, most call sites would be quite nice:
pid_t result = fork_process(PROC_TYPE_FOO);
if (result == -1) { /* fork failed, in parent */ }
else if (result == 0) { /* in child */ }
else { /* in parent, `result' is pid of child */ }
I'd also like to move the implementation of fork_process() and friends,
as well as internal_forkexec(), into a separate file -- I'd rather not
clutter up postmaster.c with it.
Comments?
-Neil
>While going through the usual motions needed to fork a child
>process of
>the postmaster, it occurred to me that there's a fair bit of
>duplicated
>code involved. There are also #ifdef for various situations (BeOS,
>LINUX_PROFILE, and EXEC_BACKEND), which makes the code yet
>more ugly. I
>think we could make this a lot cleaner.
>
>I'd like to define an API like so:
This is a lot like what I was planning to work towards with the
refactoring of the forkexec code I promised to do for 8.1. Glad to hear
you think in the same direction.
>pid_t fork_process(int proc_type);
>pid_t fork_backend(Port *port);
>
>If the process needs to add a lot of private information to
>the argv in
>the case of EXEC_BACKEND, they could invoke a third variant:
>
>#ifdef EXEC_BACKEND
>pid_t forkexec_process(int proc_type, int argc, char **argv);
>#endif
>
>(Or possibly using varargs, if that is cleaner for most call-sites).
>Hopefully most call sites could just use fork_process().
I was actually thinking of not passing these on the commandline at all,
in order to avoid possible quoting issues etc (recall all the problems
with the stupid commandline processing on win32). Instead moving it into
a struct that is appended to the end of the backend variable file/shared
memory.
<snip>
>So, most call sites would be quite nice:
>
>pid_t result = fork_process(PROC_TYPE_FOO);
>if (result == -1) { /* fork failed, in parent */ }
>else if (result == 0) { /* in child */ }
>else { /* in parent, `result' is pid of child */ }
You're not going to be able to get the "in child" there for an execed
process, are you? it has to be called somewhere in the new process, and
thus it would have to be a function, wouldn't it?
>I'd also like to move the implementation of fork_process() and
>friends,
>as well as internal_forkexec(), into a separate file -- I'd rather not
>clutter up postmaster.c with it.
That was also what I was thinking. Let me know if you want to split the
load somewhere :-)
//Magnus
Magnus Hagander wrote: > This is a lot like what I was planning to work towards with the > refactoring of the forkexec code I promised to do for 8.1. Cool. BTW, have we accepted that EXEC_BACKEND is the way we're going to workaround the lack of fork() on Win32 for the foreseeable future? I mean, it _works_, but it's slow, ugly, and complicates the code. If it's the only workable option for Win32 support, then fair enough -- I just don't know enough of the Win32 API to know if there's a better alternative out there (short of using threads, which is of course not really plausible). > I was actually thinking of not passing these on the commandline at all, > in order to avoid possible quoting issues etc (recall all the problems > with the stupid commandline processing on win32). Instead moving it into > a struct that is appended to the end of the backend variable file/shared > memory. Sounds good to me. Finding a cleaner way to pass data to the child process than writing it out to a file would also be nice, if possible. Again, I'm not sure what options there are on Win32... > That was also what I was thinking. Let me know if you want to split the > load somewhere :-) Given that you're planning to work on this, I've scaled back my ambitions. I'll send a patch to -patches that just cleans up fork() and doesn't change the EXEC_BACKEND case. So fork_process() will: - flush stderr/stdout - save and restore the profiling timer if LINUX_PROFILE is defined - handle BeOS which means it should not be very invasive. Of course, there is plenty of room for improvement -- if you're interested in taking a look, please do... -Neil
>> This is a lot like what I was planning to work towards with the >> refactoring of the forkexec code I promised to do for 8.1. > >Cool. BTW, have we accepted that EXEC_BACKEND is the way we're >going to >workaround the lack of fork() on Win32 for the foreseeable future? I >mean, it _works_, but it's slow, ugly, and complicates the >code. If it's >the only workable option for Win32 support, then fair enough -- I just >don't know enough of the Win32 API to know if there's a better >alternative out there (short of using threads, which is of course not >really plausible). I don't beleive there is any other way than using threads. The only "objects" you can create are processes and threads, and I don't know there to be any other way to create a process than CreateProcess(). >> I was actually thinking of not passing these on the >commandline at all, >> in order to avoid possible quoting issues etc (recall all >the problems >> with the stupid commandline processing on win32). Instead >moving it into >> a struct that is appended to the end of the backend variable >file/shared >> memory. > >Sounds good to me. Finding a cleaner way to pass data to the child >process than writing it out to a file would also be nice, if possible. >Again, I'm not sure what options there are on Win32... Win32 already passes it using shared memory. It was when I asked to get that patch in during beta (or possibly even RC) that I promised to work on the cleanup stuff for 8.1. For unix/exec_backend it still writes to a file, but since that is never expected to be used in production where performance is an issue... I think that is a fairly clean way of doing it. You could pass it through a pipe or something, but I don't see that it would be a cleaner approach. You're still going to have a single place collecting all the data, which is where most of the uglyness comes from. >> That was also what I was thinking. Let me know if you want >to split the >> load somewhere :-) > >Given that you're planning to work on this, I've scaled back my >ambitions. I'll send a patch to -patches that just cleans up >fork() and >doesn't change the EXEC_BACKEND case. So fork_process() will: > >- flush stderr/stdout >- save and restore the profiling timer if LINUX_PROFILE is defined >- handle BeOS > >which means it should not be very invasive. Of course, there is plenty >of room for improvement -- if you're interested in taking a >look, please >do... Ok. I'll look at it once your stuff is done. //Magnus