Re: [PATCH 2/2] gnu: Add GSSD and Pipefs services

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

John Darrington <jmd@gnu.org> skribis:

> * gnu/services/nfs.scm (pipefs-service-type): New Variable,
> (gss-service-type): New Variable.

Nice!  Minor comments below:

> -@node Miscellaneous Services
> -@subsubsection Miscellaneous Services
> +@node NFS Services
> +@subsubsection NFS Services

I would call it “Network File System Services” or just “Network File
System”.

> +@cindex nfs

NFS.

> +The @code{(gnu services nfs)} module provides the following services,
> +which are most commonly used in relation to mouting or exporting NFS
> +filesystems.

“… to mounting or exporting files using the @dfn{Network File System}
(NFS).”

(Always write “file system” as two words.)

> +@subsubheading GSS Daemon Service
> +@cindex gssd
> +@cindex gss
> +
> +@defvr {Scheme Variable} gss-service-type
> +A service type  for the RPC Global Security System (GSS) daemon.
                 ^^
Extra space.

Is “RPC” needed here, or is it more generic?

Would be nice to add a sentence like “The GSS daemon provides mechanism
XYZ, which can be used to implement FOOBAR.”, where FOOBAR has some
connection with NFS.  I don’t know myself what to put in here but
hopefully there’s a README or something that can shed some light.  :-)

> +@deftp {Data Type} gss-configuration
> +Data type representing the configuration of the RPC GSS Daemon service.
> +This type has the following parameters:
> +@table @asis
> +@item @code{nfs-utils} (default: @code{nfs-utils})
                                    ^^^^^
Should be @var, because here we’re talking about the value of the
‘nfs-utils’ global variable.

> +@defvr {Scheme Variable} pipefs-service-type
> +A service type for the pipefs pseudo filesystem.
                                           ^^
s/pipefs pseudo filesystem/@code{rpc_pipefs} pseudo file system/
+ “… implemented by the kernel Linux.”
+ “The @command{rpc_pipefs} allows for communication between the kernel
and the user-land NFS daemons.”

> +@deftp {Data Type} pipefs-configuration
> +Data type representing the configuration of the pipefs service.
> +There are no configurable parameters to this type.
> +@end deftp

Not necessary (see below).

> +@node Miscellaneous Services
> +@subsubsection Miscellaneous Services

Oops!

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

Since there’s really no configuration here, it’s not needed.

> +(define pipefs-service-type
> +  (shepherd-service-type
> +   'pipefs
> +   (lambda (config)
> +     (with-imported-modules '((gnu build file-systems)
> +                              (guix build bournish))
> +       (define pipefs-dir "/var/lib/nfs/rpc_pipefs")
> +
> +       (shepherd-service
> +        (documentation "Mount the pipefs pseudo filesystem.")
> +        (provision '(rpc-pipefs))
> +
> +        (start #~(lambda ()
> +                   (mkdir-p #$pipefs-dir)
> +                   (mount "rpc_pipefs" #$pipefs-dir "rpc_pipefs")))
> +        (stop #~(lambda (pid . args)
> +                  (umount #$pipefs-dir MNT_DETACH))))))))

You could achieve something similar by extending ‘file-system-service’,
like ‘elogind-service-type’ does.  Could you try if it works here?

In that case, the name of the Shepherd service would become
“file-system-/var/lib/nfs/rpc_pipefs”.

> +     (define pkg
> +       (gss-configuration-gss config))

s/pkg/nfs-utils/

Could you send an updated patch?

Thanks for working on it!

Ludo’.