From: ludo@gnu.org (Ludovic Courtès)
To: John Darrington <jmd@gnu.org>
Cc: guix-devel@gnu.org
Subject: Re: [PATCH] gnu: Add Kerberos client service.
Date: Wed, 30 Nov 2016 14:09:17 +0100 [thread overview]
Message-ID: <874m2pktc2.fsf@gnu.org> (raw)
In-Reply-To: <1480444784-32432-2-git-send-email-jmd@gnu.org> (John Darrington's message of "Tue, 29 Nov 2016 19:39:44 +0100")
John Darrington <jmd@gnu.org> skribis:
> * doc/guix.texi (Kerberos Services)[Krb5 Service]: New subsubheading.
> * gnu/services/kerberos.scm (krb5-service-type): New variable.
Please mention the configuration.scm changes.
> +@subsubheading Krb5 Service
> +
> +The krb5 service provides the configuration for Kerberos clients, using
> +the MIT implementation of the Kerberos protocol version@tie{}5.
Please take into account my previous suggestions:
https://lists.gnu.org/archive/html/guix-devel/2016-11/msg00922.html
> +@defvr {Scheme Variable} krb5-service-type
> +A service type for Kerberos 5 clients.
Ditto.
> +(service krb5-service-type (krb5-configuration
> + (default-realm "EXAMPLE.COM")
Ditto.
> +The @code{krb5-realm} and @code{krb5-configuration} types have many fields.
> +Only the most commonly used ones are described here.
> +For a full list, and more detailed explanation of each, see the MIT
> +@uref{http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html,,krb5.conf}
> +documentation.
Shouldn’t it be a single comma in @uref? Also, @file{krb5.conf}.
> +@end deftp
> +
> +
Extra newline.
> (define (validate-configuration config fields)
> (for-each (lambda (field)
> (let ((val ((configuration-field-getter field) config)))
> - (unless ((configuration-field-predicate field) val)
> + (unless (or (not val) ((configuration-field-predicate field) val))
> (configuration-field-error
> (configuration-field-name field) val))))
Here you’re assuming that when VAL is #f, it’s necessary invalid, an
assumption that’s questionable and wasn’t made until now.
Can you instead change your own field predicate to do that?
> +(define (uglify-field-name field-name)
> + (let ((str (symbol->string field-name)))
> + (string-join (string-split (if (string-suffix? "?" str)
> + (substring str 0 (1- (string-length str)))
> + str)
> + #\-)
> + "_")))
Please add a docstring to explain what it does and/or an example.
> +(define (serialize-field* field-name val)
> + (format #t "~a = ~a\n" (uglify-field-name field-name) val))
> +
> +(define (serialize-string field-name val)
> + (if val
> + (serialize-field* field-name val) ""))
Align both arms of the ‘if’.
> +;; An end-point is an address such as "192.168.0.1"
> +;; or an address port pair ("foo.example.com" . 109)
> +(define (end-point? val)
> + (or (string? val)
> + (and (pair? val)
> + (string? (car val))
> + (integer? (cdr val)))))
Rather:
(define (end-point? val)
(match val
((? string?) #t)
(((? string?) . (? integer?)) #t)
(_ #f))) ;do we need this catch-all case?
> +(define (serialize-end-point field-name val)
> + (serialize-field* field-name
> + (if (string? val)
> + ;; The [] are needed in the case of IPv6 addresses
> + (format #f "[~a]" val)
> + (format #f "[~a]:~a" (car val) (cdr val)))))
No car/cdr please.
> +(define (serialize-space-separated-string-list field-name val)
> + (if val
> + (serialize-field* field-name (string-join val " "))))
> +
> +(define (comma-separated-string-list? val)
> + (and (list? val)
> + (and-map (lambda (x)
> + (and (string? x) (not (string-index x #\,))))
> + val)))
> +
> +(define (serialize-comma-separated-string-list field-name val)
> + (serialize-field* field-name (string-join val ",")))
> +
> +(define (comma-separated-integer-list? val)
> + (and (list? val)
> + (and-map (lambda (x) (integer? x))
> + val)))
> +
> +(define (serialize-comma-separated-integer-list field-name val)
> + (if val
> + (serialize-field* field-name
> + (string-drop ; Drop the leading comma
> + (fold
> + (lambda (i prev)
> + (string-append prev "," (number->string i)))
> + "" val) 1))))
> +
> +(define (file-name? val)
> + (and (string? val)
> + (string-prefix? "/" val)))
> +
> +(define (serialize-file-name field-name val)
> + (serialize-string field-name val))
> +
> +
> +(define (serialize-boolean field-name val)
> + (serialize-string field-name (if val "true" "false")))
> +
> +(define (non-negative-integer? val)
> + (and (exact-integer? val) (not (negative? val))))
> +
> +(define (serialize-non-negative-integer field-name val)
> + (if val
> + (serialize-field* field-name val)))
> +
> +(define (serialize-integer field-name val)
> + (if val
> + (serialize-field* field-name val))
No ‘else’ here? Looks like a bug.
> +(define (free-form-fields? val)
> + (match val
> + (() #t)
> + ((((? symbol?) . (? string)) . val) (free-form-fields? val))
> + (_ #f)))
> +
> +(define (serialize-free-form-fields field-name val)
> + (for-each (match-lambda ((k . v) (serialize-field* k v))) val))
How much of this is copied from configuration.scm? I don’t want
duplicated code here.
> +(define (realm-list? val)
> + (and (list? val)
> + (and-map (lambda (x) (krb5-realm? x)) val)))
Rather:
(match val
(((? krb5-realm?) ...) #t)
(_ #f))
Could you send an updated patch?
Thank you!
Ludo’.
next prev parent reply other threads:[~2016-11-30 13:09 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-09 19:58 [PATCH] gnu: Add kerberos service John Darrington
2016-11-18 15:23 ` John Darrington
2016-11-18 22:51 ` Ludovic Courtès
2016-11-19 6:57 ` John Darrington
2016-11-21 8:59 ` Ludovic Courtès
2016-11-22 17:52 ` [PATCH] gnu: Add Kerberos client service John Darrington
2016-11-23 22:01 ` Ludovic Courtès
2016-11-29 18:39 ` John Darrington
2016-11-29 18:39 ` John Darrington
2016-11-30 13:09 ` Ludovic Courtès [this message]
2016-11-30 13:44 ` John Darrington
2016-11-30 13:52 ` Andy Wingo
2016-12-03 12:27 ` John Darrington
2016-12-03 15:13 ` Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874m2pktc2.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=guix-devel@gnu.org \
--cc=jmd@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).