Skip to content

No process spawn #3819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 3, 2015
Merged

No process spawn #3819

merged 1 commit into from
Nov 3, 2015

Conversation

juj
Copy link
Collaborator

@juj juj commented Sep 25, 2015

Replace system() and popen() with failing stubs. Fixes error 'unresolved symbol posix_spawn' when building code that calls to popen.

@kripken
Copy link
Member

kripken commented Sep 25, 2015

How about if we use the musl popen, which is at system/lib/libc/musl/src/stdio/popen.c? If we build it, it should do a syscall, and we can just implement that syscall to return the runtime error. In general fewer changes to using musl as-is seems better to me.

@juj
Copy link
Collaborator Author

juj commented Sep 25, 2015

We did build it before, but we don't build any of the posix_spawn_... functions, which are blacklisted since the whole process module is blacklisted. That module contains some 30 functions, none of which we actually would support. I tried building that module, but there's a few failures, and thought that this would be simpler?

@kripken
Copy link
Member

kripken commented Sep 25, 2015

It should be simpler the other way, I think - stop us from blacklisting the process module, and blacklist the unneeded methods in there. Fewer changes to musl that way.

But, doesn't the argument in this pull apply to all of them? We don't support any of these methods, and before this pull, all warn on build, and all abort if called; after this pull, that changes only for popen and system, but why not do the same for all process methods?

@kripken
Copy link
Member

kripken commented Sep 25, 2015

Alternatively, if we think popen and spawn are special and we just want to avoid link warnings on them, the simplest thing is to just add a stub for each in src/library.js. That also avoids changes to musl or to how we build musl.

@aidanhs
Copy link
Contributor

aidanhs commented Sep 25, 2015

Just a note on the changes as they stand, I suspect the return value of system should be -1 (to indicate a fork failure, rather than an exit code of 1) and errno in both cases should be ENOSYS i.e. passed upwards from a fork call, due to fork() is not supported on this platform, which seems appropriate?

@juj
Copy link
Collaborator Author

juj commented Sep 28, 2015

@aidanhs : musl returns 1 there on failure, see http://git.musl-libc.org/cgit/musl/tree/src/process/system.c#n22 .

I'll try to work this to build the process subdirectory instead and rely on the syscalls.

@juj
Copy link
Collaborator Author

juj commented Sep 28, 2015

Ok, redid the pull request and added the whole process subdirectory to build. How does this look @kripken ?

@aidanhs
Copy link
Contributor

aidanhs commented Sep 28, 2015

Regarding the 1, that's apparently a special case of calling system:

If the value of command is NULL, system() returns nonzero if the shell is available, and zero if not.

So it looks like musl is being a bit naughty by assuming a shell is available.

@kripken
Copy link
Member

kripken commented Sep 28, 2015

Looks fine. However, I just want to repeat my other proposal: if we implement these methods in src/library.js, it's about the same amount of code, but

  1. Building libc will be a little faster since we don't add process.
  2. Building programs using libc will be a little faster since we don't link in more code that almost always is dce'd away.

However, this solution is more future proof, in that we could eventually implement those syscalls perhaps.

I'm ok with either way, just want to make sure we consider the downsides of this approach.

@juj juj closed this Nov 3, 2015
@juj juj force-pushed the no_process_spawn branch from 97909a0 to 1de1298 Compare November 3, 2015 20:44
…p, which may be called by musl code that we do build.
@juj juj reopened this Nov 3, 2015
@juj
Copy link
Collaborator Author

juj commented Nov 3, 2015

Actually, I agree with the src/library.js approach. Implemented it now, and it is just tons simpler. How does this look?

@kripken
Copy link
Member

kripken commented Nov 3, 2015

Great, lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants