Hi Maxim,

thanks for the review. I just sent a v2 of the patch series.

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi,
>
> Roman Scherer <roman@burningswell.com> writes:
>
>> * gnu/services/sound.scm (speakersafetyd): Run as unprivileged user.
>
> Sounds good, perhaps also mention it adds a log file (is this related to
> this change?).

No, it's not related. I split the log file into another commit.

> [...]
>
>> +(define speakersafetyd-accounts
>> +  (match-record-lambda <speakersafetyd-configuration>
>> +      (blackbox-directory configuration-directory group log-file maximum-gain-reduction speakersafetyd user)
>
> Please break this and next long lines into something that fits < 80
> characters.  You can use the Emacs indentation hack to do so and leave a
> space after the opening parens to ensure it gets indented as data and
> not a procedure:
>
>          ( blackbox-directory configuration-directory ...
>            speakersafetyd user)
>

Interesting, didn't know about this Emacs indentation hack.

>> +    (list (user-group
>> +           (name group)
>> +           (system? #t))
>> +          (user-account
>> +           (name user)
>> +           (group group)
>> +           (system? #t)
>> +           (home-directory "/var/empty")
>> +           (shell (file-append shadow "/sbin/nologin"))
>> +           (supplementary-groups '("audio"))))))
>> +
>> +(define speakersafetyd-activation
>> +  (match-record-lambda <speakersafetyd-configuration>
>> +      (blackbox-directory configuration-directory group log-file maximum-gain-reduction speakersafetyd user)
>
> Line width > 80 columns.
>
>> +    (with-imported-modules (source-module-closure
>> +                            '((gnu build activation)
>> +                              (guix build utils)))
>
> Looks like you only use (gnu build activation), not (guix build utils)
> in the below snippet.
>
>> +      #~(begin
>> +          (use-modules (gnu build activation))
>> +          (let ((user (getpwnam #$user)))
>> +            (mkdir-p/perms "/run/speakersafetyd" user #o755)
>> +            (mkdir-p/perms "/var/lib/speakersafetyd" user #o755)
>> +            ;; Blackbox files contain audio recordings and might be sensitive information
>> +            (mkdir-p/perms #$blackbox-directory user #o700))))))
>>
>>  (define speakersafetyd-shepherd-service
>>    (match-record-lambda <speakersafetyd-configuration>
>> -      (blackbox-directory configuration-directory maximum-gain-reduction speakersafetyd)
>> +      (blackbox-directory configuration-directory group log-file maximum-gain-reduction speakersafetyd user)
>
> Line width > 80 columns.
>
>>      (shepherd-service
>>       (documentation "Run the speaker safety daemon")
>>       (provision '(speakersafetyd))
>> @@ -306,7 +345,11 @@ (define speakersafetyd-shepherd-service
>>                 (list #$(file-append speakersafetyd "/bin/speakersafetyd")
>>                       "--config-path" #$configuration-directory
>>                       "--blackbox-path" #$blackbox-directory
>> -                     "--max-reduction" (number->string #$maximum-gain-reduction))))
>> +                     "--max-reduction" (number->string #$maximum-gain-reduction))
>> +               #:group #$group
>> +               #:log-file #$log-file
>> +               #:supplementary-groups '("audio")
>> +               #:user #$user))
>>       (stop #~(make-kill-destructor)))))
>>
>>  (define speakersafetyd-service-type
>> @@ -324,7 +367,13 @@ (define speakersafetyd-service-type
>>             (compose list speakersafetyd-configuration-speakersafetyd))
>>            (service-extension
>>             profile-service-type
>> -           (compose list speakersafetyd-configuration-speakersafetyd))))
>> +           (compose list speakersafetyd-configuration-speakersafetyd))
>> +          (service-extension
>> +           account-service-type
>> +           speakersafetyd-accounts)
>> +          (service-extension
>> +           activation-service-type
>> +           speakersafetyd-activation)))
>
> nitpick but I like to put at least one argument on the same line unless
> respecting the 80 columns max width is challenging, as in:
>
> --8<---------------cut here---------------start------------->8---
> (service-extension account-service-type
>                    speakersafetyd-accounts)
> --8<---------------cut here---------------end--------------->8---
>
> etc.
>
> Other than these tiny details, it LGTM.  Could you please send a v2?