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

Fix EOF (^D) handling in devpts (partially addr #9333) #9336

Merged

Conversation

thundergolfer
Copy link
Contributor

This fixes the ctrl-d part of #9333.

Disclaimer: new to this codebase.

Copy link
Collaborator

@nlacasse nlacasse left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, and the nice test. One minor nit.

pkg/sentry/fsimpl/devpts/line_discipline.go Outdated Show resolved Hide resolved
@ayushr2
Copy link
Collaborator

ayushr2 commented Sep 5, 2023

Could you squash the two commits? Otherwise these will appear as two separate commits on the master branch after merge.

@thundergolfer
Copy link
Contributor Author

Oh sure, I thought I could squash just before merge. But I'll squash them now and force push.

@ayushr2
Copy link
Collaborator

ayushr2 commented Sep 5, 2023

We don't squash and merge from Github UI (unfortunately...). It needs be go through our internal system (via the "ready to pull" tag). Copybara will clone your PR as a new PR and merge that and close this. And in the process, it just pushes the commits from this branch onto master. So we request the commits are squashed (unless they are meant to be logically separated).

@thundergolfer
Copy link
Contributor Author

Make sense. Thanks for the explanation 👌

@nlacasse
Copy link
Collaborator

nlacasse commented Sep 5, 2023

This seems to break the PtyTest.TermiosONLCR test:

[ RUN      ] PtyTest.TermiosONLCR
test/syscalls/linux/pty.cc:466: Failure
Value of: ReadFd(fd.get(), &c, 1)
Expected: -1 (failure), with errno PosixError(errno=11 Resource temporarily unavailable)
  Actual: 0 (of type long)
[  FAILED  ] PtyTest.TermiosONLCR (5 ms)

Full logs here

@thundergolfer
Copy link
Contributor Author

thundergolfer commented Sep 5, 2023

Oh of course, this behavior is only valid in canonical mode, and that test disables the ICANON flag. Pushed a fix.

Edit: commit now passes make test TARGETS=//test/syscalls:pty_test_runsc_ptrace_shared

l.masterWaiter.Notify(waiter.ReadableEvents)
}
if !pushed && l.termios.LEnabled(linux.ICANON) {
Copy link
Collaborator

@nlacasse nlacasse Sep 5, 2023

Choose a reason for hiding this comment

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

We have to hold l.termiosMu to access l.termios.

I would set isCanon := l.termios.LEnabled(linux.ICANON) while the lock is held on line 200, and then reference isCanon in this conditional.

@copybara-service copybara-service bot merged commit 0230a37 into google:master Sep 6, 2023
3 checks passed
@nlacasse
Copy link
Collaborator

nlacasse commented Sep 6, 2023

This is merged! Thanks for the PR.

@thundergolfer thundergolfer deleted the jonathon/dev-pts-eof-handling branch September 7, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants