nix icon indicating copy to clipboard operation
nix copied to clipboard

Add `SSHMaster::Connection::trySetBufferSize`

Open Ericson2314 opened this issue 1 year ago • 7 comments

Motivation

It is unused in Nix currently, but will be used in Hydra. This reflects what Hydra does in https://github.com/NixOS/hydra/pull/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 :+1: to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 avatar May 23 '24 15:05 Ericson2314

Oh wat, segfault in CI in ambient Nix (i.e. nothing to do with this PR)

/Users/runner/work/_temp/8e2e66c1-0beb-427b-912b-9e63f8be4be1.sh: line 1:  4963 Segmentation fault: 11  nix --experimental-features 'nix-command flakes' flake check -L

Ericson2314 avatar May 23 '24 16:05 Ericson2314

segfault in CI in ambient Nix (i.e. nothing to do with this PR)

/Users/runner/work/_temp/8e2e66c1-0beb-427b-912b-9e63f8be4be1.sh: line 1:  4963 Segmentation fault: 11  nix --experimental-features 'nix-command flakes' flake check -L

Is that on Darwin? Can you get a stack trace for it?

roberth avatar May 24 '24 06:05 roberth

We didn't post here, but the fcntl calls are wrong. Setter takes an int, not a pointer. And getter takes nothing, returns value. EDIT: that's how man fcntl.2 reads to me, at least.

vcunat avatar May 24 '24 06:05 vcunat

Nit: why design the interface with size_t when we're only able to implement int?

EDIT: especially note that the (current) implementation silently overflows to non-sensical values.

vcunat avatar May 26 '24 17:05 vcunat

@vcunat ah I could assert, or that + change type to unsigned int. I just didn't want to accept negative numbers.

Ericson2314 avatar May 27 '24 04:05 Ericson2314

Could you add a test? We can't just rely on types here, unfortunately. You could detect the buffer size by filling a pipe in non blocking mode, without reading it.

roberth avatar May 27 '24 06:05 roberth

Accepting negative numbers? Well, if you do an unsigned int or size_t and pass a too large value, you can end up passing a negative number to the syscall. So if you don't have code somehow guarding that, I'd say it's better to make the real type visible in the API already.

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.

vcunat avatar May 27 '24 08:05 vcunat

Yes, IMO as, one needs to assert either way to check it is within [0, INT_MAX] neither int nor size_t is better than the other. I went with size_t because I think it is more evocative and suitable for a portable interface --- someday, this should be implemented on platforms other than Linux.

Ericson2314 avatar Feb 17 '25 00:02 Ericson2314

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2025-02-17-meeting-minutes-213-214/60813/1

nixos-discourse avatar Feb 24 '25 13:02 nixos-discourse