Add SSHMaster::Connection::trySetBufferSize#10765
Conversation
|
Oh wat, segfault in CI in ambient Nix (i.e. nothing to do with this PR) |
Is that on Darwin? Can you get a stack trace for it? |
|
We didn't post here, but the fcntl calls are wrong. Setter takes an |
Ericson2314
force-pushed
the
ssh-pipe-size-method
branch
from
bbeb2fc to
05be135
Compare
|
Nit: why design the interface with EDIT: especially note that the (current) implementation silently overflows to non-sensical values. |
|
@vcunat ah I could assert, or that + change type to |
|
Could you add a test? We can't just rely on types here, unfortunately. |
|
Accepting negative numbers? Well, if you do an But perhaps more generally: the function returns the resulting buffer size (or negative error code), so perhaps it would make sense to check the return value? Overriding buffer size surely won't be a very important error, but perhaps some debug-level logs in case the value differs from what was passed as desired size? (that could even cover these negative cases by itself) EDIT: I don't know the customs of Nix codebase, and I only really use pure C, so... take my comments with a grain of salt. |
It is unused in Nix currently, but will be used in Hydra. This reflects what Hydra does in NixOS/hydra#1387. We may probably to use it more widely for better SSH store performance, but this needs to be subject to more testing before we do that.
Ericson2314
force-pushed
the
ssh-pipe-size-method
branch
from
05be135 to
0d25cc6
Compare
|
Yes, IMO as, one needs to |
Exposed for Hydra. We could make it fancier but with (a) new store settings (b) switch to `ssh-ng://` both in the works, it doesn't seem worth it.
github-actions
bot
added
the
store
| fcntl(in.get(), F_SETPIPE_SZ, pipesize); | ||
| fcntl(out.get(), F_SETPIPE_SZ, pipesize); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
If any errors occur, might they be worth logging?
There was a problem hiding this comment.
I think I'll do this in the future --- e.g. if we're just going to ssh-ng:// anyways this is temporary.
…0765 Add `SSHMaster::Connection::trySetBufferSize` (backport #10765)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
4 participants
Motivation
It is unused in Nix currently, but will be used in Hydra. This reflects what Hydra does in NixOS/hydra#1387.
Context
We may probably to use it more widely for better SSH store performance, but this needs to be subject to more testing before we do that.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.