On 2020-07-22 23:07, Ludovic Courtès wrote:
> Hello Robin,

Hi

> Robin Green <greenrd@greenrd.org> skribis:
> 
>> * gnu/services/auditd.scm: Make auditd start successfully in the default case.
>> * gnu/services/aux-files/auditd/auditd.conf: New file.
>> * doc/guix.texi (Miscellaneous Services): Update docs to reflect changes.
> 
> Nice, it’s a good idea.  Some comments below:
> 
>> -(define-configuration auditd-configuration
>> -  (audit
>> -   (package audit)
>> -   "Audit package."))
>> +(define-record-type* <auditd-configuration>
> 
> I think we should keep using ‘define-configuration’, unless there’s a
> good reason to change.  WDYT?

I couldn't get it to work with ‘define-configuration’ - I kept getting
errors. I asked on #guix, and it was suggested that I do it this way
instead.

>> +  auditd-configuration make-auditd-configuration
>> +  auditd-configuration?
>> +  (audit           auditd-configuration-audit            ; package
>> +                   (default audit))
>> +  (configdir       auditd-configuration-configdir))      ; local-file
> 
> s/configdir/configuration-directory/, to be consistent with the rest of
> the code.

Done

> You can also set its default value.

I don't see the value in doing that, because the default is already set
elsewhere, and if the user wants to use a different package, they
probably also want to use a different configuration file than the
default one!

> 
>> +                  (auditd-configuration
>> +                   (configdir (local-file "aux-files/auditd" #:recursive? #t))))))
>> diff --git a/gnu/services/aux-files/auditd/auditd.conf b/gnu/services/aux-files/auditd/auditd.conf
>> new file mode 100644
>> index 0000000000..6e7555cf4c
>> --- /dev/null
>> +++ b/gnu/services/aux-files/auditd/auditd.conf
> 
> Since it’s a small file, I have a slight preference for using
> ‘plain-file’ + ‘computed-file’:
> 
>   (define auditd.conf
>     (plain-file …))
> 
>   (define %default-auditd-configuration-directory ;make it public
>     (computed-file "auditd"
>                    #~(begin
>                        (mkdir #$output)
>                        (copy-file #$auditd.conf
>                                   (string-append #$output "/auditd.conf")))))
> 
> WDYT?

Agreed - done