Re: [V2 PATCH 1/1] services: Add agetty service.

[Ricardo Wurmus <rekado@elephly.net>]

Leo Famulari <leo@famulari.name> writes:

> On Tue, Feb 14, 2017 at 07:24:17PM -0500, Leo Famulari wrote:
>> This 'extra' is a time-saving kludge. I'll add fields for all of
>> agetty's configuration options once I'm satisfied that the service works
>> on GuixSD.
>
> Here's a patch that exposes (almost all) of agetty's command-line
> options.
>
> Is this the right way? Or would we rather wrap only the most
> commonly-used options, and leave an "escape hatch" as in the first
> version of the patch? If so, which options should we expose in Scheme?

That’s hard to say.  I like to *always* keep an escape hatch, which is
useful in case we are too slow to adapt our code to changes introduced
by new versions.  In my opinion this version of the patch is better
because it exposes more values; it would be even better if it added a
general purpose escape hatch.

> I'll wait for feedback before writing the documentation.

Okay.  (Can we generate documentation for the individual fields somehow?)

Some naive comments follow.

> From 215ad705a933fda1170a5883277cd9a68db693e0 Mon Sep 17 00:00:00 2001
> From: Leo Famulari <leo@famulari.name>
> Date: Tue, 14 Feb 2017 11:28:04 -0500
> Subject: [PATCH] services: Add agetty service.
>
> * gnu/services/base.scm (<agetty-configuration>): New record type.
> (agetty-shepherd-service, agetty-service): New procedures.
> (agetty-service-type): New variable.
> ---
[…]

> +(define-record-type* <agetty-configuration>
> +  agetty-configuration make-agetty-configuration
> +  agetty-configuration?
> +  (agetty         agetty-configuration-agetty   ;<package>
> +                  (default util-linux))
> +  (tty            agetty-configuration-tty)     ;string
> +  (term           agetty-term                   ;string
> +                  (default #f))
> +  (baud-rate      agetty-baud-rate              ;string
> +                  (default #f))
> +  (auto-login     agetty-auto-login             ;string
> +                  (default #f))
> +  (login-program  agetty-login-program          ;gexp
> +                  (default (file-append shadow "/bin/login")))
> +  (login-pause?   agetty-login-pause?           ;Boolean
> +                  (default #f))
> +  (eight-bits?    agetty-eight-bits?            ;Boolean
> +                  (default #f))
> +  (no-reset?      agetty-no-reset?              ;Boolean
> +                  (default #f))
> +  (remote?        agetty-remote?                ;Boolean
> +                  (default #f))
> +  (flow-control?  agetty-flow-control?          ;Boolean
> +                  (default #f))
> +  (host           agetty-host                   ;string
> +                  (default #f))
> +  (no-issue?      agetty-no-issue?              ;Boolean
> +                  (default #f))
> +  (init-string    agetty-init-string            ;string
> +                  (default #f))
> +  (no-clear?      agetty-no-clear?              ;Boolean
> +                  (default #f))
> +  (local-line     agetty-local-line             ;always | never | auto
> +                  (default #f))
> +  (extract-baud?  agetty-extract-baud?          ;Boolean
> +                  (default #f))
> +  (skip-login?    agetty-skip-login?            ;Boolean
> +                  (default #f))
> +  (no-newline?    agetty-no-newline?            ;Boolean
> +                  (default #f))
> +  (login-options  agetty-login-options          ;string
> +                  (default #f))
> +  (chroot         agetty-chroot                 ;string
> +                  (default #f))
> +  (hangup?        agetty-hangup?                ;Boolean
> +                  (default #f))
> +  (timeout        agetty-timeout                ;integer
> +                  (default #f))
> +  (detect-case?   agetty-detect-case?           ;Boolean
> +                  (default #f))
> +  (wait-cr?       agetty-wait-cr?               ;Boolean
> +                  (default #f))
> +  (no-hints?      agetty-no-hints?              ;Boolean
> +                  (default #f))
> +  (no-hostname?   agetty-no hostname?           ;Boolean
> +                  (default #f))
> +  (long-hostname? agetty-long-hostname?         ;Boolean
> +                  (default #f))
> +  (erase-chars    agetty-erase-chars            ;string
> +                  (default #f))
> +  (kill-chars     agetty-kill-chars             ;string
> +                  (default #f))
> +  (chdir          agetty-chdir                  ;string
> +                  (default #f))
> +  (delay          agetty-delay                  ;integer
> +                  (default #f))
> +  (nice           agetty-nice                   ;integer
> +                  (default #f))
> +;;; XXX Unimplemented for now!
> +;;; (issue-file   agetty-issue-file             ;plain-file
> +;;;               (default #f))
> +  )
> +
> +(define agetty-shepherd-service
> +  (match-lambda
> +    (($ <agetty-configuration> agetty tty term baud-rate auto-login
> +        login-program login-pause? eight-bits? no-reset? remote? flow-control?
> +        host no-issue? init-string no-clear? local-line extract-baud?
> +        skip-login? no-newline? login-options chroot hangup? timeout
> +        detect-case? wait-cr? no-hints? no-hostname? long-hostname? erase-chars
> +        kill-chars chdir delay nice)
> +     (list
> +       (shepherd-service
> +         (documentation "Run agetty on a tty.")
> +         (provision (list (symbol-append 'term- (string->symbol tty))))
> +
> +         ;; Same comment as for mingetty-shepherd-service.
> +         (requirement '(user-processes host-name udev))
> +
> +         (start #~(make-forkexec-constructor
> +                    (list #$ (file-append util-linux "/sbin/agetty")
> +                          #$@(if eight-bits?
> +                                 #~("--8bits")
> +                                 #~())
> +                          #$@(if no-reset?
> +                                 #~("--noreset")
> +                                 #~())
> +                          #$@(if remote?
> +                                 #~("--remote")
> +                                 #~())

Looking at all the syntactic noise makes me long for some prettier
abstraction here, but I can’t think of anything that would make this
considerably prettier.  Oh well… ¯\_(ツ)_/¯

> +                          ;; This doesn't work as expected. According to
> +                          ;; agetty(8), if this option is not passed, then the
> +                          ;; default is 'auto'. However, in my tests, when that
> +                          ;; option is selected, agetty never presents the login
> +                          ;; prompt, and the term-ttyS0 service respawns every
> +                          ;; few seconds.

Maybe add a FIXME here, so we can more easily find it.  It might also be
worth opening a bug report after merging this, so that we can track it.

> +                          #$@(if no-newline?
> +                                 #~("--nonewline")
> +                                 #~())

I’m sure this will trip me up in the future.  “nonewline” vs
“no-newline” – I prefer your choice, but the double negative is tough
for me…

> +                          #$@(if no-hostname?
> +                                 #~("--nohostname")
> +                                 #~())
> +                          #$@(if long-hostname?
> +                                 #~("--long-hostname")
> +                                 #~())

Should we try to prevent nonsensical combinations of options?
Would it just be more confusing to merge “no-hostname?” and “long-hostname?” as
“print-hostname 'none 'long 'short”?

Everything else looks good to me!

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net