Skip to content
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

Test that file descriptor state is clean (Overhaul file descriptor handling) #223

Open
1 of 2 tasks
andychu opened this issue Jan 26, 2019 · 4 comments
Open
1 of 2 tasks

Comments

@andychu
Copy link
Contributor

andychu commented Jan 26, 2019

While investigating a bug that turned out to be related to issue #202, I found two things

  • The child process file descriptor state isn't clean. See test/spec.sh command_. bash and dash do one thing, mksh and zsh do another (they have /dev/tty open), but OSH is even worse. There are pipes open for some reason.
  • Instead of my own linear search in _GetFreeDescriptor() in core/process.py, I should be using fcntl(..., F_DUPFD, ...) and looking at the return value, like dash does.
@andychu
Copy link
Contributor Author

andychu commented Jul 24, 2019

NOTE: I just added a spec test case that I'm confused by.

OSH doesn't handle assignment builtins differently than other builtins. But it agrees a lot with zsh, but disagrees with the other shells.

Is this an issue of "change fd state state then fork" vs. "fork then change fd state" ?? Not sure why it would be different.

Maybe check smoosh?

$ test/spec.sh assign --range 37
assign.test.sh
case    line    dash    bash    mksh    zsh     osh
 37     589     ok      ok      ok      pass    pass    redirect after assignment builtin (what's going on with dash/bash/mksh here?)
2 passed, 3 ok, 0 known unimplemented, 0 known bugs, 0 failed, 39 skipped

andychu pushed a commit that referenced this issue Jul 24, 2019
@andychu
Copy link
Contributor Author

andychu commented Jul 24, 2019

Yeah there is a bug. Two things to do while fixing this:

  • Fix test cases 37 and 38 in spec/assign (related to redirects)
  • Test the file descriptor state (where did that go? I don't see it in spec/command_)
    • I guess this is demo/fd-state.sh

@andychu
Copy link
Contributor Author

andychu commented Mar 15, 2020

#653 mentions several file descriptor constructs not implemented

  • <>
  • 2>&1-
  • {fd}>

andychu pushed a commit that referenced this issue Mar 22, 2020
Feature:

- Enforce that FDs only have 2 digits, e.g. echo 99>&1 is allowed but
  echo 100>&1 isn't.  Wrote a test to show that all shells do this
  except bash.

  Addresses issue #674.

Refactoring that preserves the new move, close, and named FD features.
That is, 3>&1-  and  3>&-  and {fd}>

- Use exceptions for error handling within a redirect.  There are a
  bunch of failure cases scattered throughout, and this simplifies the
  code.
- Removed _PushMove in favor of _PushDup and some more code
- Removed duplicate code for >file.txt
- Get rid of _GetFreeDescriptor().  That was replaced by F_DUPFD, which
  tells the *kernel* to get a free descriptor (above a certain range).
- Script to strace shells (incomplete, but may be useful later.)
- Add more test cases.

All spec tests still pass.

This is most of #223, although there a few more things I'd like to clean
up.
@andychu andychu changed the title Overhaul file descriptor handling Test that file descriptor state is clean (Overhaul file descriptor handling) Mar 22, 2020
@andychu
Copy link
Contributor Author

andychu commented Mar 22, 2020

OK I did a big refactor that is most of this. We just need some more tests. I started some strace tests in demo/strace-redirects.sh

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

No branches or pull requests

1 participant