John Darrington <j...@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’.
