Hi Oleg, Oleg Pykhalov writes: > I work on isc-bind service. Currently generation of named.conf is done. > Ideas and suggestions are welcome! :-) Awesome! Thank you for working on this. I'm not familiar with BIND configuration, so I can't really comment much on the particular fields you've chosen to include in the various configuration objects you've created. It'd be nice if someone more familiar with BIND could give it a look. > (define-record-type* Are these options intended to be used when invoking bind? If so, maybe a name like "bind-options" is probably good enough. > bind-options-configuration make-bind-options-configuration > bind-options-configuration? > (user bind-options-configuration-user ; string > (default "bind")) > (group bind-options-configuration-group ; string > (default "bind")) > (run-directory bind-options-configuration-run-directory ; string > (default "/var/run/bind")) > (pid-file bind-options-configuration-pid-file ; string > (default "/var/run/bind/named.pid")) For what it's worth, nowadays some distros use /run as the "run directory" [1]. I don't know if GuixSD has adopted any particular policy about whether to use /var/run or /run for the default "run directory". I don't currently know of any reason why it matters much, so I think it's fine to use /var/run here. > (listen-v4 bind-options-configuration-listen-v4 ; string > (default "0.0.0.0")) > (listen-v6 bind-options-configuration-listen-v6 ; string > (default "::")) > (listen-port bind-options-configuration-listen-port ; integer > (default 53)) > (allow-recursion? bind-configuration-allow-recursion? ; list > (default (list "127.0.0.1"))) > (allow-transfer? bind-configuration-allow-transfer? ; list > (default (list "none"))) > (allow-update? bind-configuration-allow-update? ; list > (default (list "none"))) > (version bind-configuration-version ; string > (default "none")) > (hostname bind-configuration-hostname ; string > (default "none")) Why not use the system's host name by default? For example: (hostname bind-configuration-hostname ; string (default (gethostname))) > > (server-id bind-configuration-server-id ; string (default "none"))) > > (define (bind-configuration-statement-string statements) > (string-join (list "{" (string-join statements ";\n") "}"))) > You could also write it like this: (define (bind-configuration-statement-string statements) (string-append "{" (string-join statements ";\n") "}")) > > (define-record-type* > bind-zone-configuration make-bind-zone-configuration > bind-zone-configuration? > (network bind-zone-configuration-network ; string > (default '())) > (class bind-zone-configuration-class ; string > (default '())) > (type bind-zone-configuration-type ; string > (default '())) > (file bind-zone-configuration-filename ; string > (default '()))) > > (define-record-type* > bind-configuration-file make-bind-configuration-file > bind-configuration-file? > > ;; > (config-options bind-configuration-file-config-options > (default (bind-options-configuration))) > > ;; list of > (config-zones bind-configuration-file-config-zones > (default (list (bind-zone-configuration > (network "localhost") > (class "IN") > (type "master") > (file "localhost.zone")) > (bind-zone-configuration > (network "0.0.127.in-addr.arpa") > (class "IN") > (type "master") > (file "127.0.0.zone")) > (bind-zone-configuration > (network (string-append "1.0.0.0.0.0.0.0.0.0." > "0.0.0.0.0.0.0.0.0.0." > "0.0.0.0.0.0.0.0.0.0." > "0.0.ip6.arpa")) > (class "IN") > (type "master") > (file "localhost.ip6.zone")) > (bind-zone-configuration > (network "255.in-addr.arpa") > (class "IN") > (type "master") > (file "empty.zone")) > (bind-zone-configuration > (network "0.in-addr.arpa") > (class "IN") > (type "master") > (file "empty.zone")) > (bind-zone-configuration > (network ".") > (class "IN") > (type "master") > (file "root.hint")))))) What is the intended behavior of these defaults? In what situations will they work, and in what situations will they not? It might be good to put a comment in that explains the intended default behavior and why it is reasonable. > (define-record-type* > bind-configuration make-bind-configuration > bind-configuration? > (config-file bind-configuration-config-file > (default (bind-configuration-file))) > (package bind-configuration-package ; > (default bind))) > > (define-syntax option > (syntax-rules () > ((_ key value) (if value > (list " " (string-join (list key value)) ";" "\n") > '())))) Does this need to be a macro? By the way, you could use string-append here, too, to make it simpler. > (define-syntax key/value > (syntax-rules () > ((_ (key value) rest ...) > (append (option key value) > (key/value rest ...))) > ((_) '()))) Does this need to be a macro? > (define (emit-bind-zones-config zone) > (match zone > (($ network class type file) > (list (string-join `(,(string-join (list "zone" > (string-append "\"" > network > "\"") > class "{\n")) > ,@(key/value ("type" type) > ("file" file)) > "};\n") > ""))))) > > (define (emit-bind-options-config options) > (match options > (($ user _ run-directory pid-file > listen-v4 listen-v6 listen-port > allow-recursion? allow-transfer? > allow-update? > version hostname server-id) Some of these slots (e.g., listen-v4) appear to be un-used. Instead of listing positional slots by name, maybe it would be better to bind the entire to a variable, and then use the accessor procedures (e.g., bind-options-configuration-listen-v4) to get just the attributes you need. This has the benefit of being more resilient to refactorings which change the order of fields in the record, also. I realize that a lot of the code in Guix relies on positional matching of slots like this, so I don't mind if you keep it as-is, but consider my suggestion as food for thought. > `("options {\n" > ,@(key/value ("directory" run-directory) > ("pid-file" pid-file) > ("allow-recursion" > (bind-configuration-statement-string allow-recursion?)) > ("allow-transfer" > (bind-configuration-statement-string allow-transfer?)) > ("allow-update" > (bind-configuration-statement-string allow-update?)) > ("version" version) > ("hostname" hostname) > ("server-id" server-id)) > "};\n")))) > > (define-gexp-compiler (bind-configuration-compiler > (file ) system target) > (match file > (($ config-file) > (match config-file > (($ config-options config-zones) > (apply text-file* "named.conf" > (append (fold append '() (map emit-bind-zones-config config-zones)) > (emit-bind-options-config config-options)))))))) > Is it necessary to define a gexp compiler here? I would have thought we could just invoke plain-file or text-file instead (see (guix) G-Expressions in the Guix manual). Why can't we? Other services do this; for example, see the service definitions in gnu/services/mail.scm. Also, is it possible for a user to pass in an existing configuration file to be used verbatim, or included somewhere in the config? Having an "escape hatch" like that seems useful for most services; perhaps it could be useful here, too. Footnotes: [1] https://unix.stackexchange.com/questions/13972/what-is-this-new-run-filesystem -- Chris