[PATCH] services: lsh: Add "graceful" handling of daemonic option.

[PATCH] services: lsh: Add "graceful" handling of daemonic option.

[Deck Pickard]

[-- Attachment #1.1: Type: text/plain, Size: 248 bytes --]

#~(#$@ looks freaky, but if this is what it takes... Tried couple of other
"figures", this one appears to generate right dmd.conf, though I haven't
had yet a chance to reboot.

Drp,
-- 
(or ((,\ (x) `(,x x)) '(,\ (x) `(,x x))) (smth (that 'like)))

[-- Attachment #1.2: Type: text/html, Size: 311 bytes --]

[-- Attachment #2: 0001-services-lsh-Add-graceful-handling-of-daemonic-optio.patch --]
[-- Type: application/octet-stream, Size: 6129 bytes --]

From 1fef935d6292016c04b9234eedb5dcaf006dc152 Mon Sep 17 00:00:00 2001
From: nebuli <nebu@kipple>
Date: Wed, 3 Dec 2014 22:51:48 +0100
Subject: [PATCH] services: lsh: Add graceful handling of daemonic option.

* doc/guix.texi: Mention use case.
* gnu/services/ssh.scm (lsh-service): New #:keys (daemonic?, pid-file?,
  pid-file).  Build new lshd-command and expand service-requirement
  field.
---
 doc/guix.texi        |  7 +++++-
 gnu/services/ssh.scm | 63 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index e804d79..63f070f 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -4224,7 +4224,7 @@ configuration file.
 Furthermore, @code{(gnu services ssh)} provides the following service.
 
 @deffn {Monadic Procedure} lsh-service [#:host-key "/etc/lsh/host-key"] @
-       [#:interfaces '()] [#:port-number 22] @
+       [#:daemonic? #f] [#:interfaces '()] [#:port-number 22] @
        [#:allow-empty-passwords? #f] [#:root-login? #f] @
        [#:syslog-output? #t] [#:x11-forwarding? #t] @
        [#:tcp/ip-forwarding? #t] [#:password-authentication? #t] @
@@ -4233,6 +4233,11 @@ Run the @command{lshd} program from @var{lsh} to listen on port @var{port-number
 @var{host-key} must designate a file containing the host key, and readable
 only by root.
 
+When @var{daemonic?} is true, @command{lshd} will detach from the
+controlling terminal and log its output to syslogd, unless one sets
+@var{syslog-output?} to false.  Obviously, it also makes lsh-service
+depend on existence of syslogd service.
+
 When @var{initialize?} is true, automatically create the seed and host key
 upon service activation if they do not exist yet.  This may take long and
 require interaction.
diff --git a/gnu/services/ssh.scm b/gnu/services/ssh.scm
index 2b52c77..6659301 100644
--- a/gnu/services/ssh.scm
+++ b/gnu/services/ssh.scm
@@ -72,12 +72,15 @@
 
 (define* (lsh-service #:key
                       (lsh lsh)
+                      (daemonic? #f)
                       (host-key "/etc/lsh/host-key")
                       (interfaces '())
                       (port-number 22)
                       (allow-empty-passwords? #f)
                       (root-login? #f)
                       (syslog-output? #t)
+                      (pid-file? #f)
+                      (pid-file "/var/run/lshd.pid")
                       (x11-forwarding? #t)
                       (tcp/ip-forwarding? #t)
                       (password-authentication? #t)
@@ -87,6 +90,11 @@
 @var{host-key} must designate a file containing the host key, and readable
 only by root.
 
+When @var{daemonic?} is true, @command{lshd} will detach from the
+controlling terminal and log its output to syslogd, unless one sets
+@var{syslog-output?} to false.  Obviously, it also makes lsh-service
+depend on existence of syslogd service.
+
 When @var{initialize?} is true, automatically create the seed and host key
 upon service activation if they do not exist yet.  This may take long and
 require interaction.
@@ -106,30 +114,47 @@ root.
 
 The other options should be self-descriptive."
   (define lsh-command
-    (cons* #~(string-append #$lsh "/sbin/lshd")
-           #~(string-append "--host-key=" #$host-key)
-           #~(string-append "--password-helper=" #$lsh "/sbin/lsh-pam-checkpw")
-           #~(string-append "--subsystems=sftp=" #$lsh "/sbin/sftp-server")
-           "-p" (number->string port-number)
-           (if password-authentication? "--password" "--no-password")
-           (if public-key-authentication?
-               "--publickey" "--no-publickey")
-           (if root-login?
-               "--root-login" "--no-root-login")
-           (if x11-forwarding?
-               "--x11-forward" "--no-x11-forward")
-           (if tcp/ip-forwarding?
-               "--tcpip-forward" "--no-tcpip-forward")
-           (if (null? interfaces)
-               '()
-               (list (string-append "--interfaces="
-                                    (string-join interfaces ","))))))
+    (append
+     (cons #~(string-append #$lsh "/sbin/lshd")
+           (if daemonic?
+               (let ((syslog (if syslog-output? '()
+                                 (list "--no-syslog"))))
+                 (cons "--daemonic"
+                       (if pid-file?
+                           (cons #~(string-append "--pid-file=" #$pid-file)
+                                 syslog)
+                           (cons "--no-pid-file"
+                                 syslog))))
+               ;; will it force pid-file creation... seems it should.
+               (if pid-file? (list #~(string-append "--pid-file=" #$pid-file))
+                   '())))
+     (cons* #~(string-append "--host-key=" #$host-key)
+            #~(string-append "--password-helper=" #$lsh "/sbin/lsh-pam-checkpw")
+            #~(string-append "--subsystems=sftp=" #$lsh "/sbin/sftp-server")
+            "-p" (number->string port-number)
+            (if password-authentication? "--password" "--no-password")
+            (if public-key-authentication?
+                "--publickey" "--no-publickey")
+            (if root-login?
+                "--root-login" "--no-root-login")
+            (if x11-forwarding?
+                "--x11-forward" "--no-x11-forward")
+            (if tcp/ip-forwarding?
+                "--tcpip-forward" "--no-tcpip-forward")
+            (if (null? interfaces)
+                '()
+                (list (string-append "--interfaces="
+                                     (string-join interfaces ",")))))))
+  (define requires
+    (if (and daemonic? syslog-output?)
+        '(networking syslogd)
+        '(networking)))
 
   (with-monad %store-monad
     (return (service
              (documentation "GNU lsh SSH server")
              (provision '(ssh-daemon))
-             (requirement '(networking))
+             (requirement #~(#$@requires))
              (start #~(make-forkexec-constructor (list #$@lsh-command)))
              (stop  #~(make-kill-destructor))
              (pam-services
-- 
2.1.2

Re: [PATCH] services: lsh: Add "graceful" handling of daemonic option.

[Ludovic Courtès]

Deck Pickard <deck.r.pickard@gmail.com> skribis:

> From 1fef935d6292016c04b9234eedb5dcaf006dc152 Mon Sep 17 00:00:00 2001
> From: nebuli <nebu@kipple>
> Date: Wed, 3 Dec 2014 22:51:48 +0100
> Subject: [PATCH] services: lsh: Add graceful handling of daemonic option.
>
> * doc/guix.texi: Mention use case.
> * gnu/services/ssh.scm (lsh-service): New #:keys (daemonic?, pid-file?,
>   pid-file).  Build new lshd-command and expand service-requirement
>   field.

Nice!

>  (define* (lsh-service #:key
>                        (lsh lsh)
> +                      (daemonic? #f)
>                        (host-key "/etc/lsh/host-key")
>                        (interfaces '())
>                        (port-number 22)
>                        (allow-empty-passwords? #f)
>                        (root-login? #f)
>                        (syslog-output? #t)
> +                      (pid-file? #f)
> +                      (pid-file "/var/run/lshd.pid")
>                        (x11-forwarding? #t)
>                        (tcp/ip-forwarding? #t)
>                        (password-authentication? #t)

I would be tempted to not expose #:daemonic?, #:pid-file? and
#:syslog-output?, and instead always use --daemonic --pid-file=...

In particular, when using --daemonic, having the PID file is required,
otherwise dmd won’t know what the PID of this process is, and thus will
be unable to control it.  For that reason, #:pid-file? must not be
exposed.

WDYT?

> +  (define requires
> +    (if (and daemonic? syslog-output?)
> +        '(networking syslogd)
> +        '(networking)))

If we agree on the above, that would become '(networking syslogd)
unconditionally.

>      (return (service
>               (documentation "GNU lsh SSH server")
>               (provision '(ssh-daemon))
> -             (requirement '(networking))
> +             (requirement #~(#$@requires))

This is strictly equivalent to:

  (requirement `(,@requires))

or simply:

  (requirement requires)

:-)

G-expressions are only needed when capturing references to /gnu/store
items, packages, etc.

Thanks,
Ludo’.

Re: [PATCH] services: lsh: Add "graceful" handling of daemonic option.

[Deck Pickard]

[-- Attachment #1: Type: text/plain, Size: 3682 bytes --]

On 6 Dec 2014 15:28, "Ludovic Courtès" <ludo@gnu.org> wrote:
>
> Deck Pickard <deck.r.pickard@gmail.com> skribis:
>
> > From 1fef935d6292016c04b9234eedb5dcaf006dc152 Mon Sep 17 00:00:00 2001
> > From: nebuli <nebu@kipple>
> > Date: Wed, 3 Dec 2014 22:51:48 +0100
> > Subject: [PATCH] services: lsh: Add graceful handling of daemonic
option.
> >
> > * doc/guix.texi: Mention use case.
> > * gnu/services/ssh.scm (lsh-service): New #:keys (daemonic?, pid-file?,
> >   pid-file).  Build new lshd-command and expand service-requirement
> >   field.
>
> Nice!
>
> >  (define* (lsh-service #:key
> >                        (lsh lsh)
> > +                      (daemonic? #f)
> >                        (host-key "/etc/lsh/host-key")
> >                        (interfaces '())
> >                        (port-number 22)
> >                        (allow-empty-passwords? #f)
> >                        (root-login? #f)
> >                        (syslog-output? #t)
> > +                      (pid-file? #f)
> > +                      (pid-file "/var/run/lshd.pid")
> >                        (x11-forwarding? #t)
> >                        (tcp/ip-forwarding? #t)
> >                        (password-authentication? #t)
>
> I would be tempted to not expose #:daemonic?, #:pid-file? and
> #:syslog-output?, and instead always use --daemonic --pid-file=...
>
> In particular, when using --daemonic, having the PID file is required,
> otherwise dmd won’t know what the PID of this process is, and thus will
> be unable to control it.  For that reason, #:pid-file? must not be
> exposed.
>
> WDYT?

I implemented this because, from what I gather, lshd will write to syslog
only in '--daemonic' mode, otherwise it spams the controlling terminal on
which dmd is running. And I wanted lsh to use syslog!

As it is now, dmd captures the right PID from the "make-fork" constructor
alone, while having no idea of pid files; I went as far as to write dmd
service (and 'deco sideloding' it), which printed out both PIDs, they were
eqv...

There might still remain a use case with daemonic? equal to false for
someone out there, even for simple reason of lack of functioning syslog (as
well as a use case of choosing not to log at all), shrug...

Change default to (daemonic? #t) and adjust the docs? Your call...
I did not mention pid file related keys in the docs, because it would be
only useful to someone who had to bother to look at actual lsh-service
signature, like someone who did need pid file for some strange purpose...

>
> > +  (define requires
> > +    (if (and daemonic? syslog-output?)
> > +        '(networking syslogd)
> > +        '(networking)))
>
> If we agree on the above, that would become '(networking syslogd)
> unconditionally.
>

No, as I explained; one thing is having a chosen set of defaults, another
removing flexibility... lsh and/or dmd behaviour could change or someone
could like to rewrite lsh service definition.

> >      (return (service
> >               (documentation "GNU lsh SSH server")
> >               (provision '(ssh-daemon))
> > -             (requirement '(networking))
> > +             (requirement #~(#$@requires))
>
> This is strictly equivalent to:
>
>   (requirement `(,@requires))
>
> or simply:
>
>   (requirement requires)
>
> :-)
>
> G-expressions are only needed when capturing references to /gnu/store
> items, packages, etc.
>
> Thanks,
> Ludo’.

Roger, still groking my way around, at least it doesn't matter apart from
couple useless macro expansions.
Drp,
-- 
(or ((,\ (x) `(,x x)) '(,\ (x) `(,x x))) (smth (that 'like)))

[-- Attachment #2: Type: text/html, Size: 4804 bytes --]

Re: [PATCH] services: lsh: Add "graceful" handling of daemonic option.

[Ludovic Courtès]

Hi!

Deck Pickard <deck.r.pickard@gmail.com> skribis:

> From 1fef935d6292016c04b9234eedb5dcaf006dc152 Mon Sep 17 00:00:00 2001
> From: nebuli <nebu@kipple>
> Date: Wed, 3 Dec 2014 22:51:48 +0100
> Subject: [PATCH] services: lsh: Add graceful handling of daemonic option.
>
> * doc/guix.texi: Mention use case.
> * gnu/services/ssh.scm (lsh-service): New #:keys (daemonic?, pid-file?,
>   pid-file).  Build new lshd-command and expand service-requirement
>   field.

This patch had fallen through the cracks, sorry about that.

I’ve applied it with minor changes: I changed #:daemonic? to default to
#t, I added #:pid-file? to the documentation, and simplified the syntax
for the ‘requirements’ field as discussed.

I ended up leaving all the options, as you intended, so that users can
choose whether or not to use daemonic mode.

Thank you!

Ludo’.