search

saleyn / erlexec

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

Do not fail if a pty_opt is not implemented on exec:run #163

Closed SteffenDE closed 2 years ago

SteffenDE commented 2 years ago

This PR addresses multiple issues.

First it adds a separate pty opt validation to the C++ side separate from the set_pty_opt function. Previously it could happen that a client sends an option that is not implemented in the system, as described in https://github.com/saleyn/erlexec/pull/162. One example is olcuc on macOS. This now only fails when dynamically setting the option using exec:pty_opts but not on exec:run.

Second, we try to set the pty speed on systems that do not have pre-defined baud constants.

Finally, it should fix #160 - the pty tests did not expect the two test\r\n strings to arrive in a single message.

What do you think? If you want I can also split this PR.

saleyn commented 2 years ago

It seems that the option validation is better to be done on the Erlang side rather than in C, since sending the invalid command option to the C port driver and back is more costly than checking in Erlang.

SteffenDE commented 2 years ago

Hmm yes, I can move this to the check_cmd_options function. Do you know a nice way to not duplicate the values in the type definition and the validation code?

saleyn commented 2 years ago

Do you know a nice way to not duplicate the values in the type definition and the validation code?

No easy way, though possible by reading source code's AST tree, and parsing types. :( Not applicable this project.

saleyn commented 2 years ago

Also note that the validation is "almost" implemented in C++ by the set_pty_opt function when you pass nullptr as the first arg.

SteffenDE commented 2 years ago

Also note that the validation is "almost" implemented in C++ by the set_pty_opt function when you pass nullptr as the first arg.

Yes, I saw that, but I want to prevent the cases where an option is not defined on a system (thus no entry in the set_pty_opts function because of the ifdefs) and therefore failing. One could also just skip this check https://github.com/saleyn/erlexec/blob/e64d794b504a507038d10a44d5a2e9f1a4df638d/c_src/exec_impl.cpp#L1417 and only validate when using the exec:pty_opts function, but I do like the idea of catching completely invalid options.