saleyn / erlexec

Execute and control OS processes from Erlang/OTP
https://hexdocs.pm/erlexec/readme.html
Other
525 stars 139 forks source link

fix tty_speed setting #162

Closed SteffenDE closed 2 years ago

SteffenDE commented 2 years ago

I came across this issue in my nerves_ssh_shell project:

** (FunctionClauseError) no function clause matching in anonymous fn/1 in NervesSSH.SystemShell.exec_command/2
    (nerves_ssh 0.4.0) lib/nerves_ssh/system_shell.ex:74: anonymous fn({:error, 'pty - invalid pty argument \'tty_op_ospeed\''}) in NervesSSH.SystemShell.exec_command/2

In there I currently just forward the options from the erlang ssh pty_ch_msg to erlexec. This does not work if an option is not supported by the system. In this case it's a bug. I added the __c_ispeed and __c_ospeed checks for musl based systems, but these are struct members and not defined as macros. This commit replaces the way tty speeds are handled to use the posix cfsetispeed and cfsetospeed functions.

Generally, I think that maybe there should be a graceful error on :exec.run when there's an error while setting a pty option. Because of all the ifdefs in ttymodes.hpp there could very well be the case that SSH requests a setting that the system does not support. Rather to fail hard, maybe it should only fail if all options failed to set? Otherwise the only option for me would be to set each option one by one after starting the process. Quoting from the termios man page:

Note that tcsetattr() returns success if any of the requested changes could be successfully carried out. Therefore, when making multiple changes it may be necessary to follow this call with a further call to tcgetattr() to check that all changes have been performed successfully.

Therefore, I think it's reasonable to only fail if there was no change at all.