search

openssh-rust / openssh

Scriptable SSH through OpenSSH in Rust
Apache License 2.0
232 stars 35 forks source link

Make `-o ControlPersist` configurable #146

Closed krfricke closed 4 months ago

krfricke commented 4 months ago

The crate uses ControlPersist=yes per default. In practice we see the control processes around forever, and they are not reused across relaunches of the binary. So instead it would be nice to set the ControlPersist option to a custom value, so they can expire automatically after e.g. 5 minutes.

jonhoo commented 4 months ago

This change is Reviewable

NobodyXu commented 4 months ago

Thank you for your PR!

I have one change requested and once you resolve it, I will merge this PR and cut a new release.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.99%. Comparing base (bacc76d) to head (c95f279). Report is 2 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files | [Files](https://app.codecov.io/gh/openssh-rust/openssh/pull/146?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/builder.rs](https://app.codecov.io/gh/openssh-rust/openssh/pull/146?src=pr&el=tree&filepath=src%2Fbuilder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2J1aWxkZXIucnM=) | `74.91% <50.00%> (-1.34%)` | :arrow_down: | ... and [13 files with indirect coverage changes](https://app.codecov.io/gh/openssh-rust/openssh/pull/146/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
krfricke commented 4 months ago

Thanks for the quick review. It's a good point, I've updated the PR. I've called the enum ControlPersist, in line with the KnownHosts struct. I also use a simple conversion which just gives a string in seconds (e.g. 5 minutes will be 300s, and not 5m). Let me know if you prefer we run some maths there to give a better human readable output.

krfricke commented 4 months ago

Updated - please review my usage of Cow. I believe with deref() it should avoid allocations in the non-IdleFor cases.

krfricke commented 4 months ago

All updated and moved to NonZeroUsize. Thanks for your help