From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Othacehe Subject: Re: [PATCH 3/5] gnu: Add tlp service. Date: Thu, 16 Mar 2017 18:08:01 +0100 Message-ID: <87d1dh2lqm.fsf@gmail.com> References: <20170315204642.27626-1-m.othacehe@gmail.com> <20170315204642.27626-4-m.othacehe@gmail.com> <87d1dhjiiq.fsf@lassieur.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:52913) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1coYsk-0005Ie-Pj for guix-devel@gnu.org; Thu, 16 Mar 2017 13:08:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1coYsg-0002Pk-If for guix-devel@gnu.org; Thu, 16 Mar 2017 13:08:10 -0400 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]:36046) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1coYsg-0002PX-9i for guix-devel@gnu.org; Thu, 16 Mar 2017 13:08:06 -0400 Received: by mail-wm0-x233.google.com with SMTP id n11so117408552wma.1 for ; Thu, 16 Mar 2017 10:08:06 -0700 (PDT) In-reply-to: <87d1dhjiiq.fsf@lassieur.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: =?utf-8?Q?Cl=C3=A9ment?= Lassieur Cc: guix-devel@gnu.org 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