Skip to content

PtyProcess: add NO_CTTY flag #101

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

Closed
wants to merge 6 commits into from

Conversation

petreeftime
Copy link
Collaborator

The default behavior of posix_openpt seems to be to replace the controlling terminal for the calling process, and PtyProcess should only change it for child instead. I think this might be a reason why some of the tests were failing non-deterministacally sometimes, although I am not 100% confident in this fix.

Signed-off-by: Petre Eftime [email protected]

@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build succeeded:

@petreeftime
Copy link
Collaborator Author

@matthiasbeyer so far this passes 100% of the tests.

@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@petreeftime
Copy link
Collaborator Author

Ah. This failed as well.

@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build failed:

@petreeftime petreeftime marked this pull request as draft April 23, 2023 17:59
rust-cache@v1 seems to generate some deprecation warnings, so better
update to v2.

Signed-off-by: Petre Eftime <[email protected]>
The default behavior of posix_openpt seems to be to replace the
controlling terminal for the calling process, and PtyProcess should
only change it for child instead. I think this might be a reason why
some of the tests were failing non-deterministacally sometimes, although
I am not 100% confident in this fix.

Signed-off-by: Petre Eftime <[email protected]>
@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build failed:

Signed-off-by: Petre Eftime <[email protected]>
@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build succeeded:

@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build failed:

@petreeftime
Copy link
Collaborator Author

Signed-off-by: Petre Eftime <[email protected]>
@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build succeeded:

@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build failed:

Signed-off-by: Petre Eftime <[email protected]>
@petreeftime
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Apr 23, 2023
@bors
Copy link
Contributor

bors bot commented Apr 23, 2023

try

Build failed:

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the patch per se, if it helps. Only one annotation to simplify a bit of code here.

But as far as I can see, CI still fails? Or am I missing something?

Comment on lines +121 to +126
unsafe {
match ioctl(master_fd.as_raw_fd(), TIOCSCTTY) {
0 => Ok(()),
_ => Err(nix::Error::last()),
}?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the unsafe block is only needed for the ioctl call here, isn't it?
also, we don't need to match here, I think.

Suggested change
unsafe {
match ioctl(master_fd.as_raw_fd(), TIOCSCTTY) {
0 => Ok(()),
_ => Err(nix::Error::last()),
}?;
}
if 0 != unsafe { ioctl(master_fd.as_raw_fd(), TIOCSCTTY) } {
return Err(nix::Error::last())
}

@petreeftime
Copy link
Collaborator Author

Yes, so far I was reading through how pexpect or some other terminal emulators set this up and see if we're missing anything. So far, not luck with getting the issue fixed unfortunately, and no matter what I do I can't seem to be able to reproduce it on my setup.

@matthiasbeyer
Copy link
Member

Yeah that also was the case for me... it seems like we only have this issue in CI, which makes it even worse.

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.

2 participants