Re: [PATCH 3/5] gnu: Add tlp service.

[Mathieu Othacehe]

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 
>> address@hidden))))))
>> +    (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