[bug#75528,2/2] services: Add power.
Commit Message
* gnu/services/power.scm: New file.
* gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
* doc/guix.texi (Power Management Services): Document service and data types.
Change-Id: If205d19bea1d20a99309626e28521a2d6fe6702f
---
doc/guix.texi | 374 +++++++++++++++++++++-
gnu/local.mk | 1 +
gnu/services/power.scm | 690 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 1062 insertions(+), 3 deletions(-)
create mode 100644 gnu/services/power.scm
Comments
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
> * gnu/services/power.scm: New file.
I see there's already a gnu/services/pm.scm file; it seems we don't need
another one? Or was it a conscious decision?
[...]
> +The @code{(gnu services power)} module provides a service definition for
> +@uref{http://www.apcupsd.org/, apcupsd}, a utility to interact with APC
> +UPSes. Apcupsd also works with some OEM-branded products manufactured
> +by APC.
I'd introduce a few @acronym here to help readers, e.g. for OEM and UPS.
> +@defvar apcupsd-service-type
> +The service type for apcupsd. For USB UPSes no configuration is
> +necessary, however tweaking some fields to better suit your needs might
> +be desirable. The defaults are taken from the upstream configuration
> +and they are not very conservative (@code{battery-level} of 5% is too
> +low in my opinion).
I'd say, (for example, the default @code{battery-level} of 5% may be
considered too low by some), to keep it neutral.
> +The default event handlers do send emails, read more in
> +@ref{apcupsd-event-handlers}.
> +
> +@lisp
> +(service apcupsd-service-type)
> +@end lisp
> +@end defvar
> +
> +@deftp {Data Type} apcupsd-configuration
> +
> +Available @code{apcupsd-configuration} fields are:
> +
> +@table @asis
> +@item @code{package} (default: @code{apcupsd}) (type: package)
> +Package to use.
That's more conventionally named with the same of the package,
e.g. 'acpupsd', not 'package'. I'd word the description as 'The
@code{apcupsd} package to use'.
> +@item @code{shepherd-base-name} (default: @code{apcupsd}) (type: symbol)
> +Base name of the shepherd service. You can add the service multiple
> +times with different prefix to manage multiple UPSes.
s/prefix/prefixes/
aaa
> +
> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)
> +Should the shepherd service auto-start?
> +
> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)
> +Path to the pid file.
All file names becoming /run/ prefixed instead of /var/run, based on a
previous comment.
We don't use path in GNU to denote file names, we use 'file names'.
That's mentioned in info '(standards) GNU Manuals':
Please do not use the term "pathname" that is used in Unix
documentation; use "file name" (two words) instead. We use the term
"path" only for search paths, which are lists of directory names.
> +
> +@item @code{debug-level} (default: @code{0}) (type: integer)
> +Logging verbosity. Bigger number means more logs. In the source code I
> +saw up to @code{300}, so for all logs, @code{999} seems reasonable.
I think it's best to not use first person pronoun in the manual to keep
it a bit more formal. So instead of 'I saw', this could be reworded to:
'The source code uses up to @code{300} as debug level value, so a value
of @code{999} seems reasonable to enable all logs.' or similar.
> +
> +@item @code{run-dir} (default: @code{"/var/run/apcupsd"}) (type: string)
> +Directory containing runtime information. You need to change this if
> +you desire to run multiple instances of the daemon.
> +
> +@item @code{name} (type: maybe-string)
> +Use this to give your UPS a name in log files and such. This is
> +particularly useful if you have multiple UPSes. This does not set the
> +EEPROM. It should be 8 characters or less.
> +
> +@item @code{cable} (default: @code{usb}) (type: enum-cable)
> +The type of cable connecting the UPS to your computer. Possible generic
> +choices are @code{'simple}, @code{'smart}, @code{'ether} and
> +@code{'usb}. Or a specific cable model number may be used:
nitpick: I'd reword to 'Alternatively, a specific [...]', which reads
better to me.
[...]
> +@item usb
> +A null string setting enables autodetection, which is the best choice
> +for most installations.
nitpick: My dictionary (aspell) prefers auto-detection.
[...]
> +@anchor{apcupsd-event-handlers}
> +@deftp {Data Type} apcupsd-event-handlers
> +
> +For description of the events please refer to the @command{apcupsd}'s
> +manual, which can be found in the @samp{apcupsd-doc} package.
I think you also meant it is in the @samp{doc} output, not in a
different package, right? In any case, I'd refer them to the man page
of apccontrol, which details the events. Also, to avoid the odd
rendering as "the ‘apcupsd’’s manual" in info, I'd instead rephrase to:
For a description of the events please refer to @samp{man 8 apccontrol}.
> +
> +Each handler shall be a gexp. It is spliced into the control script for
> +the daemon. In addition to the standard Guile programming environment,
> +these procedures and variables are also available.
s/these/the following/ [...] s/available./available:/
Since these are spliced in the control script and thus not at the top
level, users won't be able to import Guile modules they may want to use,
so it seems there should be a field to define which Guile modules should
be imported and used by default in the configuration.
> +
> +@table @code
> +@item conf
> +Variable containing path to the configuration file.
s/path/file name/, anywhere this appears.
> +
> +@item powerfail-file
> +Variable containing path to the powerfail file.
Ditto.
> +@item cmd
> +The event currently being handled.
> +
> +@item name
> +The name of the UPS as specified in the configuration file.
> +
> +@item connected
> +Is @code{"1"} if apcupsd is connected to the UPS via a serial port (or a
@command{apcupsd}
Also, any reason why these integer values are expressed as strings?
> +USB port). In most configurations, this will be the case. In the case
> +of a Slave machine where apcupsd is not directly connected to the UPS,
s/Slave/slave/ ? Could be nicer to use a neutral alternative like
'follower machine'.
> +this value will be @code{"0"}.
> +
> +@item powered
> +Is @code{"1"} if the computer on which @command{apcupsd} is running is
> +powered by the UPS and @code{"0"} if not. At the moment, this value is
> +unimplemented and always @code{"0"}.
> +
> +@item (err @var{fmt} @var{args...})
> +Wrapper around @code{format} outputting to @code{(current-error-port)}.
> +
> +@item (wall @var{fmt} @var{args...})
> +Wrapper around @code{format} outputting via @command{wall}.
> +
> +@item (apcupsd @var{args...})
> +Call @command{apcupsd} while passing the correct configuration file and
> +all the arguments.
> +
> +@item (mail-to-root @var{subject} @var{body})
> +Send an email to the local administrator. This procedure assumes the
> +@command{sendmail} is located at @command{/run/privileged/bin/sendmail}
> +(as would be the case with @code{opensmtpd-service-type}).
> +
> +@end table
> +
> +Available @code{apcupsd-event-handlers} fields are:
> +
> +@table @asis
> +@item @code{killpower} (type: gexp)
> +Handler for killpower event.
> +
> +@item @code{commfailure} (type: gexp)
> +Handler for commfailure event.
> +
> +@item @code{commok} (type: gexp)
> +Handler for commfailure event.
> +
> +@item @code{powerout} (type: gexp)
> +Handler for powerout event.
> +
> +@item @code{onbattery} (type: gexp)
> +Handler for onbattery event.
> +
> +@item @code{offbattery} (type: gexp)
> +Handler for offbattery event.
> +
> +@item @code{mainsback} (type: gexp)
> +Handler for mainsback event.
> +
> +@item @code{failing} (type: gexp)
> +Handler for failing event.
> +
> +@item @code{timeout} (type: gexp)
> +Handler for timeout event.
> +
> +@item @code{loadlimit} (type: gexp)
> +Handler for loadlimit event.
> +
> +@item @code{runlimit} (type: gexp)
> +Handler for runlimit event.
> +
> +@item @code{doreboot} (type: gexp)
> +Handler for doreboot event.
> +
> +@item @code{doshutdown} (type: gexp)
> +Handler for doshutdown event.
> +
> +@item @code{annoyme} (type: gexp)
> +Handler for annoyme event.
> +
> +@item @code{emergency} (type: gexp)
> +Handler for emergency event.
> +
> +@item @code{changeme} (type: gexp)
> +Handler for changeme event.
> +
> +@item @code{remotedown} (type: gexp)
> +Handler for remotedown event.
> +
> +@item @code{startselftest} (type: gexp)
> +Handler for startselftest event.
> +
> +@item @code{endselftest} (type: gexp)
> +Handler for endselftest event.
> +
> +@item @code{battdetach} (type: gexp)
> +Handler for battdetach event.
> +
> +@item @code{battattach} (type: gexp)
> +Handler for battattach event.
> +
> +@end table
> +
> +@end deftp
nitpick: I'd remove the newline between the two @end above (I know, it
gets generated this way...).
[...]
> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>
We use the unicode symbol (©) for copyright throughout Guix; please use
it too, for uniformity.
> +
> +;;;; Commentary:
> +
> +;;; Power-related services.
> +
> +;;;; Code:
> +
> +(define-module (gnu services power)
> + #:use-module (srfi srfi-1)
> + #:use-module (srfi srfi-26)
> + #:use-module (ice-9 match)
> + #:use-module (gnu)
> + #:use-module (gnu packages admin)
> + #:use-module (gnu packages linux)
> + #:use-module (gnu packages power)
> + #:use-module (gnu services)
> + #:use-module (gnu services configuration)
> + #:use-module (gnu services shepherd)
> + #:use-module (guix packages)
> + #:use-module (guix records)
Please order lexicographically (that is, alphabetically but on things
which are not necessarily words :-)).
> + #:export (apcupsd-service-type
> +
> + apcupsd-configuration
> + apcupsd-configuration-package
> + apcupsd-configuration-shepherd-base-name
> + apcupsd-configuration-auto-start?
> + apcupsd-configuration-pid-file
> + apcupsd-configuration-debug-level
> + apcupsd-configuration-run-dir
> + apcupsd-configuration-name
> + apcupsd-configuration-cable
> + apcupsd-configuration-type
> + apcupsd-configuration-device
> + apcupsd-configuration-poll-time
> + apcupsd-configuration-on-batery-delay
> + apcupsd-configuration-battery-level
> + apcupsd-configuration-remaining-minutes
> + apcupsd-configuration-timeout
> + apcupsd-configuration-annoy-interval
> + apcupsd-configuration-annoy-delay
> + apcupsd-configuration-no-logon
> + apcupsd-configuration-kill-delay
> + apcupsd-configuration-net-server
> + apcupsd-configuration-net-server-ip
> + apcupsd-configuration-net-server-port
> + apcupsd-configuration-net-server-events-file
> + apcupsd-configuration-net-server-events-file-max-size
> + apcupsd-configuration-class
> + apcupsd-configuration-mode
> + apcupsd-configuration-stat-time
> + apcupsd-configuration-log-stats
> + apcupsd-configuration-data-time
> + apcupsd-configuration-facility
> + apcupsd-configuration-event-handlers
> +
> + apcupsd-event-handlers-annoyme
> + apcupsd-event-handlers-battattach
> + apcupsd-event-handlers-battdetach
> + apcupsd-event-handlers-changeme
> + apcupsd-event-handlers-commfailure
> + apcupsd-event-handlers-commok
> + apcupsd-event-handlers-doreboot
> + apcupsd-event-handlers-doshutdown
> + apcupsd-event-handlers-emergency
> + apcupsd-event-handlers-endselftest
> + apcupsd-event-handlers-failing
> + apcupsd-event-handlers-killpower
> + apcupsd-event-handlers-loadlimit
> + apcupsd-event-handlers-mainsback
> + apcupsd-event-handlers-offbattery
> + apcupsd-event-handlers-onbattery
> + apcupsd-event-handlers-powerout
> + apcupsd-event-handlers-remotedown
> + apcupsd-event-handlers-runlimit
> + apcupsd-event-handlers-startselftest
> + apcupsd-event-handlers-timeout))
> +
> +(define-configuration/no-serialization apcupsd-event-handlers
> + (killpower
> + (gexp
> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
> + (sleep 10)
> + (apcupsd "--killpower")
> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
> + "Handler for killpower event.")
In many places in your writing, I find that it skips adding 'the'
(definite article), which I've learned should be use when describing
something specific, such as here with the specific handlers:
"Handler for the @code{killpower} event."
In case it helps, I've found this page seems useful in briefly
explaining when to use 'the' [0].
[0] https://owl.purdue.edu/owl/general_writing/grammar/using_articles.html
> + (commfailure
> + (gexp
> + #~((let ((msg (format #f "~a Communications with UPS ~a lost."
> + (gethostname) name)))
> + (mail-to-root msg msg))
> + (wall "Warning: communications lost with UPS ~a" name)))
> + "Handler for commfailure event.")
> + (commok
> + (gexp
> + #~((let ((msg (format #f "~a Communications with UPS ~a restored."
> + (gethostname) name)))
> + (mail-to-root msg msg))
> + (wall "Communications restored with UPS ~a" name)))
> + "Handler for commfailure event.")
> + (powerout
> + (gexp
> + #~(#t))
> + "Handler for powerout event.")
> + (onbattery
> + (gexp
> + #~((let ((msg (format #f "~a UPS ~a Power Failure !!!"
> + (gethostname) name)))
> + (mail-to-root msg msg))
> + (wall "Power failure on UPS ~a. Running on batteries." name)))
> + "Handler for onbattery event.")
> + (offbattery
> + (gexp
> + #~((let ((msg (format #f "~a UPS ~a Power has returned."
> + (gethostname) name)))
> + (mail-to-root msg msg))
> + (wall "Power has returned on UPS ~a..." name)))
> + "Handler for offbattery event.")
> + (mainsback
> + (gexp
> + #~((when (file-exists? powerfail-file)
> + (wall "Continuing with shutdown."))))
> + "Handler for mainsback event.")
> + (failing
> + (gexp
> + #~((wall "Battery power exhausted on UPS ~a. Doing shutdown." name)))
> + "Handler for failing event.")
> + (timeout
> + (gexp
> + #~((wall "Battery time limit exceeded on UPS ~a. Doing shutdown." name)))
> + "Handler for timeout event.")
> + (loadlimit
> + (gexp
> + #~((wall "Remaining battery charge below limit on UPS ~a. Doing shutdown." name)))
> + "Handler for loadlimit event.")
> + (runlimit
> + (gexp
> + #~((wall "Remaining battery runtime below limit on UPS ~a. Doing shutdown." name)))
> + "Handler for runlimit event.")
> + (doreboot
> + (gexp
> + #~((wall "UPS ~a initiating Reboot Sequence" name)
> + (system* #$(file-append shepherd "/sbin/reboot"))))
> + "Handler for doreboot event.")
> + (doshutdown
> + (gexp
> + #~((wall "UPS ~a initiated Shutdown Sequence" name)
> + (system* #$(file-append shepherd "/sbin/halt"))))
> + "Handler for doshutdown event.")
> + (annoyme
> + (gexp
> + #~((wall "Power problems with UPS ~a. Please logoff." name)))
> + "Handler for annoyme event.")
> + (emergency
> + (gexp
> + #~((wall "Emergency Shutdown. Possible battery failure on UPS ~a." name)))
> + "Handler for emergency event.")
> + (changeme
> + (gexp
> + #~((let ((msg (format #f "~a UPS ~a battery needs changing NOW."
> + (gethostname) name)))
> + (mail-to-root msg msg))
> + (wall "Emergency! Batteries have failed on UPS ~a. Change them NOW." name)))
> + "Handler for changeme event.")
> + (remotedown
> + (gexp
> + #~((wall "Remote Shutdown. Beginning Shutdown Sequence.")))
> + "Handler for remotedown event.")
> + (startselftest
> + (gexp
> + #~(#t))
> + "Handler for startselftest event.")
> + (endselftest
> + (gexp
> + #~(#t))
> + "Handler for endselftest event.")
> + (battdetach
> + (gexp
> + #~(#t))
> + "Handler for battdetach event.")
> + (battattach
> + (gexp
> + #~(#t))
> + "Handler for battattach event."))
Sounds fun :-). I was wondering if the handlers could be of the
maybe-gexp type, and when not provided the handler would not be called
at all? Or does the design of apcupsd mandates that all handlers be
defined?
> +(define-syntax define-enum
> + (lambda (x)
> + (syntax-case x ()
> + ((_ name values)
> + (let* ((d/n (syntax->datum #'name))
> + (d/predicate (string->symbol
> + (format #f "enum-~a?" d/n)))
> + (d/serialize (string->symbol
> + (format #f "serialize-enum-~a" d/n))))
Even for internal bindings, better variable names would help reading the
code.
> + (with-syntax
> + ((predicate (datum->syntax x d/predicate))
> + (serialize (datum->syntax x d/serialize)))
> + #'(begin
> + (define (predicate value)
> + (memq value values))
> + (define serialize serialize-symbol))))))))
That is a nice minimal enum for using with define-configuration. Note
that Guile has some kind of very limited enum from (rnrs enums), but I
don't think they'd be too useful here. See for example
'make-enumeration' from (rnrs enums), which I used in for the
ntp-server-types in (gnu services networking).
> +(define mangle-field-name
> + (match-lambda
> + ('name "UPSNAME")
> + ('cable "UPSCABLE")
> + ('type "UPSTYPE")
> + ('device "DEVICE")
> + ('poll-time "POLLTIME")
> + ('lock-dir "LOCKFILE")
> + ('power-fail-dir "PWRFAILDIR")
> + ('no-login-dir "NOLOGINDIR")
> + ('on-batery-delay "ONBATTERYDELAY")
> + ('battery-level "BATTERYLEVEL")
> + ('remaining-minutes "MINUTES")
> + ('timeout "TIMEOUT")
> + ('annoy-interval "ANNOY")
> + ('annoy-delay "ANNOYDELAY")
> + ('no-logon "NOLOGON")
> + ('kill-delay "KILLDELAY")
> + ('net-server "NETSERVER")
> + ('net-server-ip "NISIP")
> + ('net-server-port "NISPORT")
> + ('net-server-events-file "EVENTSFILE")
> + ('net-server-events-file-max-size "EVENTSFILEMAX")
> + ('class "UPSCLASS")
> + ('mode "UPSMODE")
> + ('stat-time "STATTIME")
> + ('stat-file "STATFILE")
> + ('log-stats "LOGSTATS")
> + ('data-time "DATATIME")
> + ('facility "FACILITY")))
> +
> +(define (serialize-string field-name value)
> + #~(format #f "~a ~a\n" #$(mangle-field-name field-name) '#$value))
> +(define serialize-symbol serialize-string)
> +(define serialize-integer serialize-string)
> +(define (serialize-boolean field-name value)
> + #~(format #f "~a ~a\n" #$(mangle-field-name field-name) #$(if value "on" "off")))
You'll want to break the above line.
> +
> +(define-maybe string)
> +
> +(define-enum cable '( simple smart ether usb
> + 940-0119A 940-0127A 940-0128A 940-0020B 940-0020C
> + 940-0023A 940-0024B 940-0024C 940-1524C 940-0024G
> + 940-0095A 940-0095B 940-0095C 940-0625A MAM-04-02-2000))
> +(define-enum type '(apcsmart usb net snmp netsnmp dumb pcnet modbus test))
> +(define-enum no-logon '(disable timeout percent minutes always))
> +(define-enum class '(standalone shareslave sharemaster))
> +(define-enum mode '(disable share))
> +
> +(define-configuration apcupsd-configuration
> + (package (package apcupsd) "Package to use.")
Please stick to the convention where the name of the package is the name
of the field used to specify such package (here: apcupsd).
> + (shepherd-base-name
> + (symbol 'apcupsd)
> + "Base name of the shepherd service. You can add the service multiple times
> +with different prefix to manage multiple UPSes."
> + empty-serializer)
> + (auto-start?
> + (boolean #t)
> + "Should the shepherd service auto-start?"
> + empty-serializer)
> + (pid-file
> + (string "/var/run/apcupsd.pid")
/var/run -> /run (adjust everywhere)
> + "Path to the pid file."
I'm repeating myself because I reviewed the texi produced from this, but
yeah, s/path/file name/, and make sure to adjust the rest of the
comments I made for the .texi in the source (I'd just edit the source
and regenerate the doc from it).
[...]
> +(define (s/apccontrol cfg)
what is the 's/' prefix for in the procedure name? 'serialize'? Spell
this out in full to be clear rather than cryptic (that's a convention
followed at large in general in Scheme).
> + (program-file
> + "apccontrol"
> + #~(begin
> + (use-modules (srfi srfi-9)
> + (ice-9 format)
> + (ice-9 match)
> + (ice-9 popen))
Please sort modules lexicographically.
> + ;; Script dir depends on these, and the configuration depends on the script
> + ;; dir. To sever the cyclic dependency, pass the paths via environment
> + ;; variables.
> + (define conf (getenv "GUIX_APCUPSD_CONF"))
> + (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
> +
> + (define (err . args)
> + (apply format (current-error-port) args))
> + (define (wall . args)
> + (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
> + (define (apcupsd . args)
> + (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
> + (define (mail-to-root subject body)
> + (let ((port (open-pipe* OPEN_WRITE
> + "/run/privileged/bin/sendmail"
> + "-F" "apcupsd"
> + "root")))
> + (format port "Subject: ~a~%~%~a~&" subject body)
> + (close-pipe port)))
> + (match (cdr (command-line))
> + (((? string? cmd) name connected powered)
> + (match cmd
> + ;; I am sure this could be done by macro, but meh. Last release of
> + ;; apcupsd was in 2016, so maintaining this will not be much work.
> + ("killpower"
> + #$@(apcupsd-event-handlers-killpower
> + (apcupsd-configuration-event-handlers cfg)))
> + ("commfailure"
> + #$@(apcupsd-event-handlers-commfailure
> + (apcupsd-configuration-event-handlers cfg)))
> + ("commok"
> + #$@(apcupsd-event-handlers-commok
> + (apcupsd-configuration-event-handlers cfg)))
> + ("powerout"
> + #$@(apcupsd-event-handlers-powerout
> + (apcupsd-configuration-event-handlers cfg)))
> + ("onbattery"
> + #$@(apcupsd-event-handlers-onbattery
> + (apcupsd-configuration-event-handlers cfg)))
> + ("offbattery"
> + #$@(apcupsd-event-handlers-offbattery
> + (apcupsd-configuration-event-handlers cfg)))
> + ("mainsback"
> + #$@(apcupsd-event-handlers-mainsback
> + (apcupsd-configuration-event-handlers cfg)))
> + ("failing"
> + #$@(apcupsd-event-handlers-failing
> + (apcupsd-configuration-event-handlers cfg)))
> + ("timeout"
> + #$@(apcupsd-event-handlers-timeout
> + (apcupsd-configuration-event-handlers cfg)))
> + ("loadlimit"
> + #$@(apcupsd-event-handlers-loadlimit
> + (apcupsd-configuration-event-handlers cfg)))
> + ("runlimit"
> + #$@(apcupsd-event-handlers-runlimit
> + (apcupsd-configuration-event-handlers cfg)))
> + ("doreboot"
> + #$@(apcupsd-event-handlers-doreboot
> + (apcupsd-configuration-event-handlers cfg)))
> + ("doshutdown"
> + #$@(apcupsd-event-handlers-doshutdown
> + (apcupsd-configuration-event-handlers cfg)))
> + ("annoyme"
> + #$@(apcupsd-event-handlers-annoyme
> + (apcupsd-configuration-event-handlers cfg)))
> + ("emergency"
> + #$@(apcupsd-event-handlers-emergency
> + (apcupsd-configuration-event-handlers cfg)))
> + ("changeme"
> + #$@(apcupsd-event-handlers-changeme
> + (apcupsd-configuration-event-handlers cfg)))
> + ("remotedown"
> + #$@(apcupsd-event-handlers-remotedown
> + (apcupsd-configuration-event-handlers cfg)))
> + ("startselftest"
> + #$@(apcupsd-event-handlers-startselftest
> + (apcupsd-configuration-event-handlers cfg)))
> + ("endselftest"
> + #$@(apcupsd-event-handlers-endselftest
> + (apcupsd-configuration-event-handlers cfg)))
> + ("battdetach"
> + #$@(apcupsd-event-handlers-battdetach
> + (apcupsd-configuration-event-handlers cfg)))
> + ("battattach"
> + #$@(apcupsd-event-handlers-battattach
> + (apcupsd-configuration-event-handlers cfg)))
> + (_
> + (err "Unknown event: ~a~%" cmd)
> + (err "Iff the event was passed by apcupsd, this is a bug.~%")
s/Iff/If/, perhaps s/passed/emitted/. I don't understand, in which scenario
would an event *not* be emitted by apcupsd?
> + (err "Please report to bug-guix@gnu.org.~%")
> + (exit #f))))
> + (args
> + (err "Unknown arguments: ~a~%" args)
> + (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
s/Iff/If/
> + (err "Please report to bug-guix@gnu.org.~%")
> + (exit #f))))))
> +
> +(define (apcupsd-script-dir cfg)
s/cfg/config/
> + (computed-file
> + "apcupsd-script-dir"
> + #~(begin
> + (mkdir #$output)
> + (chdir #$output)
> + (symlink #$(s/apccontrol cfg) "apccontrol"))))
> +
> +(define (apcupsd-config-file cfg)
s/cfg/config/
> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))
> + (mixed-text-file
> + "apcupsd.conf"
> + "\
> +## apcupsd.conf v1.1 ##
> +#
> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix
Is this config really bound to a version like above? Unless they change
things in backward incompatible ways, it should work for newer ones too,
no?
> +#
> +# \"apcupsd\" POSIX config file (crafted via apcupsd-service-type)
s/crafted via/auto-generated by/
> +"
> + (serialize-configuration cfg apcupsd-configuration-fields)
> + ;; This one is confusing. The manual page states:
> + ;;
> + ;; > It must be changed when running more than one copy of apcupsd on the
> + ;; > same computer to control multiple UPSes.
> + ;;
> + ;; However would you not want the lock to be per-device, not per-process?
> + ;; I decided to follow the documentation, but I do not understand why it
> + ;; should be like this. I do not have multiple UPSes to try.
> + (serialize-string 'lock-dir (string-append run-dir "/lock"))
> + (serialize-string 'power-fail-dir run-dir)
> + (serialize-string 'no-login-dir run-dir)
> + (serialize-string 'stat-file (string-append run-dir "/apcupsd.status"))
> + "SCRIPTDIR " (apcupsd-script-dir cfg) "\n")))
> +
> +(define (apcupsd-shepherd-services cfg)
s/cfg/config/
> + (match-record cfg <apcupsd-configuration>
> + ( package pid-file debug-level run-dir
^ extraneous space
> + shepherd-base-name auto-start?)
> + (let* ((config-file (apcupsd-config-file cfg))
> + (s/ shepherd-base-name)
Please improve the variables names a bit; what is 's/' ?
> + (s/run-dirs (string->symbol (format #f "~a-run-dirs" s/))))
> + (list
> + (shepherd-service
> + (documentation "Create the run directories.")
> + (provision (list s/run-dirs))
> + (one-shot? #t)
> + (auto-start? auto-start?)
> + (start #~(lambda _
> + ((@ (guix build utils) mkdir-p)
> + #$(string-append run-dir "/lock"))
> + #t)))
Is there a reason why this is not done in an activation script instead?
That's more conventional, I think.
> + (shepherd-service
> + (documentation "Run apcupsd daemon.")
Run *the* apcupsd daemon.
> + (requirement (list s/run-dirs))
> + (provision (list s/))
> + (auto-start? auto-start?)
> + (start #~(make-forkexec-constructor
> + '(#$(file-append package "/sbin/apcupsd")
> + "-b" ; Do not daemonize.
If you want, margin comments are ok without space or full sentence
(punctuation), e.g.:
--8<---------------cut here---------------start------------->8---
;do not daemonize
--8<---------------cut here---------------end--------------->8---
> + "-f" #$config-file
> + "-P" #$pid-file
> + "-d" #$(number->string debug-level))
> + #:log-file
> + #$(format #f "/var/log/~a.log" shepherd-base-name)
> + #:environment-variables
> + (cons* (string-append "GUIX_APCUPSD_CONF="
> + #$config-file)
> + #$(string-append "GUIX_APCUPSD_POWERFAIL_FILE="
> + run-dir "/powerfail")
> + (default-environment-variables))))
> + (stop #~(make-kill-destructor))
> + (actions (list (shepherd-configuration-action config-file))))))))
> +
> +(define (apcupsd-pam-extensions cfg)
s/cfg/config/
> + (define pam-nologin
A comment explaining why this PAM extention is required would be helpful
(I have no idea myself -- but I must confess: PAM is still a bit of a
mystery to me).
> + (pam-entry
> + (control "required")
> + (module "pam_nologin.so")
> + (arguments (list (string-append "file="
> + (apcupsd-configuration-run-dir cfg)
> + "/nologin")))))
> +
> + (list (pam-extension
> + (transformer
> + (lambda (pam)
> + (pam-service
> + (inherit pam)
> + (auth (cons pam-nologin (pam-service-auth pam)))))))))
> +
> +(define apcupsd-service-type
> + (service-type
> + (name 'apcupsd)
> + (description "Configure and optionally start apcupsd.")
> + (extensions (list (service-extension shepherd-root-service-type
> + apcupsd-shepherd-services)
> + (service-extension pam-root-service-type
> + apcupsd-pam-extensions)))
> + (compose identity)
> + (extend (λ (cfg lst)
nitpick: There was a stylistic choice to prefer lambda instead of λ in
the Guix code base to try to make it more accessible at some point.
> + (fold (cut <> <>) cfg lst)))
> + (default-value (apcupsd-configuration))))
Thanks for this very complete, carefully and thoughtfully crafted
contribution. As you can see, the only comments I had were mostly
nitpicks and small details. I look forward to the next revision.
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
> Hi Tomas,
>
> Tomas Volf <~@wolfsden.cz> writes:
>
>> * gnu/services/power.scm: New file.
>
> I see there's already a gnu/services/pm.scm file; it seems we don't need
> another one? Or was it a conscious decision?
Yes, it was intentional, for couple of reasons (in no particular order).
I like the symmetry between gnu/{packages,services}/power.scm. It makes
finding the service-type easy without even opening the manual.
The "pm" is for "power management", and I have never heard anyone call
apcupsd "a power management daemon". It reacts to power outage, true,
but it does not "manage" the power in any way. So it seemed like a poor
fit.
The current pm.scm exports 8 bindings, the new power.scm 54. So it
would feel like taking over the file.
Shorter files are faster to compile (especially when
define-configuration is used), so having two files that can be built in
parallel seemed better.
And like this I can also write the changelog in to the commit message in
a reasonably short form. If I put it into pm.scm, I would need to list
every single binding, in power.scm I mention just a new file.
>
>> +The @code{(gnu services power)} module provides a service definition for
>> +@uref{http://www.apcupsd.org/, apcupsd}, a utility to interact with APC
>> +UPSes. Apcupsd also works with some OEM-branded products manufactured
>> +by APC.
>
> I'd introduce a few @acronym here to help readers, e.g. for OEM and
> UPS.
I added ones for UPS, OEM and APC.
>
>> +@defvar apcupsd-service-type
>> +The service type for apcupsd. For USB UPSes no configuration is
>> +necessary, however tweaking some fields to better suit your needs might
>> +be desirable. The defaults are taken from the upstream configuration
>> +and they are not very conservative (@code{battery-level} of 5% is too
>> +low in my opinion).
>
> I'd say, (for example, the default @code{battery-level} of 5% may be
> considered too low by some), to keep it neutral.
Done.
>
>> +The default event handlers do send emails, read more in
>> +@ref{apcupsd-event-handlers}.
>> +
>> +@lisp
>> +(service apcupsd-service-type)
>> +@end lisp
>> +@end defvar
>> +
>> +@deftp {Data Type} apcupsd-configuration
>> +
>> +Available @code{apcupsd-configuration} fields are:
>> +
>> +@table @asis
>> +@item @code{package} (default: @code{apcupsd}) (type: package)
>> +Package to use.
>
> That's more conventionally named with the same of the package,
> e.g. 'acpupsd', not 'package'. I'd word the description as 'The
> @code{apcupsd} package to use'.
I see, good to know, will try to adhere to that convention from now on.
>
>> +@item @code{shepherd-base-name} (default: @code{apcupsd}) (type: symbol)
>> +Base name of the shepherd service. You can add the service multiple
>> +times with different prefix to manage multiple UPSes.
>
> s/prefix/prefixes/
I will just take your word on this one. :)
>> +
>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)
>> +Should the shepherd service auto-start?
>> +
>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)
>> +Path to the pid file.
>
> All file names becoming /run/ prefixed instead of /var/run, based on a
> previous comment.
I will just quote myself from the package's patch, since it seems I did
not get an answer there:
--8<---------------cut here---------------start------------->8---
It seems pretty much all programs on my system are using /var/run,
including Shepherd. Only programs that store information in /run seem
to be elogind and dbus.
Is deprecation of /var/run listed somewhere in the manual? I am
surprised Shepherd (since it is actively maintained and core component)
is still in /var/run. I looked and found 42 references to /var/run, but
none with the deprecation notice.
Assuming you will insist on moving it to /run, is there some structure
to follow in that directory? Or just s~/var/run~/run~? What about /var
and /var/log? Should those be moved somewhere as well?
--8<---------------cut here---------------end--------------->8---
Thanks.
>
> We don't use path in GNU to denote file names, we use 'file names'.
> That's mentioned in info '(standards) GNU Manuals':
>
> Please do not use the term "pathname" that is used in Unix
> documentation; use "file name" (two words) instead. We use the term
> "path" only for search paths, which are lists of directory names.
>
Changed into "File name of the pid file.".
Out of curiosity, does this apply to directories as well? So,
hypothetically, I should instead of "Path to the directory storing XY."
use "File name of the directory storing XY."?
>> +
>> +@item @code{debug-level} (default: @code{0}) (type: integer)
>> +Logging verbosity. Bigger number means more logs. In the source code I
>> +saw up to @code{300}, so for all logs, @code{999} seems reasonable.
>
> I think it's best to not use first person pronoun in the manual to keep
> it a bit more formal. So instead of 'I saw', this could be reworded to:
> 'The source code uses up to @code{300} as debug level value, so a value
> of @code{999} seems reasonable to enable all logs.' or similar.
Sorry, I have probably gotten too influenced by (gnus) and its personal
style I have enjoyed a lot.
I have used your suggestion verbatim.
>
>> +
>> +@item @code{run-dir} (default: @code{"/var/run/apcupsd"}) (type: string)
>> +Directory containing runtime information. You need to change this if
>> +you desire to run multiple instances of the daemon.
>> +
>> +@item @code{name} (type: maybe-string)
>> +Use this to give your UPS a name in log files and such. This is
>> +particularly useful if you have multiple UPSes. This does not set the
>> +EEPROM. It should be 8 characters or less.
>> +
>> +@item @code{cable} (default: @code{usb}) (type: enum-cable)
>> +The type of cable connecting the UPS to your computer. Possible generic
>> +choices are @code{'simple}, @code{'smart}, @code{'ether} and
>> +@code{'usb}. Or a specific cable model number may be used:
>
> nitpick: I'd reword to 'Alternatively, a specific [...]', which reads
> better to me.
Done.
>
>> +@item usb
>> +A null string setting enables autodetection, which is the best choice
>> +for most installations.
>
> nitpick: My dictionary (aspell) prefers auto-detection.
Huh, so does hunspell. Shame on me, I need to remember to enable
flyspell-prog-mode more often.
>
> [...]
>
>> +@anchor{apcupsd-event-handlers}
>> +@deftp {Data Type} apcupsd-event-handlers
>> +
>> +For description of the events please refer to the @command{apcupsd}'s
>> +manual, which can be found in the @samp{apcupsd-doc} package.
>
> I think you also meant it is in the @samp{doc} output, not in a
> different package, right?
Right.
> In any case, I'd refer them to the man page of apccontrol, which
> details the events. Also, to avoid the odd rendering as "the
> ‘apcupsd’’s manual" in info, I'd instead rephrase to:
>
> For a description of the events please refer to @samp{man 8
> apccontrol}.
I like this version, thank you.
>
>> +
>> +Each handler shall be a gexp. It is spliced into the control script for
>> +the daemon. In addition to the standard Guile programming environment,
>> +these procedures and variables are also available.
>
> s/these/the following/ [...] s/available./available:/
Done.
> Since these are spliced in the control script and thus not at the top
> level, users won't be able to import Guile modules they may want to use,
> so it seems there should be a field to define which Guile modules should
> be imported and used by default in the configuration.
Hm, that is a good point. I did not think of that. I have added a
`modules' field to the apcupsd-event-handlers record.
>
>> +
>> +@table @code
>> +@item conf
>> +Variable containing path to the configuration file.
>
> s/path/file name/, anywhere this appears.
Done. I went back to the package and replaced it there as well. It is
bit unfortunate that autotools call the variables `ac_cv_path_', instead
of `ac_cv_file_name_', but nothing I can do about that.
>
>> +
>> +@item powerfail-file
>> +Variable containing path to the powerfail file.
>
> Ditto.
And done.
>
>> +@item cmd
>> +The event currently being handled.
>> +
>> +@item name
>> +The name of the UPS as specified in the configuration file.
>> +
>> +@item connected
>> +Is @code{"1"} if apcupsd is connected to the UPS via a serial port (or a
>
> @command{apcupsd}
Thanks.
>
> Also, any reason why these integer values are expressed as strings?
No. I have just adhered to the original calling convention of the shell
script, and argv is all char*. These are even not really an integer
values, but bools. So I changed this one to `connected?', and the other
one to `powered?', with #t/#f values.
>
>> +USB port). In most configurations, this will be the case. In the case
>> +of a Slave machine where apcupsd is not directly connected to the UPS,
>
> s/Slave/slave/ ? Could be nicer to use a neutral alternative like
> 'follower machine'.
The "Slave machine" (including the capitalization) is a term used in the
manual. I can lower-case it if you would prefer, seems that manual
actually uses both casings now when I looked for it.
I do not think inventing new terminology is a good idea, since users
will not be able to find it in the manual distributed with the program.
>
>> +this value will be @code{"0"}.
>> +
>> +@item powered
>> +Is @code{"1"} if the computer on which @command{apcupsd} is running is
>> +powered by the UPS and @code{"0"} if not. At the moment, this value is
>> +unimplemented and always @code{"0"}.
>> +
>> +@item (err @var{fmt} @var{args...})
>> +Wrapper around @code{format} outputting to @code{(current-error-port)}.
>> +
>> +@item (wall @var{fmt} @var{args...})
>> +Wrapper around @code{format} outputting via @command{wall}.
>> +
>> +@item (apcupsd @var{args...})
>> +Call @command{apcupsd} while passing the correct configuration file and
>> +all the arguments.
>> +
>> +@item (mail-to-root @var{subject} @var{body})
>> +Send an email to the local administrator. This procedure assumes the
>> +@command{sendmail} is located at @command{/run/privileged/bin/sendmail}
>> +(as would be the case with @code{opensmtpd-service-type}).
>> +
>> +@end table
>> +
>> +Available @code{apcupsd-event-handlers} fields are:
>> +
>> +@table @asis
>> +@item @code{killpower} (type: gexp)
>> +Handler for killpower event.
>> +
>> +@item @code{commfailure} (type: gexp)
>> +Handler for commfailure event.
>> +
>> +@item @code{commok} (type: gexp)
>> +Handler for commfailure event.
>> +
>> +@item @code{powerout} (type: gexp)
>> +Handler for powerout event.
>> +
>> +@item @code{onbattery} (type: gexp)
>> +Handler for onbattery event.
>> +
>> +@item @code{offbattery} (type: gexp)
>> +Handler for offbattery event.
>> +
>> +@item @code{mainsback} (type: gexp)
>> +Handler for mainsback event.
>> +
>> +@item @code{failing} (type: gexp)
>> +Handler for failing event.
>> +
>> +@item @code{timeout} (type: gexp)
>> +Handler for timeout event.
>> +
>> +@item @code{loadlimit} (type: gexp)
>> +Handler for loadlimit event.
>> +
>> +@item @code{runlimit} (type: gexp)
>> +Handler for runlimit event.
>> +
>> +@item @code{doreboot} (type: gexp)
>> +Handler for doreboot event.
>> +
>> +@item @code{doshutdown} (type: gexp)
>> +Handler for doshutdown event.
>> +
>> +@item @code{annoyme} (type: gexp)
>> +Handler for annoyme event.
>> +
>> +@item @code{emergency} (type: gexp)
>> +Handler for emergency event.
>> +
>> +@item @code{changeme} (type: gexp)
>> +Handler for changeme event.
>> +
>> +@item @code{remotedown} (type: gexp)
>> +Handler for remotedown event.
>> +
>> +@item @code{startselftest} (type: gexp)
>> +Handler for startselftest event.
>> +
>> +@item @code{endselftest} (type: gexp)
>> +Handler for endselftest event.
>> +
>> +@item @code{battdetach} (type: gexp)
>> +Handler for battdetach event.
>> +
>> +@item @code{battattach} (type: gexp)
>> +Handler for battattach event.
>> +
>> +@end table
>> +
>> +@end deftp
>
> nitpick: I'd remove the newline between the two @end above (I know, it
> gets generated this way...).
Done.
>
>> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>
>
> We use the unicode symbol (©) for copyright throughout Guix; please use
> it too, for uniformity.
So © is fine, but λ is not. I fail to see the consistency.
(But of course adjusted, it is just that I do not get the difference.)
>
>> +
>> +;;;; Commentary:
>> +
>> +;;; Power-related services.
>> +
>> +;;;; Code:
>> +
>> +(define-module (gnu services power)
>> + #:use-module (srfi srfi-1)
>> + #:use-module (srfi srfi-26)
>> + #:use-module (ice-9 match)
>> + #:use-module (gnu)
>> + #:use-module (gnu packages admin)
>> + #:use-module (gnu packages linux)
>> + #:use-module (gnu packages power)
>> + #:use-module (gnu services)
>> + #:use-module (gnu services configuration)
>> + #:use-module (gnu services shepherd)
>> + #:use-module (guix packages)
>> + #:use-module (guix records)
>
> Please order lexicographically (that is, alphabetically but on things
> which are not necessarily words :-)).
Interesting. I personally order imports lexicographically, but within
groups of:
1. stdlib (srfi, rnrs, ...)
2. guile extensions (ice-9, ...)
3. 3rd-party dependencies (guile-lib, guile-json, ...)
4. modules from the program itself (gnu, guix, ...)
Given your requirement for sorting everything lexicographically (with no
grouping), the following:
--8<---------------cut here---------------start------------->8---
(use-modules (gnu services)
(srfi srfi-1))
(when #f
(modify-services '()
(delete 'x)))
--8<---------------cut here---------------end--------------->8---
Prints a warning when compiled:
--8<---------------cut here---------------start------------->8---
WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'
--8<---------------cut here---------------end--------------->8---
However, when done "my" way (putting srfi-1 first, since "stdlib"), the
warning goes away.
This is not really issue here, since I am not using modify-services, the
warning is not printed (I have no idea why). But since in general order
of imports seem to matter, I wonder whether mandating global order is a
good idea.
What are your thoughts here?
>
>> + #:export (apcupsd-service-type
>> +
>> + apcupsd-configuration
>> + apcupsd-configuration-package
>> + apcupsd-configuration-shepherd-base-name
>> + apcupsd-configuration-auto-start?
>> + apcupsd-configuration-pid-file
>> + apcupsd-configuration-debug-level
>> + apcupsd-configuration-run-dir
>> + apcupsd-configuration-name
>> + apcupsd-configuration-cable
>> + apcupsd-configuration-type
>> + apcupsd-configuration-device
>> + apcupsd-configuration-poll-time
>> + apcupsd-configuration-on-batery-delay
>> + apcupsd-configuration-battery-level
>> + apcupsd-configuration-remaining-minutes
>> + apcupsd-configuration-timeout
>> + apcupsd-configuration-annoy-interval
>> + apcupsd-configuration-annoy-delay
>> + apcupsd-configuration-no-logon
>> + apcupsd-configuration-kill-delay
>> + apcupsd-configuration-net-server
>> + apcupsd-configuration-net-server-ip
>> + apcupsd-configuration-net-server-port
>> + apcupsd-configuration-net-server-events-file
>> + apcupsd-configuration-net-server-events-file-max-size
>> + apcupsd-configuration-class
>> + apcupsd-configuration-mode
>> + apcupsd-configuration-stat-time
>> + apcupsd-configuration-log-stats
>> + apcupsd-configuration-data-time
>> + apcupsd-configuration-facility
>> + apcupsd-configuration-event-handlers
>> +
>> + apcupsd-event-handlers-annoyme
>> + apcupsd-event-handlers-battattach
>> + apcupsd-event-handlers-battdetach
>> + apcupsd-event-handlers-changeme
>> + apcupsd-event-handlers-commfailure
>> + apcupsd-event-handlers-commok
>> + apcupsd-event-handlers-doreboot
>> + apcupsd-event-handlers-doshutdown
>> + apcupsd-event-handlers-emergency
>> + apcupsd-event-handlers-endselftest
>> + apcupsd-event-handlers-failing
>> + apcupsd-event-handlers-killpower
>> + apcupsd-event-handlers-loadlimit
>> + apcupsd-event-handlers-mainsback
>> + apcupsd-event-handlers-offbattery
>> + apcupsd-event-handlers-onbattery
>> + apcupsd-event-handlers-powerout
>> + apcupsd-event-handlers-remotedown
>> + apcupsd-event-handlers-runlimit
>> + apcupsd-event-handlers-startselftest
>> + apcupsd-event-handlers-timeout))
>> +
>> +(define-configuration/no-serialization apcupsd-event-handlers
>> + (killpower
>> + (gexp
>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
>> + (sleep 10)
>> + (apcupsd "--killpower")
>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
>> + "Handler for killpower event.")
>
> In many places in your writing, I find that it skips adding 'the'
> (definite article), which I've learned should be use when describing
> something specific, such as here with the specific handlers:
>
> "Handler for the @code{killpower} event."
>
> In case it helps, I've found this page seems useful in briefly
> explaining when to use 'the' [0].
>
> [0] https://owl.purdue.edu/owl/general_writing/grammar/using_articles.html
Yeah I always have a problem with these. My mother tongue does not have
this concept at all, so it is bit hard for me. Thank you for the link,
it was useful. I went through the texts and tried to add them where I
felt they were missing, you are right, there were a lot of them missing.
>
>> + (commfailure
>> + (gexp
>> + #~((let ((msg (format #f "~a Communications with UPS ~a lost."
>> + (gethostname) name)))
>> + (mail-to-root msg msg))
>> + (wall "Warning: communications lost with UPS ~a" name)))
>> + "Handler for commfailure event.")
>> + (commok
>> + (gexp
>> + #~((let ((msg (format #f "~a Communications with UPS ~a restored."
>> + (gethostname) name)))
>> + (mail-to-root msg msg))
>> + (wall "Communications restored with UPS ~a" name)))
>> + "Handler for commfailure event.")
>> + (powerout
>> + (gexp
>> + #~(#t))
>> + "Handler for powerout event.")
>> + (onbattery
>> + (gexp
>> + #~((let ((msg (format #f "~a UPS ~a Power Failure !!!"
>> + (gethostname) name)))
>> + (mail-to-root msg msg))
>> + (wall "Power failure on UPS ~a. Running on batteries." name)))
>> + "Handler for onbattery event.")
>> + (offbattery
>> + (gexp
>> + #~((let ((msg (format #f "~a UPS ~a Power has returned."
>> + (gethostname) name)))
>> + (mail-to-root msg msg))
>> + (wall "Power has returned on UPS ~a..." name)))
>> + "Handler for offbattery event.")
>> + (mainsback
>> + (gexp
>> + #~((when (file-exists? powerfail-file)
>> + (wall "Continuing with shutdown."))))
>> + "Handler for mainsback event.")
>> + (failing
>> + (gexp
>> + #~((wall "Battery power exhausted on UPS ~a. Doing shutdown." name)))
>> + "Handler for failing event.")
>> + (timeout
>> + (gexp
>> + #~((wall "Battery time limit exceeded on UPS ~a. Doing shutdown." name)))
>> + "Handler for timeout event.")
>> + (loadlimit
>> + (gexp
>> + #~((wall "Remaining battery charge below limit on UPS ~a. Doing shutdown." name)))
>> + "Handler for loadlimit event.")
>> + (runlimit
>> + (gexp
>> + #~((wall "Remaining battery runtime below limit on UPS ~a. Doing shutdown." name)))
>> + "Handler for runlimit event.")
>> + (doreboot
>> + (gexp
>> + #~((wall "UPS ~a initiating Reboot Sequence" name)
>> + (system* #$(file-append shepherd "/sbin/reboot"))))
>> + "Handler for doreboot event.")
>> + (doshutdown
>> + (gexp
>> + #~((wall "UPS ~a initiated Shutdown Sequence" name)
>> + (system* #$(file-append shepherd "/sbin/halt"))))
>> + "Handler for doshutdown event.")
>> + (annoyme
>> + (gexp
>> + #~((wall "Power problems with UPS ~a. Please logoff." name)))
>> + "Handler for annoyme event.")
>> + (emergency
>> + (gexp
>> + #~((wall "Emergency Shutdown. Possible battery failure on UPS ~a." name)))
>> + "Handler for emergency event.")
>> + (changeme
>> + (gexp
>> + #~((let ((msg (format #f "~a UPS ~a battery needs changing NOW."
>> + (gethostname) name)))
>> + (mail-to-root msg msg))
>> + (wall "Emergency! Batteries have failed on UPS ~a. Change them NOW." name)))
>> + "Handler for changeme event.")
>> + (remotedown
>> + (gexp
>> + #~((wall "Remote Shutdown. Beginning Shutdown Sequence.")))
>> + "Handler for remotedown event.")
>> + (startselftest
>> + (gexp
>> + #~(#t))
>> + "Handler for startselftest event.")
>> + (endselftest
>> + (gexp
>> + #~(#t))
>> + "Handler for endselftest event.")
>> + (battdetach
>> + (gexp
>> + #~(#t))
>> + "Handler for battdetach event.")
>> + (battattach
>> + (gexp
>> + #~(#t))
>> + "Handler for battattach event."))
>
> Sounds fun :-). I was wondering if the handlers could be of the
> maybe-gexp type, and when not provided the handler would not be called
> at all? Or does the design of apcupsd mandates that all handlers be
> defined?
The control script is called for all events. So I *could* use
maybe-gexp, and change
--8<---------------cut here---------------start------------->8---
("battattach"
#$@(apcupsd-event-handlers-battattach
(apcupsd-configuration-event-handlers cfg)))
--8<---------------cut here---------------end--------------->8---
into
--8<---------------cut here---------------start------------->8---
("battattach"
#$@(let ((h (apcupsd-event-handlers-battattach
(apcupsd-configuration-event-handlers cfg))))
(if (maybe-value-set? h)
h
#~(#t))))
--8<---------------cut here---------------end--------------->8---
and do that for all the events. But I have to admit I do not consider
that an improvement. There probably is a better way to express that,
but the current approach (of always having, possibly NOP, handler
defined) helps keep the code straight forward.
>
>> +(define-syntax define-enum
>> + (lambda (x)
>> + (syntax-case x ()
>> + ((_ name values)
>> + (let* ((d/n (syntax->datum #'name))
>> + (d/predicate (string->symbol
>> + (format #f "enum-~a?" d/n)))
>> + (d/serialize (string->symbol
>> + (format #f "serialize-enum-~a" d/n))))
>
> Even for internal bindings, better variable names would help reading the
> code.
>
When I am writing syntax-case, I always struggle with the naming, since
I effectively have to have two variables for a single thing. One for
datum, and one for syntax.
I have expanded the d/ info full datum/, however if can suggest better
naming scheme for situations like this, I am all ears.
>> + (with-syntax
>> + ((predicate (datum->syntax x d/predicate))
>> + (serialize (datum->syntax x d/serialize)))
>> + #'(begin
>> + (define (predicate value)
>> + (memq value values))
>> + (define serialize serialize-symbol))))))))
>
> That is a nice minimal enum for using with define-configuration. Note
> that Guile has some kind of very limited enum from (rnrs enums), but I
> don't think they'd be too useful here. See for example
> 'make-enumeration' from (rnrs enums), which I used in for the
> ntp-server-types in (gnu services networking).
>
>> +(define mangle-field-name
>> + (match-lambda
>> + ('name "UPSNAME")
>> + ('cable "UPSCABLE")
>> + ('type "UPSTYPE")
>> + ('device "DEVICE")
>> + ('poll-time "POLLTIME")
>> + ('lock-dir "LOCKFILE")
>> + ('power-fail-dir "PWRFAILDIR")
>> + ('no-login-dir "NOLOGINDIR")
>> + ('on-batery-delay "ONBATTERYDELAY")
>> + ('battery-level "BATTERYLEVEL")
>> + ('remaining-minutes "MINUTES")
>> + ('timeout "TIMEOUT")
>> + ('annoy-interval "ANNOY")
>> + ('annoy-delay "ANNOYDELAY")
>> + ('no-logon "NOLOGON")
>> + ('kill-delay "KILLDELAY")
>> + ('net-server "NETSERVER")
>> + ('net-server-ip "NISIP")
>> + ('net-server-port "NISPORT")
>> + ('net-server-events-file "EVENTSFILE")
>> + ('net-server-events-file-max-size "EVENTSFILEMAX")
>> + ('class "UPSCLASS")
>> + ('mode "UPSMODE")
>> + ('stat-time "STATTIME")
>> + ('stat-file "STATFILE")
>> + ('log-stats "LOGSTATS")
>> + ('data-time "DATATIME")
>> + ('facility "FACILITY")))
>> +
>> +(define (serialize-string field-name value)
>> + #~(format #f "~a ~a\n" #$(mangle-field-name field-name) '#$value))
>> +(define serialize-symbol serialize-string)
>> +(define serialize-integer serialize-string)
>> +(define (serialize-boolean field-name value)
>> + #~(format #f "~a ~a\n" #$(mangle-field-name field-name) #$(if value "on" "off")))
>
> You'll want to break the above line.
Done.
>
>> +
>> +(define-maybe string)
>> +
>> +(define-enum cable '( simple smart ether usb
>> + 940-0119A 940-0127A 940-0128A 940-0020B 940-0020C
>> + 940-0023A 940-0024B 940-0024C 940-1524C 940-0024G
>> + 940-0095A 940-0095B 940-0095C 940-0625A MAM-04-02-2000))
>> +(define-enum type '(apcsmart usb net snmp netsnmp dumb pcnet modbus test))
>> +(define-enum no-logon '(disable timeout percent minutes always))
>> +(define-enum class '(standalone shareslave sharemaster))
>> +(define-enum mode '(disable share))
>> +
>> +(define-configuration apcupsd-configuration
>> + (package (package apcupsd) "Package to use.")
>
> Please stick to the convention where the name of the package is the name
> of the field used to specify such package (here: apcupsd).
Done.
>
>> + (shepherd-base-name
>> + (symbol 'apcupsd)
>> + "Base name of the shepherd service. You can add the service multiple times
>> +with different prefix to manage multiple UPSes."
>> + empty-serializer)
>> + (auto-start?
>> + (boolean #t)
>> + "Should the shepherd service auto-start?"
>> + empty-serializer)
>> + (pid-file
>> + (string "/var/run/apcupsd.pid")
>
> /var/run -> /run (adjust everywhere)
See the reaction above regarding the /var/run.
>
>> + "Path to the pid file."
>
> I'm repeating myself because I reviewed the texi produced from this, but
> yeah, s/path/file name/, and make sure to adjust the rest of the
> comments I made for the .texi in the source (I'd just edit the source
> and regenerate the doc from it).
>
> [...]
Done (as part of the texi modification).
>
>
>> +(define (s/apccontrol cfg)
>
> what is the 's/' prefix for in the procedure name? 'serialize'? Spell
> this out in full to be clear rather than cryptic (that's a convention
> followed at large in general in Scheme).
The s/ was supposed to stand for script/, I originally expected I will
have more than one. I have renamed it to %apccontrol.
>
>> + (program-file
>> + "apccontrol"
>> + #~(begin
>> + (use-modules (srfi srfi-9)
>> + (ice-9 format)
>> + (ice-9 match)
>> + (ice-9 popen))
>
> Please sort modules lexicographically.
See the reaction above.
>
>> + ;; Script dir depends on these, and the configuration depends on the script
>> + ;; dir. To sever the cyclic dependency, pass the paths via environment
>> + ;; variables.
>> + (define conf (getenv "GUIX_APCUPSD_CONF"))
>> + (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
>> +
>> + (define (err . args)
>> + (apply format (current-error-port) args))
>> + (define (wall . args)
>> + (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
>> + (define (apcupsd . args)
>> + (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
>> + (define (mail-to-root subject body)
>> + (let ((port (open-pipe* OPEN_WRITE
>> + "/run/privileged/bin/sendmail"
>> + "-F" "apcupsd"
>> + "root")))
>> + (format port "Subject: ~a~%~%~a~&" subject body)
>> + (close-pipe port)))
>> + (match (cdr (command-line))
>> + (((? string? cmd) name connected powered)
>> + (match cmd
>> + ;; I am sure this could be done by macro, but meh. Last release of
>> + ;; apcupsd was in 2016, so maintaining this will not be much work.
>> + ("killpower"
>> + #$@(apcupsd-event-handlers-killpower
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("commfailure"
>> + #$@(apcupsd-event-handlers-commfailure
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("commok"
>> + #$@(apcupsd-event-handlers-commok
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("powerout"
>> + #$@(apcupsd-event-handlers-powerout
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("onbattery"
>> + #$@(apcupsd-event-handlers-onbattery
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("offbattery"
>> + #$@(apcupsd-event-handlers-offbattery
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("mainsback"
>> + #$@(apcupsd-event-handlers-mainsback
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("failing"
>> + #$@(apcupsd-event-handlers-failing
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("timeout"
>> + #$@(apcupsd-event-handlers-timeout
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("loadlimit"
>> + #$@(apcupsd-event-handlers-loadlimit
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("runlimit"
>> + #$@(apcupsd-event-handlers-runlimit
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("doreboot"
>> + #$@(apcupsd-event-handlers-doreboot
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("doshutdown"
>> + #$@(apcupsd-event-handlers-doshutdown
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("annoyme"
>> + #$@(apcupsd-event-handlers-annoyme
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("emergency"
>> + #$@(apcupsd-event-handlers-emergency
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("changeme"
>> + #$@(apcupsd-event-handlers-changeme
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("remotedown"
>> + #$@(apcupsd-event-handlers-remotedown
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("startselftest"
>> + #$@(apcupsd-event-handlers-startselftest
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("endselftest"
>> + #$@(apcupsd-event-handlers-endselftest
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("battdetach"
>> + #$@(apcupsd-event-handlers-battdetach
>> + (apcupsd-configuration-event-handlers cfg)))
>> + ("battattach"
>> + #$@(apcupsd-event-handlers-battattach
>> + (apcupsd-configuration-event-handlers cfg)))
>> + (_
>> + (err "Unknown event: ~a~%" cmd)
>> + (err "Iff the event was passed by apcupsd, this is a bug.~%")
>
> s/Iff/If/,
I think `Iff' is used correctly here, no?
> perhaps s/passed/emitted/.
Sure, why not.
> I don't understand, in which scenario would an event *not* be emitted
> by apcupsd?
If you call the script manually from command line. ^_^
>
>> + (err "Please report to bug-guix@gnu.org.~%")
>> + (exit #f))))
>> + (args
>> + (err "Unknown arguments: ~a~%" args)
>> + (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
>
> s/Iff/If/
Same here.
>> + (err "Please report to bug-guix@gnu.org.~%")
>> + (exit #f))))))
>> +
>> +(define (apcupsd-script-dir cfg)
>
> s/cfg/config/
Done.
>
>> + (computed-file
>> + "apcupsd-script-dir"
>> + #~(begin
>> + (mkdir #$output)
>> + (chdir #$output)
>> + (symlink #$(s/apccontrol cfg) "apccontrol"))))
>> +
>> +(define (apcupsd-config-file cfg)
>
> s/cfg/config/
Done.
>
>> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))
>> + (mixed-text-file
>> + "apcupsd.conf"
>> + "\
>> +## apcupsd.conf v1.1 ##
>> +#
>> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix
>
> Is this config really bound to a version like above? Unless they change
> things in backward incompatible ways, it should work for newer ones too,
> no?
This file header is taken from the official configuration file
distributed with the apcupsd (except the GNU Guix part). So while I do
expect that this configuration file should work with newer version, the
upstream decided to put the version into the configuration file.
I have no strong opinion either way here, should I just remove the file
header completely?
>
>> +#
>> +# \"apcupsd\" POSIX config file (crafted via apcupsd-service-type)
>
> s/crafted via/auto-generated by/
But, but, "crafted" sound way more fancy. :) Well, replaced anyway.
However I have used just "generated by", the "auto-" part felt bit off.
>
>> +"
>> + (serialize-configuration cfg apcupsd-configuration-fields)
>> + ;; This one is confusing. The manual page states:
>> + ;;
>> + ;; > It must be changed when running more than one copy of apcupsd on the
>> + ;; > same computer to control multiple UPSes.
>> + ;;
>> + ;; However would you not want the lock to be per-device, not per-process?
>> + ;; I decided to follow the documentation, but I do not understand why it
>> + ;; should be like this. I do not have multiple UPSes to try.
>> + (serialize-string 'lock-dir (string-append run-dir "/lock"))
>> + (serialize-string 'power-fail-dir run-dir)
>> + (serialize-string 'no-login-dir run-dir)
>> + (serialize-string 'stat-file (string-append run-dir "/apcupsd.status"))
>> + "SCRIPTDIR " (apcupsd-script-dir cfg) "\n")))
>> +
>> +(define (apcupsd-shepherd-services cfg)
>
> s/cfg/config/
And done.
>
>> + (match-record cfg <apcupsd-configuration>
>> + ( package pid-file debug-level run-dir
> ^ extraneous space
This is intentional, it is neat little trick I have learned recently.
Without the extra leading space, Emacs indents it in this way:
--8<---------------cut here---------------start------------->8---
(match-record config <apcupsd-configuration>
(apcupsd pid-file debug-level run-dir
shepherd-base-name auto-start?)
--8<---------------cut here---------------end--------------->8---
However, with the extra leading space, the indentation changes to:
--8<---------------cut here---------------start------------->8---
(match-record config <apcupsd-configuration>
( apcupsd pid-file debug-level run-dir
shepherd-base-name auto-start?)
--8<---------------cut here---------------end--------------->8---
Since in this case it is not a procedure call, but a list, first member
(apcupsd) is not special in any way. So it feels weird for it to have a
special priority regarding the indentation. With the extra leading
space, all list members are equal, as they should be.
>
>> + shepherd-base-name auto-start?)
>> + (let* ((config-file (apcupsd-config-file cfg))
>> + (s/ shepherd-base-name)
>
> Please improve the variables names a bit; what is 's/' ?
I removed them completely, with activation-service-type they are not
needed at all now.
>
>> + (s/run-dirs (string->symbol (format #f "~a-run-dirs" s/))))
>> + (list
>> + (shepherd-service
>> + (documentation "Create the run directories.")
>> + (provision (list s/run-dirs))
>> + (one-shot? #t)
>> + (auto-start? auto-start?)
>> + (start #~(lambda _
>> + ((@ (guix build utils) mkdir-p)
>> + #$(string-append run-dir "/lock"))
>> + #t)))
>
> Is there a reason why this is not done in an activation script instead?
> That's more conventional, I think.
The reason is simple, I forgot that activation-service-type exists. ^_^
I have converted the code to use it.
>
>> + (shepherd-service
>> + (documentation "Run apcupsd daemon.")
>
> Run *the* apcupsd daemon.
Done.
>
>> + (requirement (list s/run-dirs))
>> + (provision (list s/))
>> + (auto-start? auto-start?)
>> + (start #~(make-forkexec-constructor
>> + '(#$(file-append package "/sbin/apcupsd")
>> + "-b" ; Do not daemonize.
>
> If you want, margin comments are ok without space or full sentence
> (punctuation), e.g.:
>
> ;do not daemonize
I have no strong preference. What is the convention in Guix/Guile
community? Or is this down to each individual programmer?
>
>
>> + "-f" #$config-file
>> + "-P" #$pid-file
>> + "-d" #$(number->string debug-level))
>> + #:log-file
>> + #$(format #f "/var/log/~a.log" shepherd-base-name)
>> + #:environment-variables
>> + (cons* (string-append "GUIX_APCUPSD_CONF="
>> + #$config-file)
>> + #$(string-append "GUIX_APCUPSD_POWERFAIL_FILE="
>> + run-dir "/powerfail")
>> + (default-environment-variables))))
>> + (stop #~(make-kill-destructor))
>> + (actions (list (shepherd-configuration-action config-file))))))))
>> +
>> +(define (apcupsd-pam-extensions cfg)
>
> s/cfg/config/
Done.
>
>> + (define pam-nologin
>
> A comment explaining why this PAM extention is required would be helpful
> (I have no idea myself -- but I must confess: PAM is still a bit of a
> mystery to me).
I have added:
--8<---------------cut here---------------start------------->8---
;; The apcupsd can be configured to prevent users from logging in on certain
;; conditions. This is implemented by creation of a "nologin" file, and
;; using a pam nologin module to prevent the login (if the file exists).
--8<---------------cut here---------------end--------------->8---
>
>> + (pam-entry
>> + (control "required")
>> + (module "pam_nologin.so")
>> + (arguments (list (string-append "file="
>> + (apcupsd-configuration-run-dir cfg)
>> + "/nologin")))))
>> +
>> + (list (pam-extension
>> + (transformer
>> + (lambda (pam)
>> + (pam-service
>> + (inherit pam)
>> + (auth (cons pam-nologin (pam-service-auth pam)))))))))
>> +
>> +(define apcupsd-service-type
>> + (service-type
>> + (name 'apcupsd)
>> + (description "Configure and optionally start apcupsd.")
>> + (extensions (list (service-extension shepherd-root-service-type
>> + apcupsd-shepherd-services)
>> + (service-extension pam-root-service-type
>> + apcupsd-pam-extensions)))
>> + (compose identity)
>> + (extend (λ (cfg lst)
>
> nitpick: There was a stylistic choice to prefer lambda instead of λ in
> the Guix code base to try to make it more accessible at some point.
Nice, thanks for catching it. I am aware, but sometimes I miss one or
two when copying code from my channel.
>
>> + (fold (cut <> <>) cfg lst)))
>> + (default-value (apcupsd-configuration))))
>
> Thanks for this very complete, carefully and thoughtfully crafted
> contribution. As you can see, the only comments I had were mostly
> nitpicks and small details. I look forward to the next revision.
And thank you for the very thorough review. It is mostly details, but
it still surprised me how many I have managed to get wrong.
Thanks,
Tomas
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
[...]
>> I see there's already a gnu/services/pm.scm file; it seems we don't need
>> another one? Or was it a conscious decision?
>
> Yes, it was intentional, for couple of reasons (in no particular order).
>
> I like the symmetry between gnu/{packages,services}/power.scm. It makes
> finding the service-type easy without even opening the manual.
>
> The "pm" is for "power management", and I have never heard anyone call
> apcupsd "a power management daemon". It reacts to power outage, true,
> but it does not "manage" the power in any way. So it seemed like a poor
> fit.
>
> The current pm.scm exports 8 bindings, the new power.scm 54. So it
> would feel like taking over the file.
OK, that's reasonable, let's keep it (gnu services power).
> Shorter files are faster to compile (especially when
> define-configuration is used), so having two files that can be built in
> parallel seemed better.
>
> And like this I can also write the changelog in to the commit message in
> a reasonably short form. If I put it into pm.scm, I would need to list
> every single binding, in power.scm I mention just a new file.
Haha, yes, the last point is a funny 'loophole' of the change log
format. Change a couple things to a package? Needs a lot of
description. Add a new package? Two words suffice :-). It's
reasonable though (but still funny).
[...]
>>> +
>>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)
>>> +Should the shepherd service auto-start?
>>> +
>>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)
>>> +Path to the pid file.
>>
>> All file names becoming /run/ prefixed instead of /var/run, based on a
>> previous comment.
>
> I will just quote myself from the package's patch, since it seems I did
> not get an answer there:
>
> It seems pretty much all programs on my system are using /var/run,
> including Shepherd. Only programs that store information in /run seem
> to be elogind and dbus.
>
> Is deprecation of /var/run listed somewhere in the manual? I am
> surprised Shepherd (since it is actively maintained and core component)
> is still in /var/run. I looked and found 42 references to /var/run, but
> none with the deprecation notice.
>
> Assuming you will insist on moving it to /run, is there some structure
> to follow in that directory? Or just s~/var/run~/run~? What about /var
> and /var/log? Should those be moved somewhere as well?
Deprecating /var/run to /run is not a Guix thing, but a File Hiearchy
Standard (FHS) thing [0]. Since we already use /run, there's no good
reason to have /var/run something else than a symlink to /run, and
there's an actual change pending merge that would do that, along making
/run mounted as a tmpfs (bug#73494).
[0] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html
>
> Thanks.
>
>>
>> We don't use path in GNU to denote file names, we use 'file names'.
>> That's mentioned in info '(standards) GNU Manuals':
>>
>> Please do not use the term "pathname" that is used in Unix
>> documentation; use "file name" (two words) instead. We use the term
>> "path" only for search paths, which are lists of directory names.
>>
>
> Changed into "File name of the pid file.".
nitpick: s/pid/PID/
> Out of curiosity, does this apply to directories as well? So,
> hypothetically, I should instead of "Path to the directory storing XY."
> use "File name of the directory storing XY."?
Since a directory is implicitly a file, you can just say 'Directory
storing XY.'
>>> +
>>> +@item @code{debug-level} (default: @code{0}) (type: integer)
>>> +Logging verbosity. Bigger number means more logs. In the source code I
>>> +saw up to @code{300}, so for all logs, @code{999} seems reasonable.
>>
>> I think it's best to not use first person pronoun in the manual to keep
>> it a bit more formal. So instead of 'I saw', this could be reworded to:
>> 'The source code uses up to @code{300} as debug level value, so a value
>> of @code{999} seems reasonable to enable all logs.' or similar.
>
> Sorry, I have probably gotten too influenced by (gnus) and its personal
> style I have enjoyed a lot.
>
> I have used your suggestion verbatim.
Opinions seem to vary on which style is best, but in Guix I'm pretty
sure we've stuck to what I've described for the most part until now, so
we should continue, for consistency.
[...]
> Done. I went back to the package and replaced it there as well. It is
> bit unfortunate that autotools call the variables `ac_cv_path_', instead
> of `ac_cv_file_name_', but nothing I can do about that.
That's funny; the 'standards' Texinfo manual comes from autoconf, and is
the one documenting one. But autoconf is old, perhaps that guide line was
clarified later.
[...]
>> Also, any reason why these integer values are expressed as strings?
>
> No. I have just adhered to the original calling convention of the shell
> script, and argv is all char*. These are even not really an integer
> values, but bools. So I changed this one to `connected?', and the other
> one to `powered?', with #t/#f values.
>
>>
>>> +USB port). In most configurations, this will be the case. In the case
>>> +of a Slave machine where apcupsd is not directly connected to the UPS,
>>
>> s/Slave/slave/ ? Could be nicer to use a neutral alternative like
>> 'follower machine'.
>
> The "Slave machine" (including the capitalization) is a term used in the
> manual. I can lower-case it if you would prefer, seems that manual
> actually uses both casings now when I looked for it.
>
> I do not think inventing new terminology is a good idea, since users
> will not be able to find it in the manual distributed with the program.
The 'slave' word always carry a negative connotation. I think it's a
good idea to be proactive on modernizing the terminology where we can.
[...]
>> We use the unicode symbol (©) for copyright throughout Guix; please use
>> it too, for uniformity.
>
> So © is fine, but λ is not. I fail to see the consistency.
I guess that could be explained somewhere along those lines:
- λ is used in code; newcomers could be confused as to what it means, or
have to copy-paste it around thinking it's required by the syntax to
define anonymous procedures (while it's not -- they can just type
'lambda').
- © is just a widely understood symbol used in a text comment.
>>> +
>>> +;;;; Commentary:
>>> +
>>> +;;; Power-related services.
>>> +
>>> +;;;; Code:
>>> +
>>> +(define-module (gnu services power)
>>> + #:use-module (srfi srfi-1)
>>> + #:use-module (srfi srfi-26)
>>> + #:use-module (ice-9 match)
>>> + #:use-module (gnu)
>>> + #:use-module (gnu packages admin)
>>> + #:use-module (gnu packages linux)
>>> + #:use-module (gnu packages power)
>>> + #:use-module (gnu services)
>>> + #:use-module (gnu services configuration)
>>> + #:use-module (gnu services shepherd)
>>> + #:use-module (guix packages)
>>> + #:use-module (guix records)
>>
>> Please order lexicographically (that is, alphabetically but on things
>> which are not necessarily words :-)).
>
> Interesting. I personally order imports lexicographically, but within
> groups of:
>
> 1. stdlib (srfi, rnrs, ...)
> 2. guile extensions (ice-9, ...)
> 3. 3rd-party dependencies (guile-lib, guile-json, ...)
> 4. modules from the program itself (gnu, guix, ...)
That's similar to what I'd do in C or C++, except I'd reverse it so that
I can catch missing includes from my own files.
In Guix I've never read we do that. Perhaps it'd make sense. But
currently we typically globally sort lexicographically, I think (but
many modules imports are in a messy state).
> Given your requirement for sorting everything lexicographically (with no
> grouping), the following:
>
> (use-modules (gnu services)
> (srfi srfi-1))
>
> (when #f
> (modify-services '()
> (delete 'x)))
>
>
> Prints a warning when compiled:
>
> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'
>
> However, when done "my" way (putting srfi-1 first, since "stdlib"), the
> warning goes away.
That's an interesting workaround, but I think we should look into that
problem and fix it definitely I often used #:hidden (delete) without any
issue it seems, so I'm not even sure if its existence still solves a
true problem in current Guile.
[...]
>>> +(define-configuration/no-serialization apcupsd-event-handlers
>>> + (killpower
>>> + (gexp
>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
>>> + (sleep 10)
>>> + (apcupsd "--killpower")
>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
>>> + "Handler for killpower event.")
>>
By the way, why do we need to sleep 10 seconds here? It seems unnecessary.
[...]
>> Sounds fun :-). I was wondering if the handlers could be of the
>> maybe-gexp type, and when not provided the handler would not be called
>> at all? Or does the design of apcupsd mandates that all handlers be
>> defined?
>
> The control script is called for all events. So I *could* use
> maybe-gexp, and change
>
> ("battattach"
> #$@(apcupsd-event-handlers-battattach
> (apcupsd-configuration-event-handlers cfg)))
>
>
> into
>
> ("battattach"
> #$@(let ((h (apcupsd-event-handlers-battattach
> (apcupsd-configuration-event-handlers cfg))))
> (if (maybe-value-set? h)
> h
> #~(#t))))
>
>
> and do that for all the events. But I have to admit I do not consider
> that an improvement. There probably is a better way to express that,
> but the current approach (of always having, possibly NOP, handler
> defined) helps keep the code straight forward.
OK; we could probably script the control script differently, but let's
keep what we have so far, it's simple and I doubt performance is an
issue ;-).
>>> +(define-syntax define-enum
>>> + (lambda (x)
>>> + (syntax-case x ()
>>> + ((_ name values)
>>> + (let* ((d/n (syntax->datum #'name))
>>> + (d/predicate (string->symbol
>>> + (format #f "enum-~a?" d/n)))
>>> + (d/serialize (string->symbol
>>> + (format #f "serialize-enum-~a" d/n))))
>>
>> Even for internal bindings, better variable names would help reading the
>> code.
>>
>
> When I am writing syntax-case, I always struggle with the naming, since
> I effectively have to have two variables for a single thing. One for
> datum, and one for syntax.
>
> I have expanded the d/ info full datum/, however if can suggest better
> naming scheme for situations like this, I am all ears.
I'd stick to describe what they really are, e.g. for the enum-$name?
symbol, 'predicate-name' or 'predicate-variable-name' instead of
d/predicate. When deeply nested, it's OK to abbreviate the names a bit
if it becomes difficult to fit in our 80 columns width, so it could be
'pred-var-name' or the likes.
[...]
>>> + (shepherd-base-name
>>> + (symbol 'apcupsd)
>>> + "Base name of the shepherd service. You can add the service multiple times
>>> +with different prefix to manage multiple UPSes."
>>> + empty-serializer)
>>> + (auto-start?
>>> + (boolean #t)
>>> + "Should the shepherd service auto-start?"
>>> + empty-serializer)
>>> + (pid-file
>>> + (string "/var/run/apcupsd.pid")
>>
>> /var/run -> /run (adjust everywhere)
>
> See the reaction above regarding the /var/run.
Which I've now covered in my reply above (sorry for missing in in my
first replies).
[...]
> The s/ was supposed to stand for script/, I originally expected I will
> have more than one. I have renamed it to %apccontrol.
>
>>
>>> + (program-file
>>> + "apccontrol"
>>> + #~(begin
>>> + (use-modules (srfi srfi-9)
>>> + (ice-9 format)
>>> + (ice-9 match)
>>> + (ice-9 popen))
>>
>> Please sort modules lexicographically.
>
> See the reaction above.
I've replied above. I'd suggest to stick to lexicographically sort the
whole imports block to stick with current conventions (although I don't
think this particular one is documented).
>>> + ;; Script dir depends on these, and the configuration depends on the script
>>> + ;; dir. To sever the cyclic dependency, pass the paths via environment
>>> + ;; variables.
>>> + (define conf (getenv "GUIX_APCUPSD_CONF"))
>>> + (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
>>> +
>>> + (define (err . args)
>>> + (apply format (current-error-port) args))
>>> + (define (wall . args)
>>> + (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
>>> + (define (apcupsd . args)
>>> + (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
>>> + (define (mail-to-root subject body)
>>> + (let ((port (open-pipe* OPEN_WRITE
>>> + "/run/privileged/bin/sendmail"
>>> + "-F" "apcupsd"
>>> + "root")))
>>> + (format port "Subject: ~a~%~%~a~&" subject body)
>>> + (close-pipe port)))
>>> + (match (cdr (command-line))
>>> + (((? string? cmd) name connected powered)
>>> + (match cmd
>>> + ;; I am sure this could be done by macro, but meh. Last release of
>>> + ;; apcupsd was in 2016, so maintaining this will not be much work.
>>> + ("killpower"
>>> + #$@(apcupsd-event-handlers-killpower
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("commfailure"
>>> + #$@(apcupsd-event-handlers-commfailure
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("commok"
>>> + #$@(apcupsd-event-handlers-commok
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("powerout"
>>> + #$@(apcupsd-event-handlers-powerout
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("onbattery"
>>> + #$@(apcupsd-event-handlers-onbattery
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("offbattery"
>>> + #$@(apcupsd-event-handlers-offbattery
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("mainsback"
>>> + #$@(apcupsd-event-handlers-mainsback
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("failing"
>>> + #$@(apcupsd-event-handlers-failing
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("timeout"
>>> + #$@(apcupsd-event-handlers-timeout
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("loadlimit"
>>> + #$@(apcupsd-event-handlers-loadlimit
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("runlimit"
>>> + #$@(apcupsd-event-handlers-runlimit
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("doreboot"
>>> + #$@(apcupsd-event-handlers-doreboot
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("doshutdown"
>>> + #$@(apcupsd-event-handlers-doshutdown
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("annoyme"
>>> + #$@(apcupsd-event-handlers-annoyme
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("emergency"
>>> + #$@(apcupsd-event-handlers-emergency
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("changeme"
>>> + #$@(apcupsd-event-handlers-changeme
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("remotedown"
>>> + #$@(apcupsd-event-handlers-remotedown
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("startselftest"
>>> + #$@(apcupsd-event-handlers-startselftest
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("endselftest"
>>> + #$@(apcupsd-event-handlers-endselftest
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("battdetach"
>>> + #$@(apcupsd-event-handlers-battdetach
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("battattach"
>>> + #$@(apcupsd-event-handlers-battattach
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + (_
>>> + (err "Unknown event: ~a~%" cmd)
>>> + (err "Iff the event was passed by apcupsd, this is a bug.~%")
I just thought about it, perhaps it could be more concise to extract the
field values using match-record (these are bound to shorter names, since
they omit the 'apcupsd-configuration-' prefix).
[...]
>> I don't understand, in which scenario would an event *not* be emitted
>> by apcupsd?
>
> If you call the script manually from command line. ^_^
I see!
[...]
>>> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))
>>> + (mixed-text-file
>>> + "apcupsd.conf"
>>> + "\
>>> +## apcupsd.conf v1.1 ##
>>> +#
>>> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix
>>
>> Is this config really bound to a version like above? Unless they change
>> things in backward incompatible ways, it should work for newer ones too,
>> no?
>
> This file header is taken from the official configuration file
> distributed with the apcupsd (except the GNU Guix part). So while I do
> expect that this configuration file should work with newer version, the
> upstream decided to put the version into the configuration file.
>
> I have no strong opinion either way here, should I just remove the file
> header completely?
I think so, since we're going the path of producing our own, we don't
need to keep things that are not deemed useful.
>>> +#
>>> +# \"apcupsd\" POSIX config file (crafted via apcupsd-service-type)
>>
>> s/crafted via/auto-generated by/
>
> But, but, "crafted" sound way more fancy. :) Well, replaced anyway.
> However I have used just "generated by", the "auto-" part felt bit off.
Eheh. Good!
>>> +"
>>> + (serialize-configuration cfg apcupsd-configuration-fields)
>>> + ;; This one is confusing. The manual page states:
>>> + ;;
>>> + ;; > It must be changed when running more than one copy of apcupsd on the
>>> + ;; > same computer to control multiple UPSes.
>>> + ;;
>>> + ;; However would you not want the lock to be per-device, not per-process?
>>> + ;; I decided to follow the documentation, but I do not understand why it
>>> + ;; should be like this. I do not have multiple UPSes to try.
>>> + (serialize-string 'lock-dir (string-append run-dir "/lock"))
>>> + (serialize-string 'power-fail-dir run-dir)
>>> + (serialize-string 'no-login-dir run-dir)
>>> + (serialize-string 'stat-file (string-append run-dir "/apcupsd.status"))
>>> + "SCRIPTDIR " (apcupsd-script-dir cfg) "\n")))
>>> +
>>> +(define (apcupsd-shepherd-services cfg)
>>
>> s/cfg/config/
>
> And done.
>
>>
>>> + (match-record cfg <apcupsd-configuration>
>>> + ( package pid-file debug-level run-dir
>> ^ extraneous space
>
> This is intentional, it is neat little trick I have learned recently.
> Without the extra leading space, Emacs indents it in this way:
>
> (match-record config <apcupsd-configuration>
> (apcupsd pid-file debug-level run-dir
> shepherd-base-name auto-start?)
>
>
> However, with the extra leading space, the indentation changes to:
>
> (match-record config <apcupsd-configuration>
> ( apcupsd pid-file debug-level run-dir
> shepherd-base-name auto-start?)
>
>
> Since in this case it is not a procedure call, but a list, first member
> (apcupsd) is not special in any way. So it feels weird for it to have a
> special priority regarding the indentation. With the extra leading
> space, all list members are equal, as they should be.
I see; that's an issue with our .dir-locals.el indentation rules. It
should be fixed there, not worked around in actual code, though I can
that workaround documented in 'info "(emacs) Lisp Indent"', for lists.
In theory, we could define a function and assign it to the
lisp-indent-function property (see .dir-locals.el), per 'C-h f
scheme-indent-function' in Emacs to do whatever we like. While an
interesting side project, I think using the workaround you propose is
fine until then :-).
[...]
>> If you want, margin comments are ok without space or full sentence
>> (punctuation), e.g.:
>>
>> ;do not daemonize
>
> I have no strong preference. What is the convention in Guix/Guile
> community? Or is this down to each individual programmer?
Our coding style in general comes from
https://mumble.net/~campbell/scheme/style.txt (info '(guix) Formatting
Code'), which mentions the above but leaves the choice. Since Ludo uses
the above style, and authored most of the original code, it's the
prevalent style in the code base and I suggest we stick to it, for
consistency.
[...]
> I have added:
>
> ;; The apcupsd can be configured to prevent users from logging in on certain
> ;; conditions. This is implemented by creation of a "nologin" file, and
> ;; using a pam nologin module to prevent the login (if the file exists).
Thanks, that helps!
I'll let you take care of the small remaining things, and probably the
next revision will be good to go.
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>>>> +
>>>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)
>>>> +Should the shepherd service auto-start?
>>>> +
>>>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)
>>>> +Path to the pid file.
>>>
>>> All file names becoming /run/ prefixed instead of /var/run, based on a
>>> previous comment.
>>
>> I will just quote myself from the package's patch, since it seems I did
>> not get an answer there:
>>
>> It seems pretty much all programs on my system are using /var/run,
>> including Shepherd. Only programs that store information in /run seem
>> to be elogind and dbus.
>>
>> Is deprecation of /var/run listed somewhere in the manual? I am
>> surprised Shepherd (since it is actively maintained and core component)
>> is still in /var/run. I looked and found 42 references to /var/run, but
>> none with the deprecation notice.
>>
>> Assuming you will insist on moving it to /run, is there some structure
>> to follow in that directory? Or just s~/var/run~/run~? What about /var
>> and /var/log? Should those be moved somewhere as well?
>
> Deprecating /var/run to /run is not a Guix thing, but a File Hiearchy
> Standard (FHS) thing [0]. Since we already use /run, there's no good
> reason to have /var/run something else than a symlink to /run, and
> there's an actual change pending merge that would do that, along making
> /run mounted as a tmpfs (bug#73494).
>
> [0] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html
I did not know we actually care about FHS (for example our /usr/bin
definitely does not look like it should in that case :) ).
I did the s|/var/run|/run|.
>
> [..]
>
>>>
>>> s/Slave/slave/ ? Could be nicer to use a neutral alternative like
>>> 'follower machine'.
>>
>> The "Slave machine" (including the capitalization) is a term used in the
>> manual. I can lower-case it if you would prefer, seems that manual
>> actually uses both casings now when I looked for it.
>>
>> I do not think inventing new terminology is a good idea, since users
>> will not be able to find it in the manual distributed with the program.
>
> The 'slave' word always carry a negative connotation. I think it's a
> good idea to be proactive on modernizing the terminology where we can.
In that case I would suggest to contact the upstream and convince them
regarding this topic. Once that happens, we will be able to adjust the
documentation with the next release (which would be free of offensive
terminology).
In any case, this is not a first occurrence of the word "slave" in the
Guix repository nor manual.
>> Given your requirement for sorting everything lexicographically (with no
>> grouping), the following:
>>
>> (use-modules (gnu services)
>> (srfi srfi-1))
>>
>> (when #f
>> (modify-services '()
>> (delete 'x)))
>>
>>
>> Prints a warning when compiled:
>>
>> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'
>>
>> However, when done "my" way (putting srfi-1 first, since "stdlib"), the
>> warning goes away.
>
> That's an interesting workaround, but I think we should look into that
> problem and fix it definitely I often used #:hidden (delete) without any
> issue it seems, so I'm not even sure if its existence still solves a
> true problem in current Guile.
I have recently (~few months back) tried to remove the `delete' and it a
wall quickly, so I assume for reasons I do not understand it is still
required.
Having to use #:hidden (delete) on every single relevant import is in my
opinion much worse solution than just ordering imports with srfi-1 at
the top.
But, I concur, it would be best it this just worked in any order.
Anyway, I have sorted the modules as requested. I am not sure whether
to sort
--8<---------------cut here---------------start------------->8---
#$@(apcupsd-event-handlers-modules
(apcupsd-configuration-event-handlers cfg))
--8<---------------cut here---------------end--------------->8---
Under `a' or `m' though. Opinion?
>
> [...]
>
>>>> +(define-configuration/no-serialization apcupsd-event-handlers
>>>> + (killpower
>>>> + (gexp
>>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
>>>> + (sleep 10)
>>>> + (apcupsd "--killpower")
>>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
>>>> + "Handler for killpower event.")
>>>
>
> By the way, why do we need to sleep 10 seconds here? It seems
> unnecessary.
All default handlers are just conversions of the official apccontrol
script, and it has the sleep in there. I am not sure why, so I have
decided it would be better to leave it as it was.
> I just thought about it, perhaps it could be more concise to extract the
> field values using match-record (these are bound to shorter names, since
> they omit the 'apcupsd-configuration-' prefix).
If I got the idea right, you suggest converting
--8<---------------cut here---------------start------------->8---
("emergency"
#$@(apcupsd-event-handlers-emergency
(apcupsd-configuration-event-handlers cfg)))
--8<---------------cut here---------------end--------------->8---
into
--8<---------------cut here---------------start------------->8---
("emergency"
#$@(match-record cfg <apcupsd-configuration> (handlers)
(match-record handlers <apcupsd-event-handlers> (emergency)
emergency)))
--8<---------------cut here---------------end--------------->8---
I am not sure it is increase in readability, but maybe I misunderstood
the suggestion.
>>>> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))
>>>> + (mixed-text-file
>>>> + "apcupsd.conf"
>>>> + "\
>>>> +## apcupsd.conf v1.1 ##
>>>> +#
>>>> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix
>>>
>>> Is this config really bound to a version like above? Unless they change
>>> things in backward incompatible ways, it should work for newer ones too,
>>> no?
>>
>> This file header is taken from the official configuration file
>> distributed with the apcupsd (except the GNU Guix part). So while I do
>> expect that this configuration file should work with newer version, the
>> upstream decided to put the version into the configuration file.
>>
>> I have no strong opinion either way here, should I just remove the file
>> header completely?
>
> I think so, since we're going the path of producing our own, we don't
> need to keep things that are not deemed useful.
I have removed the version, but kept the header, so that there is
reference to apcupsd-service-type left in the file.
>>>> + (match-record cfg <apcupsd-configuration>
>>>> + ( package pid-file debug-level run-dir
>>> ^ extraneous space
>>
>> This is intentional, it is neat little trick I have learned recently.
>> Without the extra leading space, Emacs indents it in this way:
>>
>> (match-record config <apcupsd-configuration>
>> (apcupsd pid-file debug-level run-dir
>> shepherd-base-name auto-start?)
>>
>>
>> However, with the extra leading space, the indentation changes to:
>>
>> (match-record config <apcupsd-configuration>
>> ( apcupsd pid-file debug-level run-dir
>> shepherd-base-name auto-start?)
>>
>>
>> Since in this case it is not a procedure call, but a list, first member
>> (apcupsd) is not special in any way. So it feels weird for it to have a
>> special priority regarding the indentation. With the extra leading
>> space, all list members are equal, as they should be.
>
> I see; that's an issue with our .dir-locals.el indentation rules. It
> should be fixed there, not worked around in actual code, though I can
> that workaround documented in 'info "(emacs) Lisp Indent"', for lists.
> In theory, we could define a function and assign it to the
> lisp-indent-function property (see .dir-locals.el), per 'C-h f
> scheme-indent-function' in Emacs to do whatever we like. While an
> interesting side project, I think using the workaround you propose is
> fine until then :-).
Getting bit off topic, but I am not sure how that would work. Between
various macros, and Scheme being LISP-1, I have no idea how to write
robust code that would just magically always indent "right".
But I would be glad to be proven wrong. :)
>
> [...]
>
>>> If you want, margin comments are ok without space or full sentence
>>> (punctuation), e.g.:
>>>
>>> ;do not daemonize
>>
>> I have no strong preference. What is the convention in Guix/Guile
>> community? Or is this down to each individual programmer?
>
> Our coding style in general comes from
> https://mumble.net/~campbell/scheme/style.txt (info '(guix) Formatting
> Code'), which mentions the above but leaves the choice. Since Ludo uses
> the above style, and authored most of the original code, it's the
> prevalent style in the code base and I suggest we stick to it, for
> consistency.
Sure, adjusted.
>
>> I have added:
>>
>> ;; The apcupsd can be configured to prevent users from logging in on certain
>> ;; conditions. This is implemented by creation of a "nologin" file, and
>> ;; using a pam nologin module to prevent the login (if the file exists).
>
> Thanks, that helps!
>
> I'll let you take care of the small remaining things, and probably the
> next revision will be good to go.
One additional change I have made is to change
--8<---------------cut here---------------start------------->8---
(requirement '())
--8<---------------cut here---------------end--------------->8---
into
--8<---------------cut here---------------start------------->8---
(requirement '(user-processes))
--8<---------------cut here---------------end--------------->8---
Without the dependency, the daemon got respawned by shepherd on shutdown
and prevented the machine from going offline, which sucked a bit. ^_^
I thought I would mention it so that you can keep it in mind for future
code reviews.
With that I think I am out of things that I want to change or would
appreciate feedback on, so I am sending a v2. Thank you for working
with me through this, the code is much better than in v1.
Tomas
@@ -123,7 +123,7 @@
Copyright @copyright{} 2023 Thomas Ieong@*
Copyright @copyright{} 2023 Saku Laesvuori@*
Copyright @copyright{} 2023 Graham James Addis@*
-Copyright @copyright{} 2023, 2024 Tomas Volf@*
+Copyright @copyright{} 2023-2025 Tomas Volf@*
Copyright @copyright{} 2024 Herman Rimm@*
Copyright @copyright{} 2024 Matthew Trzcinski@*
Copyright @copyright{} 2024 Richard Sent@*
@@ -420,7 +420,7 @@ Top
* Network File System:: NFS related services.
* Samba Services:: Samba services.
* Continuous Integration:: Cuirass and Laminar services.
-* Power Management Services:: Extending battery life.
+* Power Management Services:: Extending battery life, etc.
* Audio Services:: The MPD.
* Virtualization Services:: Virtualization services.
* Version Control Services:: Providing remote access to Git repositories.
@@ -19255,7 +19255,7 @@ Services
* Network File System:: NFS related services.
* Samba Services:: Samba services.
* Continuous Integration:: Cuirass and Laminar services.
-* Power Management Services:: Extending battery life.
+* Power Management Services:: Extending battery life, etc.
* Audio Services:: The MPD.
* Virtualization Services:: Virtualization services.
* Version Control Services:: Providing remote access to Git repositories.
@@ -36263,6 +36263,374 @@ Power Management Services
@end table
@end deftp
+The @code{(gnu services power)} module provides a service definition for
+@uref{http://www.apcupsd.org/, apcupsd}, a utility to interact with APC
+UPSes. Apcupsd also works with some OEM-branded products manufactured
+by APC.
+
+@defvar apcupsd-service-type
+The service type for apcupsd. For USB UPSes no configuration is
+necessary, however tweaking some fields to better suit your needs might
+be desirable. The defaults are taken from the upstream configuration
+and they are not very conservative (@code{battery-level} of 5% is too
+low in my opinion).
+
+The default event handlers do send emails, read more in
+@ref{apcupsd-event-handlers}.
+
+@lisp
+(service apcupsd-service-type)
+@end lisp
+@end defvar
+
+@deftp {Data Type} apcupsd-configuration
+
+Available @code{apcupsd-configuration} fields are:
+
+@table @asis
+@item @code{package} (default: @code{apcupsd}) (type: package)
+Package to use.
+
+@item @code{shepherd-base-name} (default: @code{apcupsd}) (type: symbol)
+Base name of the shepherd service. You can add the service multiple
+times with different prefix to manage multiple UPSes.
+
+@item @code{auto-start?} (default: @code{#t}) (type: boolean)
+Should the shepherd service auto-start?
+
+@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)
+Path to the pid file.
+
+@item @code{debug-level} (default: @code{0}) (type: integer)
+Logging verbosity. Bigger number means more logs. In the source code I
+saw up to @code{300}, so for all logs, @code{999} seems reasonable.
+
+@item @code{run-dir} (default: @code{"/var/run/apcupsd"}) (type: string)
+Directory containing runtime information. You need to change this if
+you desire to run multiple instances of the daemon.
+
+@item @code{name} (type: maybe-string)
+Use this to give your UPS a name in log files and such. This is
+particularly useful if you have multiple UPSes. This does not set the
+EEPROM. It should be 8 characters or less.
+
+@item @code{cable} (default: @code{usb}) (type: enum-cable)
+The type of cable connecting the UPS to your computer. Possible generic
+choices are @code{'simple}, @code{'smart}, @code{'ether} and
+@code{'usb}. Or a specific cable model number may be used:
+@code{'940-0119A}, @code{'940-0127A}, @code{'940-0128A},
+@code{'940-0020B}, @code{'940-0020C}, @code{'940-0023A},
+@code{'940-0024B}, @code{'940-0024C}, @code{'940-1524C},
+@code{'940-0024G}, @code{'940-0095A}, @code{'940-0095B},
+@code{'940-0095C}, @code{'940-0625A}, @code{'M-04-02-2000}.
+
+@item @code{type} (default: @code{usb}) (type: enum-type)
+Type of the UPS you have.
+
+@table @code
+@item apcsmart
+Newer serial character device, appropriate for SmartUPS models using a
+serial cable (not USB).
+
+@item usb
+Most new UPSes are USB.
+
+@item net
+Network link to a master apcupsd through apcupsd's Network Information
+Server. This is used if the UPS powering your computer is connected to
+a different computer for monitoring.
+
+@item snmp
+SNMP network link to an SNMP-enabled UPS device.
+
+@item netsnmp
+Same as SNMP above but requires use of the net-snmp library. Unless you
+have a specific need for this old driver, you should use @code{'snmp}
+instead.
+
+@item dumb
+Old serial character device for use with simple-signaling UPSes.
+
+@item pcnet
+PowerChute Network Shutdown protocol which can be used as an alternative
+to SNMP with the AP9617 family of smart slot cards.
+
+@item modbus
+Serial device for use with newest SmartUPS models supporting the MODBUS
+protocol.
+
+@end table
+
+@item @code{device} (default: @code{""}) (type: string)
+For USB UPSes, usually you want to set this to an empty string (the
+default). For other UPS types, you must specify an appropriate port or
+address.
+
+@table @code
+@item apcsmart
+Set to the appropriate @file{/dev/tty**} device.
+
+@item usb
+A null string setting enables autodetection, which is the best choice
+for most installations.
+
+@item net
+Set to @code{@var{hostname}:@var{port}}.
+
+@item snmp
+Set to @code{@var{hostname}:@var{port}:@var{vendor}:@var{community}}.
+@var{hostname} is the ip address or hostname of the UPS on the network.
+@var{vendor} can be can be "APC" or "APC_NOTRAP". "APC_NOTRAP" will
+disable SNMP trap catching; you usually want "APC". @var{port} is
+usually 161. @var{community} is usually "private".
+
+@item netsnmp
+Same as @code{'snmp}.
+
+@item dumb
+Set to the appropriate @file{/dev/tty**} device.
+
+@item pcnet
+Set to @code{@var{ipaddr}:@var{username}:@var{passphrase}:@var{port}}.
+@var{ipaddr} is the IP address of the UPS management card.
+@var{username} and @var{passphrase} are the credentials for which the
+card has been configured. @var{port} is the port number on which to
+listen for messages from the UPS, normally 3052. If this parameter is
+empty or missing, the default of 3052 will be used.
+
+@item modbus
+Set to the appropriate @file{/dev/tty**} device. You can also leave it
+empty for MODBUS over USB or set to the serial number of the UPS.
+
+@end table
+
+@item @code{poll-time} (default: @code{60}) (type: integer)
+Interval (in seconds) at which apcupsd polls the UPS for status. This
+setting applies both to directly-attached UPSes (apcsmart, usb, dumb)
+and networked UPSes (net, snmp). Lowering this setting will improve
+apcupsd's responsiveness to certain events at the cost of higher CPU
+utilization.
+
+@item @code{on-batery-delay} (default: @code{6}) (type: integer)
+The the time in seconds from when a power failure is detected until we
+react to it with an onbattery event. The @code{'powerout} event will be
+triggered immediately when a power failure is detected. However, the
+@code{'onbattery} event will be trigger only after this delay.
+
+@item @code{battery-level} (default: @code{5}) (type: integer)
+If during a power failure, the remaining battery percentage (as reported
+by the UPS) is below or equal to this value, apcupsd will initiate a
+system shutdown.
+
+@quotation Note
+@code{battery-level}, @code{remaining-minutes}, and @code{timeout} work
+in conjunction, so the first that occurs will cause the initation of a
+shutdown.
+@end quotation
+
+@item @code{remaining-minutes} (default: @code{3}) (type: integer)
+If during a power failure, the remaining runtime in minutes (as
+calculated internally by the UPS) is below or equal to this value,
+apcupsd will initiate a system shutdown.
+
+@quotation Note
+@code{battery-level}, @code{remaining-minutes}, and @code{timeout} work
+in conjunction, so the first that occurs will cause the initation of a
+shutdown.
+@end quotation
+
+@item @code{timeout} (default: @code{0}) (type: integer)
+If during a power failure, the UPS has run on batteries for this many
+seconds or longer, apcupsd will initiate a system shutdown. A value of
+0 disables this timer.
+
+@quotation Note
+@code{battery-level}, @code{remaining-minutes}, and @code{timeout} work
+in conjunction, so the first that occurs will cause the initation of a
+shutdown.
+@end quotation
+
+@item @code{annoy-interval} (default: @code{300}) (type: integer)
+Time in seconds between annoying users (via the @code{'annoyme} event)
+to sign off prior to system shutdown. 0 disables.
+
+@item @code{annoy-delay} (default: @code{60}) (type: integer)
+Initial delay in seconds after power failure before warning users to get
+off the system.
+
+@item @code{no-logon} (default: @code{disable}) (type: enum-no-logon)
+The condition which determines when users are prevented from logging in
+during a power failure.
+
+@item @code{kill-delay} (default: @code{0}) (type: integer)
+If this is non-zero, apcupsd will continue running after a shutdown has
+been requested, and after the specified time in seconds attempt to kill
+the power. This is for use on systems where apcupsd cannot regain
+control after a shutdown.
+
+@item @code{net-server} (default: @code{#f}) (type: boolean)
+If enabled, a network information server process will be started.
+
+@item @code{net-server-ip} (default: @code{"127.0.0.1"}) (type: string)
+IP address on which NIS server will listen for incoming connections.
+
+@item @code{net-server-port} (default: @code{3551}) (type: integer)
+IP port on which NIS server will listen for incoming connections.
+
+@item @code{net-server-events-file} (type: maybe-string)
+If you want the last few EVENTS to be available over the network by the
+network information server, you must set this to a file patch.
+
+@item @code{net-server-events-file-max-size} (default: @code{10}) (type: integer)
+Maximum size of the events file in kilobytes.
+
+@item @code{class} (default: @code{standalone}) (type: enum-class)
+Normally standalone unless you share an UPS using an APC ShareUPS card.
+
+@item @code{mode} (default: @code{disable}) (type: enum-mode)
+Normally disable unless you share an UPS using an APC ShareUPS card.
+
+@item @code{stat-time} (default: @code{0}) (type: integer)
+Time interval in seconds between writing the status file, 0 disables.
+
+@item @code{log-stats} (default: @code{#f}) (type: boolean)
+Also write the stats as a logs. This generates a lot of output.
+
+@item @code{data-time} (default: @code{0}) (type: integer)
+Time interval in seconds between writing the data records to the log
+file, 0 disables.
+
+@item @code{facility} (type: maybe-string)
+The logging facility for the syslog.
+
+@item @code{event-handlers} (type: apcupsd-event-handlers)
+Handlers for events produced by apcupsd.
+
+@end table
+
+@end deftp
+
+@anchor{apcupsd-event-handlers}
+@deftp {Data Type} apcupsd-event-handlers
+
+For description of the events please refer to the @command{apcupsd}'s
+manual, which can be found in the @samp{apcupsd-doc} package.
+
+Each handler shall be a gexp. It is spliced into the control script for
+the daemon. In addition to the standard Guile programming environment,
+these procedures and variables are also available.
+
+@table @code
+@item conf
+Variable containing path to the configuration file.
+
+@item powerfail-file
+Variable containing path to the powerfail file.
+
+@item cmd
+The event currently being handled.
+
+@item name
+The name of the UPS as specified in the configuration file.
+
+@item connected
+Is @code{"1"} if apcupsd is connected to the UPS via a serial port (or a
+USB port). In most configurations, this will be the case. In the case
+of a Slave machine where apcupsd is not directly connected to the UPS,
+this value will be @code{"0"}.
+
+@item powered
+Is @code{"1"} if the computer on which @command{apcupsd} is running is
+powered by the UPS and @code{"0"} if not. At the moment, this value is
+unimplemented and always @code{"0"}.
+
+@item (err @var{fmt} @var{args...})
+Wrapper around @code{format} outputting to @code{(current-error-port)}.
+
+@item (wall @var{fmt} @var{args...})
+Wrapper around @code{format} outputting via @command{wall}.
+
+@item (apcupsd @var{args...})
+Call @command{apcupsd} while passing the correct configuration file and
+all the arguments.
+
+@item (mail-to-root @var{subject} @var{body})
+Send an email to the local administrator. This procedure assumes the
+@command{sendmail} is located at @command{/run/privileged/bin/sendmail}
+(as would be the case with @code{opensmtpd-service-type}).
+
+@end table
+
+Available @code{apcupsd-event-handlers} fields are:
+
+@table @asis
+@item @code{killpower} (type: gexp)
+Handler for killpower event.
+
+@item @code{commfailure} (type: gexp)
+Handler for commfailure event.
+
+@item @code{commok} (type: gexp)
+Handler for commfailure event.
+
+@item @code{powerout} (type: gexp)
+Handler for powerout event.
+
+@item @code{onbattery} (type: gexp)
+Handler for onbattery event.
+
+@item @code{offbattery} (type: gexp)
+Handler for offbattery event.
+
+@item @code{mainsback} (type: gexp)
+Handler for mainsback event.
+
+@item @code{failing} (type: gexp)
+Handler for failing event.
+
+@item @code{timeout} (type: gexp)
+Handler for timeout event.
+
+@item @code{loadlimit} (type: gexp)
+Handler for loadlimit event.
+
+@item @code{runlimit} (type: gexp)
+Handler for runlimit event.
+
+@item @code{doreboot} (type: gexp)
+Handler for doreboot event.
+
+@item @code{doshutdown} (type: gexp)
+Handler for doshutdown event.
+
+@item @code{annoyme} (type: gexp)
+Handler for annoyme event.
+
+@item @code{emergency} (type: gexp)
+Handler for emergency event.
+
+@item @code{changeme} (type: gexp)
+Handler for changeme event.
+
+@item @code{remotedown} (type: gexp)
+Handler for remotedown event.
+
+@item @code{startselftest} (type: gexp)
+Handler for startselftest event.
+
+@item @code{endselftest} (type: gexp)
+Handler for endselftest event.
+
+@item @code{battdetach} (type: gexp)
+Handler for battdetach event.
+
+@item @code{battattach} (type: gexp)
+Handler for battattach event.
+
+@end table
+
+@end deftp
+
@node Audio Services
@subsection Audio Services
@@ -758,6 +758,7 @@ GNU_SYSTEM_MODULES = \
%D%/services/nix.scm \
%D%/services/nfs.scm \
%D%/services/pam-mount.scm \
+ %D%/services/power.scm \
%D%/services/science.scm \
%D%/services/security.scm \
%D%/services/security-token.scm \
new file mode 100644
@@ -0,0 +1,690 @@
+;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>
+
+;;;; Commentary:
+
+;;; Power-related services.
+
+;;;; Code:
+
+(define-module (gnu services power)
+ #:use-module (srfi srfi-1)
+ #:use-module (srfi srfi-26)
+ #:use-module (ice-9 match)
+ #:use-module (gnu)
+ #:use-module (gnu packages admin)
+ #:use-module (gnu packages linux)
+ #:use-module (gnu packages power)
+ #:use-module (gnu services)
+ #:use-module (gnu services configuration)
+ #:use-module (gnu services shepherd)
+ #:use-module (guix packages)
+ #:use-module (guix records)
+ #:export (apcupsd-service-type
+
+ apcupsd-configuration
+ apcupsd-configuration-package
+ apcupsd-configuration-shepherd-base-name
+ apcupsd-configuration-auto-start?
+ apcupsd-configuration-pid-file
+ apcupsd-configuration-debug-level
+ apcupsd-configuration-run-dir
+ apcupsd-configuration-name
+ apcupsd-configuration-cable
+ apcupsd-configuration-type
+ apcupsd-configuration-device
+ apcupsd-configuration-poll-time
+ apcupsd-configuration-on-batery-delay
+ apcupsd-configuration-battery-level
+ apcupsd-configuration-remaining-minutes
+ apcupsd-configuration-timeout
+ apcupsd-configuration-annoy-interval
+ apcupsd-configuration-annoy-delay
+ apcupsd-configuration-no-logon
+ apcupsd-configuration-kill-delay
+ apcupsd-configuration-net-server
+ apcupsd-configuration-net-server-ip
+ apcupsd-configuration-net-server-port
+ apcupsd-configuration-net-server-events-file
+ apcupsd-configuration-net-server-events-file-max-size
+ apcupsd-configuration-class
+ apcupsd-configuration-mode
+ apcupsd-configuration-stat-time
+ apcupsd-configuration-log-stats
+ apcupsd-configuration-data-time
+ apcupsd-configuration-facility
+ apcupsd-configuration-event-handlers
+
+ apcupsd-event-handlers-annoyme
+ apcupsd-event-handlers-battattach
+ apcupsd-event-handlers-battdetach
+ apcupsd-event-handlers-changeme
+ apcupsd-event-handlers-commfailure
+ apcupsd-event-handlers-commok
+ apcupsd-event-handlers-doreboot
+ apcupsd-event-handlers-doshutdown
+ apcupsd-event-handlers-emergency
+ apcupsd-event-handlers-endselftest
+ apcupsd-event-handlers-failing
+ apcupsd-event-handlers-killpower
+ apcupsd-event-handlers-loadlimit
+ apcupsd-event-handlers-mainsback
+ apcupsd-event-handlers-offbattery
+ apcupsd-event-handlers-onbattery
+ apcupsd-event-handlers-powerout
+ apcupsd-event-handlers-remotedown
+ apcupsd-event-handlers-runlimit
+ apcupsd-event-handlers-startselftest
+ apcupsd-event-handlers-timeout))
+
+(define-configuration/no-serialization apcupsd-event-handlers
+ (killpower
+ (gexp
+ #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
+ (sleep 10)
+ (apcupsd "--killpower")
+ (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
+ "Handler for killpower event.")
+ (commfailure
+ (gexp
+ #~((let ((msg (format #f "~a Communications with UPS ~a lost."
+ (gethostname) name)))
+ (mail-to-root msg msg))
+ (wall "Warning: communications lost with UPS ~a" name)))
+ "Handler for commfailure event.")
+ (commok
+ (gexp
+ #~((let ((msg (format #f "~a Communications with UPS ~a restored."
+ (gethostname) name)))
+ (mail-to-root msg msg))
+ (wall "Communications restored with UPS ~a" name)))
+ "Handler for commfailure event.")
+ (powerout
+ (gexp
+ #~(#t))
+ "Handler for powerout event.")
+ (onbattery
+ (gexp
+ #~((let ((msg (format #f "~a UPS ~a Power Failure !!!"
+ (gethostname) name)))
+ (mail-to-root msg msg))
+ (wall "Power failure on UPS ~a. Running on batteries." name)))
+ "Handler for onbattery event.")
+ (offbattery
+ (gexp
+ #~((let ((msg (format #f "~a UPS ~a Power has returned."
+ (gethostname) name)))
+ (mail-to-root msg msg))
+ (wall "Power has returned on UPS ~a..." name)))
+ "Handler for offbattery event.")
+ (mainsback
+ (gexp
+ #~((when (file-exists? powerfail-file)
+ (wall "Continuing with shutdown."))))
+ "Handler for mainsback event.")
+ (failing
+ (gexp
+ #~((wall "Battery power exhausted on UPS ~a. Doing shutdown." name)))
+ "Handler for failing event.")
+ (timeout
+ (gexp
+ #~((wall "Battery time limit exceeded on UPS ~a. Doing shutdown." name)))
+ "Handler for timeout event.")
+ (loadlimit
+ (gexp
+ #~((wall "Remaining battery charge below limit on UPS ~a. Doing shutdown." name)))
+ "Handler for loadlimit event.")
+ (runlimit
+ (gexp
+ #~((wall "Remaining battery runtime below limit on UPS ~a. Doing shutdown." name)))
+ "Handler for runlimit event.")
+ (doreboot
+ (gexp
+ #~((wall "UPS ~a initiating Reboot Sequence" name)
+ (system* #$(file-append shepherd "/sbin/reboot"))))
+ "Handler for doreboot event.")
+ (doshutdown
+ (gexp
+ #~((wall "UPS ~a initiated Shutdown Sequence" name)
+ (system* #$(file-append shepherd "/sbin/halt"))))
+ "Handler for doshutdown event.")
+ (annoyme
+ (gexp
+ #~((wall "Power problems with UPS ~a. Please logoff." name)))
+ "Handler for annoyme event.")
+ (emergency
+ (gexp
+ #~((wall "Emergency Shutdown. Possible battery failure on UPS ~a." name)))
+ "Handler for emergency event.")
+ (changeme
+ (gexp
+ #~((let ((msg (format #f "~a UPS ~a battery needs changing NOW."
+ (gethostname) name)))
+ (mail-to-root msg msg))
+ (wall "Emergency! Batteries have failed on UPS ~a. Change them NOW." name)))
+ "Handler for changeme event.")
+ (remotedown
+ (gexp
+ #~((wall "Remote Shutdown. Beginning Shutdown Sequence.")))
+ "Handler for remotedown event.")
+ (startselftest
+ (gexp
+ #~(#t))
+ "Handler for startselftest event.")
+ (endselftest
+ (gexp
+ #~(#t))
+ "Handler for endselftest event.")
+ (battdetach
+ (gexp
+ #~(#t))
+ "Handler for battdetach event.")
+ (battattach
+ (gexp
+ #~(#t))
+ "Handler for battattach event."))
+
+(define-syntax define-enum
+ (lambda (x)
+ (syntax-case x ()
+ ((_ name values)
+ (let* ((d/n (syntax->datum #'name))
+ (d/predicate (string->symbol
+ (format #f "enum-~a?" d/n)))
+ (d/serialize (string->symbol
+ (format #f "serialize-enum-~a" d/n))))
+ (with-syntax
+ ((predicate (datum->syntax x d/predicate))
+ (serialize (datum->syntax x d/serialize)))
+ #'(begin
+ (define (predicate value)
+ (memq value values))
+ (define serialize serialize-symbol))))))))
+
+(define mangle-field-name
+ (match-lambda
+ ('name "UPSNAME")
+ ('cable "UPSCABLE")
+ ('type "UPSTYPE")
+ ('device "DEVICE")
+ ('poll-time "POLLTIME")
+ ('lock-dir "LOCKFILE")
+ ('power-fail-dir "PWRFAILDIR")
+ ('no-login-dir "NOLOGINDIR")
+ ('on-batery-delay "ONBATTERYDELAY")
+ ('battery-level "BATTERYLEVEL")
+ ('remaining-minutes "MINUTES")
+ ('timeout "TIMEOUT")
+ ('annoy-interval "ANNOY")
+ ('annoy-delay "ANNOYDELAY")
+ ('no-logon "NOLOGON")
+ ('kill-delay "KILLDELAY")
+ ('net-server "NETSERVER")
+ ('net-server-ip "NISIP")
+ ('net-server-port "NISPORT")
+ ('net-server-events-file "EVENTSFILE")
+ ('net-server-events-file-max-size "EVENTSFILEMAX")
+ ('class "UPSCLASS")
+ ('mode "UPSMODE")
+ ('stat-time "STATTIME")
+ ('stat-file "STATFILE")
+ ('log-stats "LOGSTATS")
+ ('data-time "DATATIME")
+ ('facility "FACILITY")))
+
+(define (serialize-string field-name value)
+ #~(format #f "~a ~a\n" #$(mangle-field-name field-name) '#$value))
+(define serialize-symbol serialize-string)
+(define serialize-integer serialize-string)
+(define (serialize-boolean field-name value)
+ #~(format #f "~a ~a\n" #$(mangle-field-name field-name) #$(if value "on" "off")))
+
+(define-maybe string)
+
+(define-enum cable '( simple smart ether usb
+ 940-0119A 940-0127A 940-0128A 940-0020B 940-0020C
+ 940-0023A 940-0024B 940-0024C 940-1524C 940-0024G
+ 940-0095A 940-0095B 940-0095C 940-0625A MAM-04-02-2000))
+(define-enum type '(apcsmart usb net snmp netsnmp dumb pcnet modbus test))
+(define-enum no-logon '(disable timeout percent minutes always))
+(define-enum class '(standalone shareslave sharemaster))
+(define-enum mode '(disable share))
+
+(define-configuration apcupsd-configuration
+ (package (package apcupsd) "Package to use.")
+
+ (shepherd-base-name
+ (symbol 'apcupsd)
+ "Base name of the shepherd service. You can add the service multiple times
+with different prefix to manage multiple UPSes."
+ empty-serializer)
+ (auto-start?
+ (boolean #t)
+ "Should the shepherd service auto-start?"
+ empty-serializer)
+ (pid-file
+ (string "/var/run/apcupsd.pid")
+ "Path to the pid file."
+ empty-serializer)
+ (debug-level
+ (integer 0)
+ "Logging verbosity. Bigger number means more logs. In the source code I saw
+up to @code{300}, so for all logs, @code{999} seems reasonable."
+ empty-serializer)
+
+ (run-dir
+ (string "/var/run/apcupsd")
+ "Directory containing runtime information. You need to change this if you
+desire to run multiple instances of the daemon."
+ empty-serializer)
+
+ ;; General configuration parameters
+ (name
+ maybe-string
+ "Use this to give your UPS a name in log files and such. This is
+particularly useful if you have multiple UPSes. This does not set the EEPROM.
+It should be 8 characters or less.")
+ (cable
+ (enum-cable 'usb)
+ "The type of cable connecting the UPS to your computer. Possible generic
+choices are @code{'simple}, @code{'smart}, @code{'ether} and
+@code{'usb}.
+
+Or a specific cable model number may be used: @code{'940-0119A},
+@code{'940-0127A}, @code{'940-0128A}, @code{'940-0020B}, @code{'940-0020C},
+@code{'940-0023A}, @code{'940-0024B}, @code{'940-0024C}, @code{'940-1524C},
+@code{'940-0024G}, @code{'940-0095A}, @code{'940-0095B}, @code{'940-0095C},
+@code{'940-0625A}, @code{'M-04-02-2000}.")
+ (type
+ (enum-type 'usb)
+ "Type of the UPS you have.
+
+@table @code
+@item apcsmart
+Newer serial character device, appropriate for SmartUPS models using a serial
+cable (not USB).
+
+@item usb
+Most new UPSes are USB.
+
+@item net
+Network link to a master apcupsd through apcupsd's Network Information Server.
+This is used if the UPS powering your computer is connected to a different
+computer for monitoring.
+
+@item snmp
+SNMP network link to an SNMP-enabled UPS device.
+
+@item netsnmp
+Same as SNMP above but requires use of the net-snmp library. Unless you have a
+specific need for this old driver, you should use @code{'snmp} instead.
+
+@item dumb
+Old serial character device for use with simple-signaling UPSes.
+
+@item pcnet
+PowerChute Network Shutdown protocol which can be used as an alternative to SNMP
+with the AP9617 family of smart slot cards.
+
+@item modbus
+Serial device for use with newest SmartUPS models supporting the MODBUS
+protocol.
+
+@end table")
+ (device
+ (string "")
+ "For USB UPSes, usually you want to set this to an empty string (the
+default). For other UPS types, you must specify an appropriate port or address.
+
+@table @code
+@item apcsmart
+Set to the appropriate @file{/dev/tty**} device.
+
+@item usb
+A null string setting enables autodetection, which is the best choice for most
+installations.
+
+@item net
+Set to @code{@var{hostname}:@var{port}}.
+
+@item snmp
+Set to @code{@var{hostname}:@var{port}:@var{vendor}:@var{community}}.
+@var{hostname} is the ip address or hostname of the UPS on the network.
+@var{vendor} can be can be \"APC\" or \"APC_NOTRAP\". \"APC_NOTRAP\" will
+disable SNMP trap catching; you usually want \"APC\". @var{port} is usually
+161. @var{community} is usually \"private\".
+
+@item netsnmp
+Same as @code{'snmp}.
+
+@item dumb
+Set to the appropriate @file{/dev/tty**} device.
+
+@item pcnet
+Set to @code{@var{ipaddr}:@var{username}:@var{passphrase}:@var{port}}.
+@var{ipaddr} is the IP address of the UPS management card. @var{username} and
+@var{passphrase} are the credentials for which the card has been configured.
+@var{port} is the port number on which to listen for messages from the UPS,
+normally 3052. If this parameter is empty or missing, the default of 3052 will
+be used.
+
+@item modbus
+Set to the appropriate @file{/dev/tty**} device. You can also leave it empty
+for MODBUS over USB or set to the serial number of the UPS.
+
+@end table")
+ (poll-time
+ (integer 60)
+ "Interval (in seconds) at which apcupsd polls the UPS for status. This
+setting applies both to directly-attached UPSes (apcsmart, usb, dumb) and
+networked UPSes (net, snmp). Lowering this setting will improve apcupsd's
+responsiveness to certain events at the cost of higher CPU utilization.")
+
+ ;; Configuration parameters used during power failures
+ (on-batery-delay
+ (integer 6)
+ "The the time in seconds from when a power failure is detected until we react
+to it with an onbattery event. The @code{'powerout} event will be triggered
+immediately when a power failure is detected. However, the @code{'onbattery}
+event will be trigger only after this delay.")
+ (battery-level
+ (integer 5)
+ "If during a power failure, the remaining battery percentage (as reported by
+the UPS) is below or equal to this value, apcupsd will initiate a system
+shutdown.
+
+@quotation Note
+@code{battery-level}, @code{remaining-minutes}, and @code{timeout} work
+in conjunction, so the first that occurs will cause the initation of a
+shutdown.
+@end quotation")
+ (remaining-minutes
+ (integer 3)
+ "If during a power failure, the remaining runtime in minutes (as calculated
+internally by the UPS) is below or equal to this value, apcupsd will initiate a
+system shutdown.
+
+@quotation Note
+@code{battery-level}, @code{remaining-minutes}, and @code{timeout} work
+in conjunction, so the first that occurs will cause the initation of a
+shutdown.
+@end quotation")
+ (timeout
+ (integer 0)
+ "If during a power failure, the UPS has run on batteries for this many
+seconds or longer, apcupsd will initiate a system shutdown. A value of 0
+disables this timer.
+
+@quotation Note
+@code{battery-level}, @code{remaining-minutes}, and @code{timeout} work
+in conjunction, so the first that occurs will cause the initation of a
+shutdown.
+@end quotation")
+ (annoy-interval
+ (integer 300)
+ "Time in seconds between annoying users (via the @code{'annoyme} event) to
+sign off prior to system shutdown. 0 disables.")
+ (annoy-delay
+ (integer 60)
+ "Initial delay in seconds after power failure before warning users to get off
+the system.")
+ (no-logon
+ (enum-no-logon 'disable)
+ "The condition which determines when users are prevented from logging in
+during a power failure.")
+ (kill-delay
+ (integer 0)
+ "If this is non-zero, apcupsd will continue running after a shutdown has been
+requested, and after the specified time in seconds attempt to kill the power.
+This is for use on systems where apcupsd cannot regain control after a
+shutdown.")
+
+ ;; Configuration statements for Network Information Server
+ (net-server
+ (boolean #f)
+ "If enabled, a network information server process will be started.")
+ (net-server-ip
+ (string "127.0.0.1")
+ "IP address on which NIS server will listen for incoming connections.")
+ (net-server-port
+ (integer 3551)
+ "IP port on which NIS server will listen for incoming connections.")
+ (net-server-events-file
+ maybe-string
+ "If you want the last few EVENTS to be available over the network by the
+network information server, you must set this to a file patch.")
+ (net-server-events-file-max-size
+ (integer 10)
+ "Maximum size of the events file in kilobytes.")
+ ;; Configuration statements used if sharing a UPS with more than one machine
+ (class (enum-class 'standalone)
+ "Normally standalone unless you share an UPS using an APC ShareUPS card.")
+ (mode (enum-mode 'disable)
+ "Normally disable unless you share an UPS using an APC ShareUPS card.")
+ ;; Configuration statements to control apcupsd system logging
+ (stat-time
+ (integer 0)
+ "Time interval in seconds between writing the status file, 0 disables.")
+ (log-stats
+ (boolean #f)
+ "Also write the stats as a logs. This generates a lot of output.")
+ (data-time
+ (integer 0)
+ "Time interval in seconds between writing the data records to the log file, 0
+disables.")
+ (facility
+ maybe-string
+ "The logging facility for the syslog.")
+
+ ;; Event handlers
+ (event-handlers
+ (apcupsd-event-handlers (apcupsd-event-handlers))
+ "Handlers for events produced by apcupsd."
+ empty-serializer))
+
+(define (s/apccontrol cfg)
+ (program-file
+ "apccontrol"
+ #~(begin
+ (use-modules (srfi srfi-9)
+ (ice-9 format)
+ (ice-9 match)
+ (ice-9 popen))
+ ;; Script dir depends on these, and the configuration depends on the script
+ ;; dir. To sever the cyclic dependency, pass the paths via environment
+ ;; variables.
+ (define conf (getenv "GUIX_APCUPSD_CONF"))
+ (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
+
+ (define (err . args)
+ (apply format (current-error-port) args))
+ (define (wall . args)
+ (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
+ (define (apcupsd . args)
+ (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
+ (define (mail-to-root subject body)
+ (let ((port (open-pipe* OPEN_WRITE
+ "/run/privileged/bin/sendmail"
+ "-F" "apcupsd"
+ "root")))
+ (format port "Subject: ~a~%~%~a~&" subject body)
+ (close-pipe port)))
+ (match (cdr (command-line))
+ (((? string? cmd) name connected powered)
+ (match cmd
+ ;; I am sure this could be done by macro, but meh. Last release of
+ ;; apcupsd was in 2016, so maintaining this will not be much work.
+ ("killpower"
+ #$@(apcupsd-event-handlers-killpower
+ (apcupsd-configuration-event-handlers cfg)))
+ ("commfailure"
+ #$@(apcupsd-event-handlers-commfailure
+ (apcupsd-configuration-event-handlers cfg)))
+ ("commok"
+ #$@(apcupsd-event-handlers-commok
+ (apcupsd-configuration-event-handlers cfg)))
+ ("powerout"
+ #$@(apcupsd-event-handlers-powerout
+ (apcupsd-configuration-event-handlers cfg)))
+ ("onbattery"
+ #$@(apcupsd-event-handlers-onbattery
+ (apcupsd-configuration-event-handlers cfg)))
+ ("offbattery"
+ #$@(apcupsd-event-handlers-offbattery
+ (apcupsd-configuration-event-handlers cfg)))
+ ("mainsback"
+ #$@(apcupsd-event-handlers-mainsback
+ (apcupsd-configuration-event-handlers cfg)))
+ ("failing"
+ #$@(apcupsd-event-handlers-failing
+ (apcupsd-configuration-event-handlers cfg)))
+ ("timeout"
+ #$@(apcupsd-event-handlers-timeout
+ (apcupsd-configuration-event-handlers cfg)))
+ ("loadlimit"
+ #$@(apcupsd-event-handlers-loadlimit
+ (apcupsd-configuration-event-handlers cfg)))
+ ("runlimit"
+ #$@(apcupsd-event-handlers-runlimit
+ (apcupsd-configuration-event-handlers cfg)))
+ ("doreboot"
+ #$@(apcupsd-event-handlers-doreboot
+ (apcupsd-configuration-event-handlers cfg)))
+ ("doshutdown"
+ #$@(apcupsd-event-handlers-doshutdown
+ (apcupsd-configuration-event-handlers cfg)))
+ ("annoyme"
+ #$@(apcupsd-event-handlers-annoyme
+ (apcupsd-configuration-event-handlers cfg)))
+ ("emergency"
+ #$@(apcupsd-event-handlers-emergency
+ (apcupsd-configuration-event-handlers cfg)))
+ ("changeme"
+ #$@(apcupsd-event-handlers-changeme
+ (apcupsd-configuration-event-handlers cfg)))
+ ("remotedown"
+ #$@(apcupsd-event-handlers-remotedown
+ (apcupsd-configuration-event-handlers cfg)))
+ ("startselftest"
+ #$@(apcupsd-event-handlers-startselftest
+ (apcupsd-configuration-event-handlers cfg)))
+ ("endselftest"
+ #$@(apcupsd-event-handlers-endselftest
+ (apcupsd-configuration-event-handlers cfg)))
+ ("battdetach"
+ #$@(apcupsd-event-handlers-battdetach
+ (apcupsd-configuration-event-handlers cfg)))
+ ("battattach"
+ #$@(apcupsd-event-handlers-battattach
+ (apcupsd-configuration-event-handlers cfg)))
+ (_
+ (err "Unknown event: ~a~%" cmd)
+ (err "Iff the event was passed by apcupsd, this is a bug.~%")
+ (err "Please report to bug-guix@gnu.org.~%")
+ (exit #f))))
+ (args
+ (err "Unknown arguments: ~a~%" args)
+ (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
+ (err "Please report to bug-guix@gnu.org.~%")
+ (exit #f))))))
+
+(define (apcupsd-script-dir cfg)
+ (computed-file
+ "apcupsd-script-dir"
+ #~(begin
+ (mkdir #$output)
+ (chdir #$output)
+ (symlink #$(s/apccontrol cfg) "apccontrol"))))
+
+(define (apcupsd-config-file cfg)
+ (let ((run-dir (apcupsd-configuration-run-dir cfg)))
+ (mixed-text-file
+ "apcupsd.conf"
+ "\
+## apcupsd.conf v1.1 ##
+#
+# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix
+#
+# \"apcupsd\" POSIX config file (crafted via apcupsd-service-type)
+"
+ (serialize-configuration cfg apcupsd-configuration-fields)
+ ;; This one is confusing. The manual page states:
+ ;;
+ ;; > It must be changed when running more than one copy of apcupsd on the
+ ;; > same computer to control multiple UPSes.
+ ;;
+ ;; However would you not want the lock to be per-device, not per-process?
+ ;; I decided to follow the documentation, but I do not understand why it
+ ;; should be like this. I do not have multiple UPSes to try.
+ (serialize-string 'lock-dir (string-append run-dir "/lock"))
+ (serialize-string 'power-fail-dir run-dir)
+ (serialize-string 'no-login-dir run-dir)
+ (serialize-string 'stat-file (string-append run-dir "/apcupsd.status"))
+ "SCRIPTDIR " (apcupsd-script-dir cfg) "\n")))
+
+(define (apcupsd-shepherd-services cfg)
+ (match-record cfg <apcupsd-configuration>
+ ( package pid-file debug-level run-dir
+ shepherd-base-name auto-start?)
+ (let* ((config-file (apcupsd-config-file cfg))
+ (s/ shepherd-base-name)
+ (s/run-dirs (string->symbol (format #f "~a-run-dirs" s/))))
+ (list
+ (shepherd-service
+ (documentation "Create the run directories.")
+ (provision (list s/run-dirs))
+ (one-shot? #t)
+ (auto-start? auto-start?)
+ (start #~(lambda _
+ ((@ (guix build utils) mkdir-p)
+ #$(string-append run-dir "/lock"))
+ #t)))
+ (shepherd-service
+ (documentation "Run apcupsd daemon.")
+ (requirement (list s/run-dirs))
+ (provision (list s/))
+ (auto-start? auto-start?)
+ (start #~(make-forkexec-constructor
+ '(#$(file-append package "/sbin/apcupsd")
+ "-b" ; Do not daemonize.
+ "-f" #$config-file
+ "-P" #$pid-file
+ "-d" #$(number->string debug-level))
+ #:log-file
+ #$(format #f "/var/log/~a.log" shepherd-base-name)
+ #:environment-variables
+ (cons* (string-append "GUIX_APCUPSD_CONF="
+ #$config-file)
+ #$(string-append "GUIX_APCUPSD_POWERFAIL_FILE="
+ run-dir "/powerfail")
+ (default-environment-variables))))
+ (stop #~(make-kill-destructor))
+ (actions (list (shepherd-configuration-action config-file))))))))
+
+(define (apcupsd-pam-extensions cfg)
+ (define pam-nologin
+ (pam-entry
+ (control "required")
+ (module "pam_nologin.so")
+ (arguments (list (string-append "file="
+ (apcupsd-configuration-run-dir cfg)
+ "/nologin")))))
+
+ (list (pam-extension
+ (transformer
+ (lambda (pam)
+ (pam-service
+ (inherit pam)
+ (auth (cons pam-nologin (pam-service-auth pam)))))))))
+
+(define apcupsd-service-type
+ (service-type
+ (name 'apcupsd)
+ (description "Configure and optionally start apcupsd.")
+ (extensions (list (service-extension shepherd-root-service-type
+ apcupsd-shepherd-services)
+ (service-extension pam-root-service-type
+ apcupsd-pam-extensions)))
+ (compose identity)
+ (extend (λ (cfg lst)
+ (fold (cut <> <>) cfg lst)))
+ (default-value (apcupsd-configuration))))