Page MenuHomePhabricator

[Patch] for terminal modes
Needs ReviewPublic

Authored by Xiaomao on Aug 17 2017, 10:00 AM.

Details

Reviewers
asn
aris
Summary

libssh lacks terminal modes support, the OpenSSH sends tty modes to remote side, while libssh does not.

Without terminal modes, some applications (e.g. Karaf OSGi container) will sends incorrect control sequences perversely (e.g. client requires CRLF, but sends just LF).

It's very easy to reproduce the problem by using our Mac app -- SSH Shell, please see attached screenshots for before/after patch applied.

This patch based on master branch, we added a new function ssh_channel_request_pty_size_modes for sending tty modes. Compare to ssh_channel_request_pty_size, this function takes an extra parameter with type struct termios, and convert it into terminal modes data that remote side could parse.

The patch was tested under MacOS, but it should also work in other platform. The function would be disabled if the platform does not have termios terminology.

Note: we only implement terminal modes in client-side, the server-side still missing.

  • reference **
Test Plan

Test with our Mac app -- SSH Shell, please see attached screenshots for before/after patch applied.

before

after

Diff Detail

Repository
rLIBSSH libssh
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Xiaomao created this revision.Aug 17 2017, 10:00 AM
asn added a reviewer: aris.Aug 17 2017, 2:59 PM
asn edited edge metadata.

I do not like the void pointer for ssh_termios. I think we should define a structure here. Aris what is your opinion?

Xiaomao changed the visibility from "All Users" to "Public (No Login Required)".Aug 20 2017, 7:53 AM
Xiaomao updated this revision to Diff 26.Sep 13 2017, 8:41 AM

Add termios.c_cc/termios.c_ispeed/termios.c_ospeed support. (previous version only support termios flag)

[bugfix]

  • add missing flag IMAXBEL
  • correct c_oflag to c_cflag on set flag
aris edited edge metadata.Nov 26 2017, 6:26 PM

I'm very new with phabricator, apologies if I'm using it incorrectly.
I have two questions:

  1. how is the user supposed to call it ? Is it using the system's termios structure? Then I see nothing wrong in using "struct termios" directly in the prototype of the function. Even if struct termios isn't defined on the user's system, they can still use it as an incomplete type.
  2. Is all that work (especially ttymodes.c) your own work or was it inspired from somewhere else (I remember seeing something similar in OpenSSH). Not to dismiss it, I just want to be 100% in the clear here.

Not a question, but we should have a small testcase added that tries to set a very basic terminal on the remote side. It's hard to test for correctness, but at least we know it doesn't crash on trivial inputs. It's also useful as a small example of how to use that new API.
Could you also check that no unrelated formating change appears in the diff, like in channels.c:792

Thanks!