Re: [PATCH] gnu: Add NFS related services.

[ludo@gnu.org (Ludovic Courtès)]

John Darrington <jmd@gnu.org> skribis:

> Another draft for review ...

Could you please include an iteration number in the subject line, and a
terse summary of the changes compared to the previous iteration?

That would be greatly helpful—I’m getting lost in a maze of unrelated
patch series and sometimes have a hard time remembering where we are and
what it is that I’m doing here.  ;-)

>
>
>
>
> * gnu/services/nfs.scm (pipefs-service-type): New Variable,
> (gss-service-type): New Variable, (idmap-service-type) New Variable.
> ---
>  doc/guix.texi        |  98 ++++++++++++++++++++++++++++++++++--
>  gnu/services/nfs.scm | 138 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 230 insertions(+), 6 deletions(-)

Please also mention the idmap things, the doc/guix.texi changes, etc.

> +@subsubheading GSS Daemon Service
> +@cindex GSSD
> +@cindex GSS
> +
> +The GSS daemon provides strong security for RPC based protocols.

“The @dfn{global security system} (GSS) daemon provides …”

>  
>  (define-record-type* <rpcbind-configuration>
>    rpcbind-configuration make-rpcbind-configuration
> @@ -38,11 +58,11 @@
>    (shepherd-service-type
>     'rpcbind
>     (lambda (config)
> -     (define pkg
> +     (define nfs-utils
>         (rpcbind-configuration-rpcbind config))
>  
>       (define rpcbind-command
> -       #~(list (string-append #$pkg "/bin/rpcbind") "-f"
> +       #~(list (string-append #$nfs-utils "/bin/rpcbind") "-f"

Should have been part of a previous patch I guess, but that’s fine.

> +(define-record-type* <pipefs-configuration>
> +  pipefs-configuration make-pipefs-configuration
> +  pipefs-configuration?
> +  (mount-point           pipefs-configuration-mount-point
> +                         (default default-pipefs-dir)))

Seems to me we don’t even need <pipefs-configuration>; a string would be
enough, no?

> +(define-record-type* <gss-configuration>
> +  gss-configuration make-gss-configuration
> +  gss-configuration?
> +  (pipefs-dir            gss-configuration-pipefs-dir
> +                         (default default-pipefs-dir))

s/dir/directory/

> +(define-record-type* <idmap-configuration>
> +  idmap-configuration make-idmap-configuration
> +  idmap-configuration?
> +  (pipefs-dir            idmap-configuration-pipefs-dir
> +                         (default default-pipefs-dir))
> +  (domain                idmap-configuration-domain
> +                           (default #f))
> +  (nfs-utils             idmap-configuration-idmap
> +                         (default nfs-utils)))
> +
> +(define idmap-service-type
> +  (shepherd-service-type
> +   'idmap
> +   (lambda (config)
> +
> +     (define nfs-utils
> +       (idmap-configuration-idmap config))
> +
> +     (define pipefs-dir
> +       (idmap-configuration-pipefs-dir config))
> +
> +     (define conf-file "/etc/guix-idmapd.conf")
> +
> +     (define idmap-command
> +       #~(list (string-append #$nfs-utils "/sbin/rpc.idmapd") "-f"
> +               "-p" #$pipefs-dir
> +               "-c" #$conf-file))
> +
> +     (define domain (idmap-configuration-domain config))
> +
> +     (shepherd-service
> +      (documentation "Start the RPC IDMAP daemon.")
> +      (requirement '(rpcbind-daemon rpc-pipefs))
> +      (provision '(idmap-daemon))
> +
> +      (start #~(lambda ()
> +                 (let ((pid (primitive-fork)))
> +                   (if (zero? pid)
> +                       (begin
> +                         (call-with-output-file #$conf-file
> +                           (lambda (port)
> +                             (format port "\n[General]\n")
> +                             (if #$domain
> +                                 (format port "Domain = ~a\n" #$domain))
> +                             (format port "\n[Mapping]\n")
> +                             (format port "Nobody-User = nobody\n")
> +                             (format port "Nobody-Group = nogroup\n")))
> +                         (exec-command #$idmap-command))
> +                       pid))))

I think the configuration file should be created elsewhere, in the
store:

  (define (idmap-config-file config)
    (plain-file "idmap.conf"
                (string-append "[General]" …)))

and then:

  (define idmap-command
    #~(list … "-c" #$(idmap-config-file config)))

  (shepherd-service
    ;; …
    (start #~(make-forkexec-constructor #$idmap-command)))

In general we should avoid populating /etc.

Could you send an updated patch?

Overall this seems to be almost ready, no?  Since this is a pretty
involved service composition, I think it would be fruitful in the future
to add a full test case in (gnu tests nfs) where we would export an NFS
tree and mount it.

Thank you!

Ludo’.