Julien Lepiller <jul...@lepiller.eu> skribis: > * gnu/services/vpn.scm: New file. > * gnu/local.mk (GNU_SYSTEM_MODULES): Add it. > * doc/guix.texi (VPN Services): New section.
Woow, neat! Overall LGTM! The following comments are about cosmetic issues, but I think it’s best to get them right. > +@node VPN Services > +@subsubsection VPN Services > +@cindex VPN > + > +The @code{(gnu services vpn)} module provides the following sevices: Please provide a few sentences of context and index entries, like: @cindex VPN (virtual private network) @cindex virtual private network (VPN) The @code{(gnu services vpn)} module provides services related to @dfn{virtual private networks} (VPNs). It provides a @emph{client} service---allowing your machine to connect to a VPN, as well as @emph{server} functionality---where your machine hosts a VPN. Both use @uref{https://openvpn.net/, OpenVPN}. > +@deffn {Scheme Procedure} openvpn-client-service @ > + [#:config (openvpn-client-configuration)] > + > +Return a service that runs @var{openvpn}, a VPN daemon, as a client. @command{openvpn} (or @command{openvpnd}, whatever it’s called), or OpenVPN. @var is for variables. > +@deffn {Scheme Procedure} openvpn-server-service @ > + [#:config (openvpn-server-configuration)] > + > +Return a service that runs @var{openvpn}, a VPN daemon, as a server. Ditto. > + openvpn-ccd-configuration > + generate-openvpn-client-documentation > + generate-openvpn-server-documentation > +)) No hanging parens please. :-) > +(define (uglify-field-name name) > + (match name > + ('verbosity "verb") > + (_ (let ((str (symbol->string name))) > + (if (string-suffix? "?" str) > + (substring str 0 (1- (string-length str))) > + str))))) The indentation is off in several places. Could you run: ./etc/indent-code.el gnu/services/vpn.scm (You need to have ‘emacs’ or ‘emacs-minimal’ installed but this is non-interactive.) > +(define (create-ccd-directory val) > + (let ((files (map (lambda (ccd) > + (list (openvpn-ccd-configuration-name ccd) > + (with-output-to-string > + (lambda () > + (serialize-configuration > + ccd openvpn-ccd-configuration-fields))))) Please add a docstring. It’s not obvious what’s happening here. > + val))) > + (computed-file "ccd" > + (with-imported-modules '((guix build utils)) > + #~(begin > + (use-modules (guix build utils)) > + (mkdir-p #$output) > + (for-each > + (lambda (ccd) > + (call-with-output-file (string-append #$output "/" (car ccd)) > + (lambda (port) (display (car (cdr ccd)) port)))) > + '#$files)))))) Please use ‘match’ instead of (car (cdr ccd)): (lambda (port) (match ccd ((_ (thing _ ...)) (display thing port)))) Of course you can use an identifier more descriptive than ‘thing’. ;-) > + (server-ipv6 > + (cidr6 #f) > + "A CIDR notation specifying the ipv6 subnet inside the virtual > network.") It would be ideal if you could expand “CIDR” the first type. Also, in comments and docstrings in the whole file (and thus in guix.texi): s/ipv6/IPv6/ s/udp/UDP/ s/tcp/TCP/ s/diffie-hellman/Diffie-Hellman/ s/Openvpn/OpenVPN/ > +(define (openvpn-shepherd-service role) > + (lambda (config) > + (let* ((config-file (openvpn-config-file role config)) > + ; #$(serialize-configuration config > + ; (match role > + ; ('server > openvpn-server-configuration-fields) > + ; ('client > openvpn-client-configuration-fields))))) Please remove the comment. > + (start #~(make-forkexec-constructor > + (list (string-append #$openvpn "/sbin/openvpn") > + "--writepid" #$pid-file "--config" > #$config-file > + "--daemon"))) Add #:pid-file #$pid-file, so that shepherd does the right thing. > +(define %openvpn-activation > + #~(begin > + (mkdir-p "/var/run/openvpn"))) ‘begin’ can be omitted. Could you send an updated patch? Besides, could you think of a system test that would allow us to test both services? Perhaps a single config that has both the OpenVPN server and client running? Thoughts? Thank you! Ludo’.