[webkit-reviews] review denied: [Bug 237031] [JSC] Set ssh keepalive in run-jsc-stress-tests : [Attachment 452852] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Angelos Oikonomopoulos
<angelos at igalia.com>'s request for review:
Bug 237031: [JSC] Set ssh keepalive in run-jsc-stress-tests
https://bugs.webkit.org/show_bug.cgi?id=237031

Attachment 452852: Patch

https://bugs.webkit.org/attachment.cgi?id=452852&action=review




--- Comment #3 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 452852
  --> https://bugs.webkit.org/attachment.cgi?id=452852
Patch

The new option added and moving the always used SSH options into a common
array LGTM, but there is one cleanup I would like to see done as part of
this patch =)

View in context: https://bugs.webkit.org/attachment.cgi?id=452852&action=review

> Tools/Scripts/run-jsc-stress-tests:2560
> +    IO.popen("ssh #{SSH_OPTIONS_DEFAULT.join(" ")} -p #{remoteHost.port}" +
(remoteHost.identity_file_path ? " -i #{remoteHost.identity_file_path}" : "") +
" #{remoteHost.user}@#{remoteHost.host} '#{cmd}'", "r") {

While doing modifications here, could we change this to pass the array of
command+arguments to IO.popen instead of using a command string that will
be passed through the shell? The “ssh” invocation itself does not need to
go through the local shell — the remote command (“cmd” here) will still
run in a shell remotely because that's how SSH behaves.