> >This framework (originally introduced by Bernhard a long time ago,
> >and enhanced by me later) is very nice for invoking external programs.
> >It supports background execution of programs, callbacks when programs
> >end, and is pretty robust when it comes to error situations.
>
> Do you mean Systemcalls class? If so, I once wrote a little patch to
> use spawn*() instead of fork()/exec*(). Maybe its of interest for
> win32, too.
I'm very happy that you did this work to make the framework better
on OS/2. However, the patch needs a little polishing before I would
let it go in:
> +// how many strings allowed in command line (AHanses)
> +#ifndef MAX_ARGV
> +#define MAX_ARGV 255
> +#endif
> +
We don't like #define's. Is this really necessary?
> #include <unistd.h>
> +#include <process.h>
> #include "syscall.h"
I don't this we need this for Unix.
> case Wait:
> - pid = Fork();
> + pid = FORK(P_WAIT);
I prefer to let the method be called Fork. Why did you change it
to FORK?
> if (pid>0) { // Fork succesful. Wait for child
> +#ifndef __EMX__
> waitForChild();
> +#else
> + retval = pid;
> +#endif
Would it be possible to use the alternative that waitForChild is
different on EMX?
> // Wait for child process to finish. Returns returncode from child.
> +#ifndef __EMX__
> void Systemcalls::waitForChild()
> {
> }
> +#endif
See above: Instead of dropping this function, please just make
the waitForChild operation a dummy operation.
> -pid_t Systemcalls::Fork()
> +pid_t Systemcalls::FORK(int spawnFlag)
Once again, please do not rename the function to capital only letters.
> - const int MAX_ARGV = 255;
If the problem is that MAX_ARGV might be defined already in OS/2,
I think the better solution is to rename this local constant.
> +/** AHanses: Eliminate unneeded parameter for Unix: 'fork(void)'
> +
> +
> + Unix will just ignore the spawnFlag parameter
> +
> +
> +
> + */
> +
> +
> +
> +
This comment should go. The remark that Unix should ignore the
spawnFlag should go as a comment near the Fork-method.
Finally, the patch should be remade against the latest cvs version.
I updated the SystemCalls code a little while ago. Among other
good stuff, it includes now the first cut at providing a kill()
feature for background processes.
This is to be extended to monitor the process, and if it does not
react to a SIGHUP at first, we would send progressively more aggressive
signals to the process until it is dead for sure.
Greets,
Asger
