Hi Clément !
Thanks for the review. I agree with all
your points, I'll publish a v2 but on debbugs this time !
Thank you,
Mathieu
> Here are a few comments:
>
> [...]
>
>> +(define-module (gnu services pm)
>> + #:use-module (guix gexp)
>> + #:use-module (guix packages)
>> + #:use-module (guix records)
>> + #:use-module (gnu packages linux)
>> + #:use-module (gnu services)
>> + #:use-module (gnu services base)
>> + #:use-module (gnu services configuration)
>> + #:use-module (gnu services shepherd)
>> + #:use-module (gnu system shadow)
>> + #:export (tlp-service-type
>> + tlp-configuration
>> + generate-tlp-documentation))
>
> I don't think generate-tlp-documentation needs to be exported.
>
>> +(define (uglify-field-name field-name)
>> + (let ((str (symbol->string field-name)))
>> + (string-join (string-split
>> + (string-upcase
>> + (if (string-suffix? "?" str)
>> + (substring str 0 (1- (string-length str)))
>> + str))
>> + #\-)
>> + "_")))
>> +
>> +(define (serialize-field field-name val)
>> + (format #t "~a=~a\n" (uglify-field-name field-name) val))
>> +
>> +(define (serialize-boolean field-name val)
>> + (serialize-field field-name (if val "1" "0")))
>> +(define-maybe boolean)
>> +
>> +(define (serialize-string field-name val)
>> + (serialize-field field-name val))
>> +(define-maybe string)
>> +
>> +(define (space-separated-string-list? val)
>> + (and (list? val)
>> + (and-map (lambda (x)
>> + (and (string? x) (not (string-index x #\space))))
>> + val)))
>> +(define (serialize-space-separated-string-list field-name val)
>> + (serialize-field field-name
>> + (format #f "~s"
>> + (string-join val " "))))
>> +(define-maybe space-separated-string-list)
>> +
>> +(define (non-negative-integer? val)
>> + (and (exact-integer? val) (not (negative? val))))
>> +(define (serialize-non-negative-integer field-name val)
>> + (serialize-field field-name val))
>> +(define-maybe non-negative-integer)
>> +
>> +(define (on-off-boolean? val)
>> + (boolean? val))
>> +(define (serialize-on-off-boolean field-name val)
>> + (serialize-field field-name (if val "on" "off")))
>> +(define-maybe on-off-boolean)
>> +
>> +(define (y-n-boolean? val)
>> + (boolean? val))
>> +(define (serialize-y-n-boolean field-name val)
>> + (serialize-field field-name (if val "Y" "N")))
>> +
>> +(define-configuration tlp-configuration
>> + (tlp
>> + (package tlp)
>> + "The TLP package.")
>> +
>> + (tlp-enable?
>> + (boolean #t)
>> + "Set to true if you wish to enable TLP.")
>
> From this point, the indentation is broken.
>
>> + (tlp-default-mode
>> + (string "AC")
>> + "Default mode when no power supply can be detected. Alternatives are
>> +AC and BAT.")
>> +
>> + (disk-idle-secs-on-ac
>> + (non-negative-integer 0)
>> + "Number of seconds Linux kernel has to wait after the disk goes idle,
>> +before syncing on AC.")
>> +
>> + (disk-idle-secs-on-bat
>> + (non-negative-integer 2)
>> + "Same as @code{disk-idle-ac} but on BAT mode.")
>> +
>> + (max-lost-work-secs-on-ac
>> + (non-negative-integer 15)
>> + "Dirty pages flushing periodicity, expressed in seconds.")
>> +
>> + (max-lost-work-secs-on-bat
>> + (non-negative-integer 60)
>> + "Same as @code{max-lost-work-secs-on-ac} but on BAT mode.")
>> +
>> + (cpu-scaling-governor-on-ac
>> + (maybe-space-separated-string-list 'disabled)
>> + "CPU frequency scaling governor on AC mode. With intel_pstate
>> +driver, alternatives are powersave and performance. With acpi-cpufreq
>> driver,
>> +alternatives are ondemand, powersave, performance and conservative.")
>
> Please, could you put two spaces between phrases? :) As in "and
> performance. With...". I've seen this in other parts of your patch as
> well.
>
> [...]
>
>> +(define (tlp-shepherd-service config)
>> + (let* ((tlp-bin (file-append
>> + (tlp-configuration-tlp config) "/bin/tlp"))
>> + (tlp-action (lambda args
>> + #~(lambda _
>> + (zero? (system* #$tlp-bin #$@args))))))
>> + (list (shepherd-service
>> + (documentation "Run TLP script.")
>> + (provision '(tlp))
>> + (requirement '(syslogd user-processes))
>
> I have not been able to see anything related to tlp in
> /var/log/messages. And if I set trace mode (TLP_DEBUG, in /etc/tlp), I
> have an error message: "logger: unknown facility name: debug", which I
> think could be patched in tlp source, maybe by removing "-p debug". Or
> maybe it is logger that needs to be patched, I don't know.
>
> Otherwise syslogd would not be needed here. WDYT?
>
> BTW, could you consider adding TLP_DEBUG to the service?
>
>> + (start (tlp-action "init" "start"))
>> + (stop (tlp-action "init" "stop"))))))
>> +
>> +(define (tlp-activation config)
>> + (let* ((config-dir "/etc")
>> + (config-str (with-output-to-string
>> + (lambda ()
>> + (serialize-configuration
>> + config
>> + tlp-configuration-fields))))
>> + (config-file (plain-file "tlp" config-str)))
>> + #~(begin
>> + (use-modules (guix build utils))
>> + (mkdir-p #$config-dir)
>> + (copy-file #$config-file (string-append #$config-dir "/tlp")))))
>
> Here I think it is better to wrap the gexp in (with-imported-modules
> '((guix build utils)) ...), as in the CUPS service for example, even
> though I don't understand fully the consequences of not doing it.
>
> [...]
>
> Thanks for this :)
> Clément
