ssh: Refactor disconnect handling to use context maps#11033
Conversation
CT Test Results 2 files 29 suites 25m 59s ⏱️ Results for commit 14ecf47. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
Replace positional arguments in send_disconnect/6,7 and disconnect/4 with a single map carrying code, details, module, line, and state_name. This simplifies call sites and makes the disconnect context extensible for future enrichment (e.g. event callbacks). - send_disconnect/6,7 -> send_disconnect/2,3 with map argument - disconnect/4 -> disconnect/1 with map argument - Rename macros to uppercase: ?send_disconnect -> ?SEND_DISCONNECT, ?DISCONNECT -> ?DISCONNECT_CONTEXT - Update ssh_dbg disconnect tracing to match new arities - Fix variable shadowing bug in terminate/3 where Reason was rebound to "Internal error" - Update all call sites across ssh_connection_handler, ssh_transport, ssh_fsm_kexinit, ssh_fsm_userauth_client, ssh_fsm_userauth_server
?CT_LOG automatically prepends pid, module, line, and function name to log output, making manual inclusion of ?MODULE and ?LINE redundant. - Replace all 54 ct:log calls with ?CT_LOG - Remove duplicate ?MODULE/?LINE/?FUNCTION_NAME/self() from format strings and argument lists where ?CT_LOG provides them - Replace deprecated standalone catch expressions with try/catch and add ?CT_LOG for exception details including stacktrace
u3s
force-pushed
the
kuba/ssh/disconnect-context-refactoring
branch
from
d6afff9 to
14ecf47
Compare
u3s
changed the title
Mikaka27
left a comment
Mikaka27
left a comment
There was a problem hiding this comment.
Do we want documentation for the ContextMap in this PR?
| disconnect_fun(DisconnectType, Details, D) -> | ||
| disconnect_fun(#{type => DisconnectType, details => Details, code => undefined}, D). | ||
|
|
||
| disconnect_fun(#{details := Details} = _DisconnectContext, D) -> | ||
| Fun = ?GET_OPT(disconnectfun, (D#data.ssh_params)#ssh.opts), | ||
| Fun(Details). |
There was a problem hiding this comment.
Previously we used ?CALL_FUN which wrapped a function execution in a 'catch', and now we call the function directly, are we ok with user's exception leaking from here?
|
ping @alexandrejbr |
Mikaka27
left a comment
Mikaka27
left a comment
There was a problem hiding this comment.
If we're fine with not catching disconnectfun's exception, looks good.
| lists:join(", ", element(W,Own)), | ||
| lists:join(", ", element(C,CounterPart))]}; | ||
| (_) -> | ||
| {"No common ~s algorithm", [Txt]} |
There was a problem hiding this comment.
I still think that this was a bad choice. It would have been much more reasonable to pick a maximum of X peer algorithms and process those and claim the rest was truncated.
How many peer algorithm exist today? I guess the biggest list is in KEX.
| %%%---------------------------------------------------------------- | ||
| %%disconnect_fun({disconnect,Msg}, D) -> ?CALL_FUN(disconnectfun,D)(Msg); | ||
| disconnect_fun(Reason, D) -> ?CALL_FUN(disconnectfun,D)(Reason). | ||
| disconnect_fun(DisconnectType, Details, D) -> |
There was a problem hiding this comment.
I see that you are bringing the disconnect type from #9157
Is that intentional, you still like the idea?
3 participants
Replace positional arguments in send_disconnect/6,7 and disconnect/4
with a single map carrying code, details, module, line, and
state_name. This simplifies call sites and makes the disconnect
context extensible for future enrichment (e.g. event callbacks).
?DISCONNECT -> ?DISCONNECT_CONTEXT
rebound to "Internal error"
ssh_transport, ssh_fsm_kexinit, ssh_fsm_userauth_client,
ssh_fsm_userauth_server