New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/hddtemp: add support for HDD/SSD temperature montoring #111383
Conversation
pkgs/tools/misc/hddtemp/default.nix
Outdated
stdenv.mkDerivation rec { | ||
pname = "hddtemp"; | ||
version = "0.3_beta15"; | ||
|
||
src = fetchurl { | ||
url = "mirror://savannah/hddtemp/hddtemp-0.3-beta15.tar.bz2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing: You could also tie this to version
.
url = "mirror://savannah/hddtemp/hddtemp-0.3-beta15.tar.bz2"; | |
url = "mirror://savannah/hddtemp/hddtemp-${builtins.replaceStrings [ "_" ] [ "-" ] version}.tar.bz2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. There's actually no reason why the version should have an underscore to begin with, so I fixed that up instead.
@GrahamcOfBorg build hddtemp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from a few escaping improvements.
cp ${pkgs.hddtemp}/share/hddtemp/hddtemp.db $file | ||
${lib.concatMapStringsSep "\n" (e: "echo '${e}' >> $file") cfg.dbEntries} | ||
|
||
exec ${pkgs.hddtemp}/bin/hddtemp ${lib.concatStringsSep " " cfg.extraArgs} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using lib.escapeShellArgs
which would prevent users from accidentally messing up the command line. Idk if there may be a genuine use for variables or something here though.
exec ${pkgs.hddtemp}/bin/hddtemp ${lib.concatStringsSep " " cfg.extraArgs} \ | |
exec ${pkgs.hddtemp}/bin/hddtemp ${lib.escapeShellArgs cfg.extraArgs} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point regd escaping - all sorted
|
||
options = { | ||
hardware.sensor.hddtemp = { | ||
enable = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use mkEnableOption
here.
|
||
file=/var/lib/hddtemp/hddtemp.db | ||
|
||
drives=(${toString (map (e: ''$(realpath "${e}") '') cfg.drives)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drives=(${toString (map (e: ''$(realpath "${e}") '') cfg.drives)}) | |
drives=(${toString (map (e: ''$(realpath ${lib.escapeShellArg e}) '') cfg.drives)}) |
drives=(${toString (map (e: ''$(realpath "${e}") '') cfg.drives)}) | ||
|
||
cp ${pkgs.hddtemp}/share/hddtemp/hddtemp.db $file | ||
${lib.concatMapStringsSep "\n" (e: "echo '${e}' >> $file") cfg.dbEntries} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${lib.concatMapStringsSep "\n" (e: "echo '${e}' >> $file") cfg.dbEntries} | |
${lib.concatMapStringsSep "\n" (e: "echo ${lib.escapeShellArg e} >> $file") cfg.dbEntries} |
This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 1 package blacklisted:
3 packages built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation for this change
Basic module for temperature monitoring + some minor
hddtemp
cleanups while at it.The module does a few things:
by-path
as hddtemp chokes on colonsExecStart
.Ideally we lock this down properly with
DynamicUser = true;
but the problem is permissions needed to fetch SMART data which doesn't seem doable with the existing capabilities.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)