Re: [PATCH] gnu: Add Kerberos client service.

[Ludovic Courtès]

John Darrington <address@hidden> 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.

> address@hidden Krb5 Service
> +
> +The krb5 service provides the configuration for Kerberos clients, using
> +the MIT implementation of the Kerberos protocol address@hidden

Please take into account my previous suggestions:

  https://lists.gnu.org/archive/html/guix-devel/2016-11/msg00922.html

> address@hidden {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
> address@hidden://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}.

> address@hidden 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’.