[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] gnu: Add openvpn service.
From: |
Ludovic Courtès |
Subject: |
Re: [PATCH 2/2] gnu: Add openvpn service. |
Date: |
Fri, 13 Jan 2017 18:58:18 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Julien Lepiller <address@hidden> 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.
> address@hidden VPN Services
> address@hidden VPN Services
> address@hidden 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}.
> address@hidden {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.
> address@hidden {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’.
- [PATCH 1/2] gnu: openvpn: Update to 2.4.0., Julien Lepiller, 2017/01/12
- [PATCH 2/2] gnu: Add openvpn service., Julien Lepiller, 2017/01/12
- Re: [PATCH 2/2] gnu: Add openvpn service.,
Ludovic Courtès <=
- Re: [PATCH 2/2] gnu: Add openvpn service., Julien Lepiller, 2017/01/14
- Re: [PATCH 2/2] gnu: Add openvpn service., Ludovic Courtès, 2017/01/14
- Re: [PATCH 2/2] gnu: Add openvpn service., Alex Kost, 2017/01/15
- Re: [PATCH 2/2] gnu: Add openvpn service., Ludovic Courtès, 2017/01/15
- Re: [PATCH 2/2] gnu: Add openvpn service., Alex Kost, 2017/01/16
- Re: [PATCH 2/2] gnu: Add openvpn service., Ludovic Courtès, 2017/01/19
- Re: [PATCH 2/2] gnu: Add openvpn service., Alex Kost, 2017/01/19
- Re: [PATCH 2/2] gnu: Add openvpn service., Ludovic Courtès, 2017/01/20
Re: [PATCH 2/2] gnu: Add openvpn service., Hartmut Goebel, 2017/01/14
Re: [PATCH 1/2] gnu: openvpn: Update to 2.4.0., Ludovic Courtès, 2017/01/13