Subject: [bug#75528] [PATCH 2/2] services: Add power.

From: Tomas Volf <[email-masked]>
Date: Sun, 23 Feb 2025 22:21:35 +0100

Maxim Cournoyer <[email-masked]> writes:

> [..]

>>> 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
>>
>> 	      ("emergency"
>> 	       #$@(apcupsd-event-handlers-emergency
>> 		   (apcupsd-configuration-event-handlers cfg)))
>>
>>
>> into
>>
>> 	      ("emergency"
>> 	       #$@(match-record cfg <apcupsd-configuration> (handlers)
>>                     (match-record handlers <apcupsd-event-handlers> (emergency)
>>                       emergency)))
>>
>>
>> I am not sure it is increase in readability, but maybe I misunderstood
>> the suggestion.
>
> I think something like this should work:
>
> modified   gnu/services/power.scm
> @@ -494,119 +494,67 @@ (define-configuration apcupsd-configuration
>     empty-serializer))
>  
>  (define (%apccontrol config)
> -  (program-file
> -   "apccontrol"
> -   #~(begin
> -       (use-modules (ice-9 format)
> -                    (ice-9 match)
> -                    (ice-9 popen)
> -                    (srfi srfi-9)
> -                    #$@(apcupsd-event-handlers-modules
> -                        (apcupsd-configuration-event-handlers config)))
> -       ;; Script dir depends on these, and the configuration depends on the
> -       ;; script dir.  To sever the cyclic dependency, pass the file names 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)
> -          (let ((connected? (match connected
> +  (define event-handlers (apcupsd-configuration-event-handlers config))
> +  (match-record event-handlers <apcupsd-event-handlers>
> +                ( modules killpower commfailure commok powerout
> +                  onbattery offbattery ...)
> +    (program-file
> +     "apccontrol"
> +     #~(begin
> +         (use-modules (ice-9 format)
> +                      (ice-9 match)
> +                      (ice-9 popen)
> +                      (srfi srfi-9)
> +                      #$@modules)
> +         ;; Script dir depends on these, and the configuration depends on the
> +         ;; script dir.  To sever the cyclic dependency, pass the file names 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)
> +            (let ((connected? (match connected
> +                                ("1" #t)
> +                                ("0" #f)))
> +                  (powered? (match powered
>                                ("1" #t)
> -                              ("0" #f)))
> -                (powered? (match powered
> -                            ("1" #t)
> -                            ("0" #f))))
> -            (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 config)))
> -              ("commfailure"
> -               #$@(apcupsd-event-handlers-commfailure
> -                   (apcupsd-configuration-event-handlers config)))
> -              ("commok"
> -               #$@(apcupsd-event-handlers-commok
> -                   (apcupsd-configuration-event-handlers config)))
> -	      ("powerout"
> -	       #$@(apcupsd-event-handlers-powerout
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("onbattery"
> -	       #$@(apcupsd-event-handlers-onbattery
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("offbattery"
> -	       #$@(apcupsd-event-handlers-offbattery
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("mainsback"
> -	       #$@(apcupsd-event-handlers-mainsback
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("failing"
> -	       #$@(apcupsd-event-handlers-failing
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("timeout"
> -	       #$@(apcupsd-event-handlers-timeout
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("loadlimit"
> -	       #$@(apcupsd-event-handlers-loadlimit
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("runlimit"
> -	       #$@(apcupsd-event-handlers-runlimit
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("doreboot"
> -	       #$@(apcupsd-event-handlers-doreboot
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("doshutdown"
> -	       #$@(apcupsd-event-handlers-doshutdown
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("annoyme"
> -	       #$@(apcupsd-event-handlers-annoyme
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("emergency"
> -	       #$@(apcupsd-event-handlers-emergency
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("changeme"
> -	       #$@(apcupsd-event-handlers-changeme
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("remotedown"
> -	       #$@(apcupsd-event-handlers-remotedown
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("startselftest"
> -	       #$@(apcupsd-event-handlers-startselftest
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("endselftest"
> -	       #$@(apcupsd-event-handlers-endselftest
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("battdetach"
> -	       #$@(apcupsd-event-handlers-battdetach
> -		   (apcupsd-configuration-event-handlers config)))
> -	      ("battattach"
> -	       #$@(apcupsd-event-handlers-battattach
> -		   (apcupsd-configuration-event-handlers config)))
> -              (_
> -               (err "Unknown event: ~a~%" cmd)
> -               (err "Iff the event was emitted by apcupsd, this is a bug.~%")
> -               (err "Please report to [email-masked].~%")
> -               (exit #f)))))
> -         (args
> -          (err "Unknown arguments: ~a~%" args)
> -          (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
> -          (err "Please report to [email-masked].~%")
> -          (exit #f))))))
> +                              ("0" #f))))
> +              (match cmd
> +                ("killpower" #$killpower)
> +                ("commfailure" #$commfailure)
> +                ("commok" #$commok)
> +	        ("powerout" #$powerout)
> +	        ("onbattery" #$onbattery)
> +	        ("offbattery" #$offbattery)
> +                [...]
> +                (_
> +                 (err "Unknown event: ~a~%" cmd)
> +                 (err "If the event was emitted by apcupsd, this is a bug.~%")
> +                 (err "Please report to [email-masked].~%")
> +                 (exit #f)))))
> +           (args
> +            (err "Unknown arguments: ~a~%" args)
> +            (err "If the arguments were passed by apcupsd, this is a bug.~%")
> +            (err "Please report to [email-masked].~%")
> +            (exit #f))))))))
>  
>  (define (apcupsd-script-dir config)
>    (computed-file
>
> [...]

I get the idea now, thank you.  Yes, that is *much* nicer.  Updated.

> [..]
> I'll let you work through my last match-record suggestion above, if you
> want and send a v3.

Incorporated and v3 sent.  This time a single patch, since the package
was already merged.

>
> I'm also attaching a fixup with the terminology changes I've submitted
> upstream if you'd like to validate it still works (I don't have any
> UPS to try it with myself).

Thank you, but I will pass on that.  I respect your choice to invest
time into this effort, but for me this issue it not high enough on my
priority list to do the same.

Have a nice day,
Tomas

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.