-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add Pty.TerminalModes #131
base: master
Are you sure you want to change the base?
Conversation
Co-Authored-By: Andrey Petrov <shazow@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable, but I've never used this feature so a sanity check on ergonomics from someone else would be great. :)
This looks like a really solid starting point. Unless I'm misreading something though, this doesn't allow for changing terminal modes at runtime. As an example, things like disabling ECHO and ICANON to enter raw mode can be changed to allow for password entry without displaying the characters. I'm not 100% sure how to solve this, especially with an ergonomic API, so I'd be open to any suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this looks like a great first step - also, from reading the spec, I believe the only terminal modes that can be sent at runtime are xon/xoff (https://tools.ietf.org/html/rfc4254#section-6.8) which are handled in a separate request and should therefore be fine (since we'd implicitly deny them).
I left a number of comments, though most of them are stylistic changes.
ssh.go
Outdated
Term string | ||
Window Window | ||
// HELP WANTED: terminal modes! | ||
Term string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it needs a gofmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I forgot it. I will modify this.
}{} | ||
|
||
TerminalModes := make(ssh.TerminalModes, 0) | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description on how terminal modes are actually passed can be found here: https://tools.ietf.org/html/rfc4254#section-8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand what you mean. Do you mean to set terminal modes to []byte
type?
util.go
Outdated
} | ||
return | ||
return pty, true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are already implicitly returned because of the named params. Please either switch back to the implicit return or remove the variable names from the function definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
util.go
Outdated
}, | ||
TerminalModes: TerminalModes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be enough for now, but we may want to have an example or a convenience function that will handle this properly. Setting TerminalModes is not fun. This is just an example: https://github.com/golang/crypto/blob/master/ssh/terminal/util.go#L37
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean like this ?
func ApplyTerminalModes(fd int, terminalModes ssh.TerminalModes) bool {
termios, err := unix.IoctlGetTermios(fd, unix.TCGETS)
fmt.Println(termios, err)
for opcode, val := range terminalModes {
switch opcode {
case ssh.TTY_OP_ISPEED:
termios.TTY_OP_OSPEED = val
case ssh.TTY_OP_OSPEED:
termios.TTY_OP_OSPEED = val
// case char
case ssh.VINTR:
termios.Cc[ssh.VINTR] = val
case ssh.VQUIT:
termios.Cc[ssh.VQUIT] = val
//...
//case mode
case ssh.ECHO:
if val {
termios.Lflag |= ssh.ECHO
} else {
termios.Lflag &= ~ssh.ECHO
}
default
// Others modes ...
}
}
if err := unix.IoctlSetTermios(fd, unix.TCSETS, termios); err != nil {
return nil, err
}
return true
}
This is the reference code https://github.com/openssh/openssh-portable/blob/1a7217ac063e48cf0082895aeee81ed2b8a57191/ttymodes.c#L397, the syntax may not be correct.
util.go
Outdated
|
||
TerminalModes := make(ssh.TerminalModes, 0) | ||
for { | ||
if len(modes) < 1 || modes[0] == ttyOPEND || len(modes) < 5 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually handles 2 different conditions - I'd argue len(modes) < 1 || modes[0] == ttyOPEND
are success, but len(modes) < 5
is a failure.
One other note: I'm not sure what https://tools.ietf.org/html/rfc4254#section-8 implies about opcodes 160-255. I think it means that when reading in the opcodes, and you get 160-255, it should end processing. It also may imply that items should be read in item by item... since I don't know if clients are required to send a Val with opcodes 160-255. |
Thanks for reviewing my code. My English may not be very good. I'm actually not too familiar with terminal modes. I want to pass it to my next |
No description provided.