Subject: Re: bug#50882: [PATCH] gnu: services: Add darkhttpd service

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 07 Jul 2022 14:02:36 -0400

tag 50882 moreinfo
thanks

Hello jgart,

Ludovic Courtès <[email-masked]> writes:

> Hi,
>
> jgart <[email-masked]> skribis:
>
>> * gnu/services/web.scm (<darkhttpd-configuration>): New record type.
>> (darkhttpd-accounts, darkhttpd-shepherd-service): New procedures.
>> (darkhttpd-service-type): New variable.
>> * doc/guix.texi (Web Services): Adds documentation for darkhttpd.
>
> Overall LGTM!  Some comments and suggestions below.
>
>> +@cindex darkhttpd
>> +@uref{https://unix4lyfe.org/darkhttpd/, darkhttpd} is a web server with a 
>> +focus on security and having a small memory footprint.
>> +
>> +Some security features are the following:
>> +
>> +@itemize
>> +@item Logging accesses, including Referer and User-Agent.
>> +@item Can chroot.
>> +@item Can drop privileges.
>> +@item Impervious to /../ sniffing.
>> +@item Times out idle connections.
>> +@item Drops overly long requests.
>> +@end itemize 
>
> I’d replace the bullet list with a simple sentence like: “Among other
> things, it can change root directories, drop privileges, it times out on
> idle connections and can drop overly long requests.”
>
>> +@deffn {Scheme Variable} darkhttpd-service-type
>> +This is the type of the darkhttpd service, whose value should be a
>> +@code{darkhttpd-service-type} object, as in this example:
>> +
>> +@lisp
>> +(service darkhttpd-service-type
>> +	 (darkhttpd-configuration
>
> Please don’t use tabs.
>
>> +@end table
>> +@end deftp
>>  @node Certificate Services
>
> Missing newline before @node.  :-)
>
>> +  (mimetypes               darkhttpd-configuration-mimetypes
>> +                           (default #f))
>> +  (default-mimetype        darkhttpd-configuration-default-mimetype
>
> Rather ‘mime-type’ (two words).
>
>> +(define darkhttpd-shepherd-service
>> +  (match-lambda
>> +    (($ <darkhttpd-configuration> package content port address 
>> +                                  maximum-connections log-file chroot? 
>> +                                  daemonize? index-file do-not-serve-listing?
>> +                                  mimetypes default-mimetype 
>> +                                  drop-user-priviledges drop-group-priviledges 
>> +                                  write-pid-file disable-keep-alive? 
>> +                                  forward forward-all 
>> +                                  no-server-id? enable-ipv6? 
>> +                                  user group)
>
> Rather use ‘match-record’ here, to make sure we’re getting the right
> fields.
>
>> +(define darkhttpd-accounts
>> +  (match-lambda
>> +    (($ <darkhttpd-configuration> _ _ _ _ _ _ _ _ 
>> +                                  _ _ _ _ _ _ _ _ 
>> +                                  _ _ user group)
>
> In such a case, simply call ‘darkhttpd-configuration-user’ and
> ‘darkhttpd-configuration-group’; it’s much less error-prone!
>
>> +(define darkhttpd-service-type
>> +  (service-type
>> +   (name 'guix)
>> +   (extensions
>> +    (list (service-extension account-service-type
>> +                             darkhttpd-accounts)
>> +          (service-extension shepherd-root-service-type
>> +                             darkhttpd-shepherd-service)))
>> +   (default-value (darkhttpd-configuration))))
>
> Please add a ‘description’ field.
>
> Could you also add a system test, under (gnu tests web)?  You can start
> by copying the nginx test; it should take around ~20 lines.

Friendly ping about the above requests from Ludovic :-).

Thanks,

Maxim