From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] gnu: Add Kerberos client service. Date: Wed, 30 Nov 2016 14:09:17 +0100 Message-ID: <874m2pktc2.fsf@gnu.org> References: <87shqh500f.fsf@gnu.org> <1480444784-32432-1-git-send-email-jmd@gnu.org> <1480444784-32432-2-git-send-email-jmd@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:49612) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cC4dX-0004RW-Ck for guix-devel@gnu.org; Wed, 30 Nov 2016 08:09:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cC4dU-0005sg-3w for guix-devel@gnu.org; Wed, 30 Nov 2016 08:09:23 -0500 Received: from fencepost.gnu.org ([2001:4830:134:3::e]:53929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cC4dU-0005sa-0G for guix-devel@gnu.org; Wed, 30 Nov 2016 08:09:20 -0500 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") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: John Darrington Cc: guix-devel@gnu.org John Darrington 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 fiel= ds. > +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_c= onf.html,,krb5.conf} > +documentation. Shouldn=E2=80=99t 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 fi= eld) val)) > (configuration-field-error > (configuration-field-name field) val)))) Here you=E2=80=99re assuming that when VAL is #f, it=E2=80=99s necessary in= valid, an assumption that=E2=80=99s questionable and wasn=E2=80=99t 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 s= tr))) > + str) > + #\-) > + "_"))) Please add a docstring to explain what it does and/or an example. > +(define (serialize-field* field-name val) > + (format #t "~a =3D ~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 =E2=80=98if=E2=80=99. > +;; 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 =E2=80=98else=E2=80=99 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=E2=80=99t 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=E2=80=99.