nixos/iperf: Init the module #44134
nixos/iperf: Init the module #44134
Conversation
|
it might be best to name it iperf3 to remove the confusion on version number. There should be an option to pass arbitrary flags too. |
|
@teto Rebased and fixed |
| description = "Path to the configuration file containing authorized users credentials to run iperf tests"; | ||
| }; | ||
| extraFlags = mkOption { | ||
| type = types.listOf types.string; |
There was a problem hiding this comment.
Use types.str instead of types.string in all of the options.
| }; | ||
|
|
||
| script = '' | ||
| exec ${pkgs.iperf3}/bin/iperf \ |
There was a problem hiding this comment.
I think you can put a multiline command invocation into serviceConfig.ExecStart, so no need to use script + exec here
| after = [ "network.target" ]; | ||
|
|
||
| serviceConfig = { | ||
| Type = "simple"; |
| script = '' | ||
| exec ${pkgs.iperf3}/bin/iperf \ | ||
| --server \ | ||
| --pidfile /run/iperf3/iperf.pid \ |
There was a problem hiding this comment.
It is preferred to not use PID files if not necessary, which is probably the case here, because iperf runs in the foreground by default, which is good.
| RuntimeDirectory = "iperf3"; | ||
| PIDFile = "/run/iperf3/iperf.pid"; | ||
| PrivateDevices = true; | ||
| CapabilityBoundingSet = ""; |
There was a problem hiding this comment.
What does an empty string do here?
There was a problem hiding this comment.
From systemd.exec(5):
If this option is not used, the capability bounding set is not modified on process execution, hence no limits on the capabilities of the process are enforced.
and:
If the empty string is assigned to this option, the bounding set is reset to the empty capability set, and all prior settings have no effect.
| extraFlags = mkOption { | ||
| type = types.listOf types.string; | ||
| default = [ ]; | ||
| description = "Extra flags to pass to iperf3(1)"; |
There was a problem hiding this comment.
All the descriptions should end with a colon.
| description = "Emit debugging output"; | ||
| }; | ||
| rsaPrivateKey = mkOption { | ||
| type = types.nullOr types.string; |
There was a problem hiding this comment.
You can make the type types.path here, which forces it to be an absolute one (starting with "/"). Same for authorizedUsersFile
| affinity = mkOption { | ||
| type = types.nullOr types.string; | ||
| default = null; | ||
| description = "CPU affinity for the process"; |
There was a problem hiding this comment.
You could add an example = "4,5"; here and explain the syntax in short in the description.
There was a problem hiding this comment.
The n,m syntax is only valid for the client, so I guess this wouldn't make too much sense.
| ${optionalString cfg.verbose "--verbose"} \ | ||
| ${optionalString cfg.debug "--debug"} \ | ||
| ${optionalString cfg.forceFlush "--forceflush"} \ | ||
| ${toString cfg.extraFlags} |
There was a problem hiding this comment.
Use lib.escapeShellArgs cfg.extraFlags here, which handles escaping correctly.
| api = { | ||
| enable = mkEnableOption "iperf3 network throughput testing server"; | ||
| port = mkOption { | ||
| type = types.int; |
There was a problem hiding this comment.
You could use the newish types.ints.u16 to prevent people trying to assign a port outside of the valid range (have seen people do this)
|
@infinisil Fixed and rebased |
| description = "Emit debugging output."; | ||
| }; | ||
| rsaPrivateKey = mkOption { | ||
| type = types.nullOr types.str; |
| description = "Server port to listen on for iperf3 client requsts."; | ||
| }; | ||
| affinity = mkOption { | ||
| type = types.nullOr types.str; |
There was a problem hiding this comment.
Ah, so yeah the n/m syntax doesn't make sense for the server, so only the n syntax does, which means the only valid values are integers, so I think this option should be adjusted to reflect that (and make the correct conversion with toString in the exec script).
| description = "Path to the configuration file containing authorized users credentials to run iperf tests."; | ||
| }; | ||
| extraFlags = mkOption { | ||
| type = types.listOf types.path; |
| }; | ||
| in { | ||
| options.services.iperf3 = api; | ||
| config = lib.mkIf cfg.enable imp; |
|
@infinisil Fixed and rebased |
|
Although, one thing I'm not so sure about: Is it really a good idea have this under the Because the default iperf version is 3 already. I'm not sure about @teto's reasoning, but I'd be in favor of using |
|
I personally prefer |
|
So what's the matter with the name @teto @infinisil ? |
|
I get bitten by the "man iperf" that doesn't work after a nix-shell -p iperf (it is man iperf3). Also with iperf you don't know which one you are running (flags are similar) while the codebases are completely different. And it seems iperf2 development resumed. As I work with both I tend to appreciate an accurate naming but don't mind me. The PR has been there long enough. In worst case, I can alias it ;) |
|
@teto I rebased the PR to master again. Also, I changed the filename to |
|
Aahh, Thank you! |
Motivation for this change
I'd like to have a NixOS module for iperf3, because it's supposed to run on my router as a daemon.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)