call-process & other process primitives
David Banks
I'm writing a compatibility layer to another R6RS process manipulation
API and I have encountered some holes in the Mosh support. One
function that is useful that was recently introduced is
'call-process'. Seeing as this is mentioned in RELNOTE I assume it's
intended to be a public API, however it's not documented yet. I
enclose a tiny patch to document it, apply it if you like it.
On a side note: if 'call-process' is intended to be a public API, it
might be in the wrong library: it's in the interaction environment and
(system) which seems to mostly be the home of internal stuff. The
patch I have attached adds documentation in Core.txt (as is the
current situation), but it might be better off in (mosh process).
More generally, the following features would be useful:
* getpid() wrapper - should return the PID of the top level interpreter.
* system() wrapper - giving the exit status of the command, which
'call-process' doesn't do ATM
* Enhanced 'waitpid' from (mosh process) that uses WTERMSIG() macro to
return the signal that terminated the child.
* Support modifying the environment in 'spawn' from (mosh process)
using execve's envp argument.
I can write these features and submit them as patches, is there any
that would definitely NOT be accepted? Any tips or policies on
implementing them?
Cheers,
--
David Banks <amo...@gmail.com>
okuoku
> Hi list,
- snip -
> * getpid() wrapper - should return the PID of the top level interpreter.
FYI, I had implemented getpid as %getpid primitive function.
Nmosh can use it like:
nmosh> (import (primitives %getpid))
nmosh> (%getpid)
Currently, you cannot use it from psyntax-mosh.(I don't know why.)
(Now I'm focusing Win32 port; I think Higepon will answer to your proposal..
IMHO, these should be implemented.)
higepon
>One function that is useful that was recently introduced is
>'call-process'. Seeing as this is mentioned in RELNOTE I assume it's
>intended to be a public API, however it's not documented yet.
call-process is a deprecated API.
It should be rewriten and replaced using (mosh process) and exported
from the library.
As you pointed out, it need to give exit status also.
>I can write these features and submit them as patches,
I would appreciate them.
>is there any that would definitely NOT be accepted?
system may not be necessary, if you can write call-process which
returns exit status.
>Any tips or policies on implementing them?
Small patches for each procedures and tests is preferable.
If you don't understand something about implementations or policies,
please ask us.
Cheers,
> --
> You received this message because you are subscribed to the Google Groups "Mosh Developer Disscus" group.
> To post to this group, send email to mosh-develo...@googlegroups.com.
> To unsubscribe from this group, send email to mosh-developer-di...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/mosh-developer-discuss?hl=en.
>
>
David Banks
> Small patches for each procedures and tests is preferable.
> If you don't understand something about implementations or policies,
> please ask us.
Hi, here is a patch to expose the getpid wrapper in (mosh process),
plus documentation and a test.
Let me know if there is any issue with it.
I plan to add tests for the rest of the features to the same
'tests/process.scm' file, that's why I added it to tests/Makefile.
Note that there are changes to the psyntax buildscript so psyntax.scm
will have to rebuilt. (By the way, is there a better way of doing
this? At the moment I am just removing the 'svn' command from the
bootstrap target whenever I clone, to stop gen-git-build.sh from
overwriting it with the master version.)
higepon
I will check it later, please give me a little more time.
Cheers.
higepon
You can add your name to the header of source code.
If you want, please add your name, on next patch.
Cheers.
https://github.com/higepon/mosh/commit/633139afa1aa0b56da1254ae442a0313ffaf10c0
David Banks
> The patch applied.
> You can add your name to the header of source code.
> If you want, please add your name, on next patch.
Thanks for your time Higepon.
David Banks
On 20 April 2011 12:12, higepon <hig...@gmail.com> wrote:
> The patch applied.
>
> https://github.com/higepon/mosh/commit/633139afa1aa0b56da1254ae442a0313ffaf10c0
Sorry for delay - just noticed that tests/process.scm from my patch
was not committed.
It's still referenced from tests/Makefile so at the moment 'make
check' target is broken.
higepon
Added.
Cheers.
David Banks
Things to note about this:
* The native call-process is renamed to %call-process and renamed by
(mosh process) on export, as happens with the other procedures.
* call-process is extended to return complete stdout, exit status, and
the terminating signal. If those are unknown #f is returned in place
of them.
* I have not handled non-ASCII at all. call-process is using
ascii_c_str() on the command, as does exec(), so non-ascii will be
silently eaten. I guess this should be handled in a separate change.
* Only unix platforms are supported.
Please let me know any issues. I understand Mosh will probably be
frozen for release of 0.2.7 so don't worry about the timing.
higepon
The patch seems good.
Some day we should rewrite call-process using spawn on (mosh process).
>okuoku
Can I apply the patch to the master?
Or should I wait 0.2.7?
Cheers.
okuoku
- snip -
>>okuoku
>
> Can I apply the patch to the master?
It's OK to import now.
(There is no "code freeze" step like gcc.. Untested feature will be
leaved as just undocumented.)
higepon
https://github.com/higepon/mosh/commit/2a1ac9cec16bb9b5d8b8881442bd039dd1c2ba46
Cheers.
David Banks
> The patch applied.
>
> https://github.com/higepon/mosh/commit/2a1ac9cec16bb9b5d8b8881442bd039dd1c2ba46
Thanks Higepon.
I will add the {} in future patches. :)
--
David Banks <amo...@gmail.com>
David Banks
Here's a patch to allow waitpid() wrapper in (mosh process) to return
the exit status and terminating signal, like call-process. Tests and
docs included.
Things to note:
At the moment I duplicate the code for determining the return value,
which is very similar to call-process. This was done to keep the
patch changes minimal. Personally I would probably prefer to abstract
it into a function, but perhaps it would be too small - what do you
think?
higepon
Thank you for your patch.
The patch seems good.
> Personally I would probably prefer to abstract
> it into a function, but perhaps it would be too small - what do you
> think?
Agreed. Less code is preferable.
Could you make an another patch?
Cheers.
Taro Minowa(Higepon)
David Banks
> Agreed. Less code is preferable.
> Could you make an another patch?
I've added two functions to OSCompat.cpp to extract the appropriate
Scheme object representing a piece of process information.
call-process and waitpid are modified to use these functions.
They don't totally fit here as they don't have non-Unix
implementations, but it seems this was the best place to put them.
higepon
Perfect!.
The patch is applied and tested.
https://github.com/higepon/mosh/commit/b8a1643f053b9db3a162446a017413171ef6d4cb
Cheers.
okuoku
> On 16 May 2011 12:23, higepon <hig...@gmail.com> wrote:
>> Agreed. Less code is preferable.
>> Could you make an another patch?
>
> I've added two functions to OSCompat.cpp to extract the appropriate
> Scheme object representing a piece of process information.
> call-process and waitpid are modified to use these functions.
I've added some signal definitions to os-constant.
https://github.com/okuoku/mosh/commit/b0e540d306b1dca576995d4793e09c0b26dd1150
In some kernels like Linux, signal numbers are different between architectures.
This change allows:
(import (rnrs) (mosh))
(os-constant 'SIGKILL) ;; => maybe 9
This feature will be included within 0.2.7 release.
> They don't totally fit here as they don't have non-Unix
> implementations, but it seems this was the best place to put them.
I think we should prepare something likely named "POSIXProcedures.cpp"...
For example, in my repository (
https://github.com/okuoku/mosh/tree/master/src ), I had split OS
specific feature related to Async-I/O to
- src/win32 - WindowsXP or later specific functions
- src/bsd - FreeBSD/Darwin specific functions
- ...
Because asynchronous I/O is very OS specific feature. OTOH, such
regions are relatively small in current Mosh
and (at least for 0.2.7,) your solution is the best (I think).
-- oku
David Banks
I have a few questions about the last piece of functionality from my original
message, the ability to modify the environment of a spawned subprocess. I have
a working patch for this functionality but I'm not sure if it is well designed.
The basic problem is that 'spawn' from (mosh process) uses the 'execvp' call
which does an implicit path search. That's not exactly documented but the
example given in the documentation does use that functionality: it calls (spawn
"ls" ...) rather than (spawn "/bin/ls" ...). I agree this is useful
functionality although I would want the ability to disable it.
I wanted to avoid breaking any APIs so my current patch extends the SPAWN
procedure with two optional arguments:
(spawn command args . io-list path-search? env)
If path-search? is #f, SPAWN will use execve() to do the call.
If path-search? is #t, SPAWN will use execvpe() to do the call.
Spawn will behave exactly the same as it does now if neither
path-search? nor env are provided.
Now the more pressing matter is that 'execvpe' doesn't exist ;) Well, it does
exist in some systems but not in POSIX. As a result I ported the execvpe from
FreeBSD to mosh - which I hope is OK from a copyright perspective. Most of the
code can be seen here:
http://www.freebsd.org/cgi/cvsweb.cgi/src/lib/libc/gen/exec.c?rev=1.27
In the function execvPe(). I don't very much like porting a ~150 line
chunk of OS
code into Mosh, but this seems to be the only robust way to do it.
[I thought about probing for the function in the configure script but doing that
means the feature will only work on certain platforms, the behaviour might also
be inconsistent in user code as the function is unspecified, and it would
complicate the Mosh code as well.]
The other way that it could be done is to implement execvpe() in Scheme using
execve() as a primitive. The trouble with this is handling the various error
cases which are far more convenient to handle from C given that AFAIK Mosh
doesn't have any wrapper to errno values. Also note that execvp() is not a
sufficient primitive so this would mean a change to the API anyway.
Also separate to this implementation question do you think it's sensible to keep
all the options in a single Scheme procedure as shown? I think that's the best
way, but some others might prefer SPAWN-WITH-ENVIRONMENT or similar.
And one more question in this long mail ;)
What's the general feeling on using C++ features? For instance, I might want to
rewrite the FreeBSD execvpe() to use the C++ string classes rather than just
porting it. Is there anything I should definitely not use?
So to sum up (TL;DR):
1. Is it OK to clone the FreeBSD execvpe() like this?
2. Does the extension of the SPAWN procedure sound OK?
3. How should I conform the BSD function to Mosh C++ style?
Cheers,
David
okuoku
> Hi all,
- snip -
> The other way that it could be done is to implement execvpe() in Scheme using
> execve() as a primitive. The trouble with this is handling the various error
> cases which are far more convenient to handle from C given that AFAIK Mosh
> doesn't have any wrapper to errno values.
I prefer this approach. We can add error encodes like EACCESS or EIO ...
into OSConstants.cpp.
> Also separate to this implementation question do you think it's sensible to keep
> all the options in a single Scheme procedure as shown?
Yeah, we can wrap our SPAWN if they need..
> What's the general feeling on using C++ features? For instance, I might want to
> rewrite the FreeBSD execvpe() to use the C++ string classes rather than just
> porting it. Is there anything I should definitely not use?
Currently, we don't have any style guide-lines and I think OS-specific
feature implementation can utilize wider constructs from C++
standards.
For example, scheme::getMoshExecutablePath in OSCompat.cpp will use
std::string in some platforms.
But as I mentioned above, I do prefer "implement in Scheme" approach :)
Exceptions and RTTIs are not used in Mosh at all. (Because Mosh was
initially developed as a shell for MonaOS -- Higepon's homebrew OS for
i386 PC -- and it doesn't support them.)
-- oku
higepon
Thank you for your work.
> 1. Is it OK to clone the FreeBSD execvpe() like this?
Yes.
As okuoku said we prefer to write it in Scheme if possible.
> 2. Does the extension of the SPAWN procedure sound OK?
Seems good.
> 3. How should I conform the BSD function to Mosh C++ style?
When you port something, just keep it as it it.
Changing style will cause a bug.
Cheers.
higepon
FYI.
I've added two empty procedures to (mosh process).
- (process-terminate! id)
kill process
- (process-list)
returns a list of a-lists which hold process information.
For now both of them are implemented for MonaOS only.
Cheers.
David Banks
Note that this is quite disruptive. Here's the approach I took:
* Change internalExecEx C++ procedure to be a very thin wrapper over execve().
* Expose dup2() as %dup procedure to Mosh space. (Not user-exposed,
but it could be.)
* (execvpe) procedure in (mosh process) implements FreeBSD semantics
for execvpe()
* (exec) front end handles the various code paths and uses %dup to
handle IO redirection
* (spawn) forks and calls to exec
Now there are a few caveats:
Rewriting this functionality in scheme raises the question of where to
put it? As well as psyntax-mosh and nmosh, there is "vanilla mosh".
I guess the files in boot/baselib are available to vanilla mosh. As a
result my changes will break use of spawn and exec in vanilla mosh. I
don't know if this is important or not.
Also the code under (mosh process) uses a few SRFIs and other mosh
libs, it's all included in Mosh so I figure it shouldn't be a big
problem. The resulting code is slightly slower than before (1/3
slower in my very unscientific testing) but that would probably be
expected given that I am bringing code into Scheme from C++. I think
most programs that use subprocesses will have significant I/O overhead
in any case that will swamp this overhead.
One thing I found is that the existing implementation of
internalExecEx() calls callAssertionViolationImmidiaImmediately() on
an error, so users who try to run something nonexistent will get an
error and an immediate termination. However, with the code moved into
Scheme-space it seems to be hard to do this from within SPAWN. EXEC
can just raise an exception, but if 'spawn' doesn't catch the
exception then the calling code will just fork into two. For the
moment I have blanket-caught all exceptions within 'spawn' and just
exit the child process in that case. Not sure what the best way to
handle that is.
I had to modify (mosh shell) to use the new version of 'spawn' because
%spawn is not exported by (system) anymore. I just noticed that (mosh
shell) is apparently deprecated, but the test for input-output-port
uses it.
One of the tests - testing the case where path-search? is #f - uses an
executable shell script 'printf.sh' which it renames to 'printf' to
test if we can unambiguously invoke local commands with the same name
as POSIX commands. The test is automatically skipped if the file
doesn't exist or is not executable, so hopefully it shouldn't cause
spurious failures. I think the diff will preserve the file mode, if
not, it needs to be set executable in the git repository. (mosh file)
doesn't export chmod wrapper yet so I can't chmod it from the test.
Note that I've exported all of the POSIX errno codes and the confstr()
codes from POSIX 2008. They seem to be conditionally defined anyway
so shouldn't cause build failures.
And the biggest fail, so far this is only tested on Linux. (It hasn't
been tested on nmosh at all as I can't run it at the moment.) Please
forward any errors to me and I'll investigate.
okuoku
- snip -
> Now there are a few caveats:
> Rewriting this functionality in scheme raises the question of where to
> put it? As well as psyntax-mosh and nmosh, there is "vanilla mosh".
> I guess the files in boot/baselib are available to vanilla mosh. As a
> result my changes will break use of spawn and exec in vanilla mosh. I
> don't know if this is important or not.
Vanilla-mosh is *not* important. It's intended to use with bootstrap process.
> Also the code under (mosh process) uses a few SRFIs and other mosh
> libs, it's all included in Mosh so I figure it shouldn't be a big
> problem. The resulting code is slightly slower than before (1/3
> slower in my very unscientific testing) but that would probably be
> expected given that I am bringing code into Scheme from C++. I think
> most programs that use subprocesses will have significant I/O overhead
> in any case that will swamp this overhead.
Relying other (mosh ...) libraries is OK.
SRFI-98 is derived from mosh's API, so there is "native"
implementation within (system) library.
i.e. You can safely replace (srfi :98) with (system).
> And the biggest fail, so far this is only tested on Linux. (It hasn't
> been tested on nmosh at all as I can't run it at the moment.) Please
> forward any errors to me and I'll investigate.
Don't worry about them. I'm a platform integration professional :)
If your nmosh won't run but psyntax-mosh run flawlessly, please run
./gen-git-build.sh twice like:
./gen-git-build.sh => configure/make/install => remove .mosh directory
=> ./gen-git-build.sh => configure/make/install
(Install git version of mosh is needed, because nmosh bootstrap
process rely psyntax-mosh on $PATH.)
(Or just install
http://storage.osdev.info/pub/mosh/mosh-current.tar.gz and
build/install git tree.
Note that mosh-current.tar.gz is release tarball and you don't need to
run ./gen-git-build.sh for them.)
higepon
I'll check the patch tomorrow.
Cheers.
higepon
Thank you for your patch.
The patch applied.
https://github.com/higepon/mosh/commit/aff1a0a0fa8014a5aea7388f5485ed97a5afcd49
>Note that this is quite disruptive. Here's the approach I took:
Good summary.
>Also the code under (mosh process) uses a few SRFIs and other mosh
>libs, it's all included in Mosh so I figure it shouldn't be a big
>problem.
It's ok, no problem.
>The resulting code is slightly slower than before (1/3
I think it's acceptable, since subprocess do more heavy things.
By the way, I have never used confstr.
I do man constr just now. :D
>One of the tests - testing the case where path-search? is #f - uses an
>executable shell script 'printf.sh' which it renames to 'printf' to
Fine.
>Note that I've exported all of the POSIX errno codes and the confstr()
>codes from POSIX 2008. They seem to be conditionally defined anyway
>so shouldn't cause build failures.
>okuoku and David
Should we expost strerror or something like?
For now, exec shows errro as an integer value which is not user friendly.
Cheers,
David Banks
On 8 June 2011 04:19, higepon <hig...@gmail.com> wrote:
> Thank you for your patch.
> The patch applied.
> https://github.com/higepon/mosh/commit/aff1a0a0fa8014a5aea7388f5485ed97a5afcd49
Cheers!
>>okuoku and David
>
> Should we expost strerror or something like?
> For now, exec shows errro as an integer value which is not user friendly.
Mmm yes. I noticed that in some other cases Mosh automatically
translates errno values. IIRC this is only working for procedures
with C++ implementation atm.
I agree the functionality would be great. But maybe we should be
careful about exporting so much POSIX, just because it creates so much
code. (ironic that I am saying this, ahahah.)
Note that tests/process.scm seems to be totally broken with nmosh at
the moment. (Thanks to okuoku, I got nmosh working again.) I think
this is just because the identifiers have not been exported from
system.nmosh.ss. I will send a patch for this tomorrow.
--
David Banks <amo...@gmail.com>
okuoku
- snip -
> I think
> this is just because the identifiers have not been exported from
> system.nmosh.ss. I will send a patch for this tomorrow.
It's already patched:
https://github.com/okuoku/mosh/commit/df3c2566bd12ffa4ffa497afe9380d61719123e0
..I think we really need single development "trunk" like svn to avoid
duplicate work and merge conflicts.
IMO, current (mosh) and (system) libraries are badly complicated.
(system) library should be generated from free-vars.scm
but in current Mosh, identifiers exported from free-vars.scm(i.e.
defined in C++) is split into (mosh) and (system) in random order...
-- oku
okuoku
> Hi
- snip -
>>okuoku and David
>
> Should we expost strerror or something like?
> For now, exec shows errro as an integer value which is not user friendly.
Of course. But they might break non-UTF8 locales and we need some
workaround(e.g. setlocale() to C)
-- oku
higepon
> https://github.com/okuoku/mosh/commit/df3c2566bd12ffa4ffa497afe9380d61719123e0
>
> ..I think we really need single development "trunk" like svn to avoid
> duplicate work and merge conflicts.
I think we have two options.
(1) If you commit something to be placed master.
Just send me "pull request".
(2) You can commit to master directory. (github collaborators).
> IMO, current (mosh) and (system) libraries are badly complicated.
> (system) library should be generated from free-vars.scm
> but in current Mosh, identifiers exported from free-vars.scm(i.e.
> defined in C++) is split into (mosh) and (system) in random order...
Yes.
They should be fixed, but have low priority now.
Cheers.
higepon
> workaround(e.g. setlocale() to C)
OK, I write it when needed.