Skip to content

Comments

Add partial support for VERASE and VWERASE in canonical mode in PTYs#9950

Merged
copybara-service[bot] merged 1 commit intogoogle:masterfrom
thecodingwizard:master
Feb 9, 2024
Merged

Add partial support for VERASE and VWERASE in canonical mode in PTYs#9950
copybara-service[bot] merged 1 commit intogoogle:masterfrom
thecodingwizard:master

Conversation

@thecodingwizard
Copy link
Contributor

@thecodingwizard thecodingwizard commented Feb 1, 2024

This PR partially fixes #9949 by porting over some code from the Linux kernel's n_tty_receive_char_canon, specifically the eraser function.

It adds partial support for VERASE (backspace) and VWERASE (word erase with ctrl+w).

@ayushr2
Copy link
Collaborator

ayushr2 commented Feb 1, 2024

Thanks for all the PRs! As with the other PRs, a test would be appreciated. For example, recently I checked in a PTY fix along with a syscall test in f82d97c.

You can run the syscall test against Linux with: make test TARGETS=//test/syscalls:pty_test_native. Then to test against runsc, run make test TARGETS=//test/syscalls:pty_test_runsc_systrap_directfs.

@ayushr2
Copy link
Collaborator

ayushr2 commented Feb 1, 2024

Could you also squash your commits in each PR? When copybara merges this, it applies all the commits onto master (without squashing them).

@thecodingwizard
Copy link
Contributor Author

Will do. Thanks for providing instructions on how to run the syscall tests!

@ayushr2
Copy link
Collaborator

ayushr2 commented Feb 1, 2024

Thanks for providing instructions on how to run the syscall tests!

NP. Running the syscall test directly with bazel may (annoyingly) fail. We have this entire Makefile business going on, in which all bazel commands are run via a bazel Docker container (implemented in tools/bazel.mk).

Copy link
Collaborator

@kevinGC kevinGC left a comment

Choose a reason for hiding this comment

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

Thanks again for another PR. More comments this time!

Also, we should probably have a simple test for each of VERASE and VWERASE that check that inputing backspace or CTRL+W will have the desired effect.

@thecodingwizard thecodingwizard force-pushed the master branch 4 times, most recently from 93ebd87 to a521ac5 Compare February 2, 2024 04:04
@thecodingwizard
Copy link
Contributor Author

I've added some tests, fixed a bug (break should have been continue), refactored some code, and left more comments. Please note that I'm not confident that my comments are actually correct: I think they are, but I do not have extensive experience with terminals.

Thank you for all your feedback -- I don't actually know Golang so your style suggestions, etc. are very helpful! Please let me know if there's anything else I should change or clarify.

Copy link
Collaborator

@kevinGC kevinGC left a comment

Choose a reason for hiding this comment

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

Good changes, just a few more things.

@thecodingwizard thecodingwizard force-pushed the master branch 4 times, most recently from 6c4deba to 5813adc Compare February 3, 2024 00:25
@thecodingwizard
Copy link
Contributor Author

thecodingwizard commented Feb 3, 2024

Thanks again for your feedback! I:

  • Fixed the isContinuationByte bug -- thanks for catching that :)
  • Reverted isCtrl check to use toErase < 0x20 -- it was failing tests with unicode.IsControl; I'm not sure if the two are equivalent. In particular, unicode.IsControl(rune(0x80)) seems to return true.
  • Rewrote a comment.
  • Used std::string instead of variable length arrays; let me know if you prefer std::vector.

Apologies, I must not have run tests properly last night; they should have caught the first two issues.

@kevinGC
Copy link
Collaborator

kevinGC commented Feb 6, 2024

LGTM and thanks for the PR.

@copybara-service copybara-service bot merged commit 3a73915 into google:master Feb 9, 2024
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.

Backspace doesn't work when reading input in canonical mode from a PTY

6 participants