[bug#47849] [PATCH v3 3/3] services: Add a service for Jami.

[Leo Prikler]

Hi,

Am Sonntag, den 01.08.2021, 02:58 -0400 schrieb Maxim Cournoyer:
> [...]

> +  (let* ((command `(,@(if dbus-send
> +                          (list dbus-send)
> +                          (list (%send-dbus-binary)))
You can use non-splicing comma notation for things that are never
supposed to be lists.
> +                    ,@(if (or bus (%send-dbus-bus))
> +                          (list (string-append "--bus="
> +                                               (or bus (%send-dbus-
> bus))))
> +                          '())
You use this style here, but
> +        (let* ((uid (or (and=> (or user (%send-dbus-user))
> +                               (compose passwd:uid getpwnam)) -1))
> +               (gid (or (and=> (or group (%send-dbus-group))
> +                               (compose group:gid getgrnam)) -1)))
this style here.  I personally think using and=> everywhere would make
things a little clearer.

More importantly, though
> +(define* (send-dbus #:key service path interface method
> +                    bus
> +                    dbus-send
> +                    user group
> +                    timeout
> +                    arguments)
If you e.g. write (bus (%send-dbus-bus)), I think the argument should
already be correctly passed upon the function call, no?

> +          (chown temp-port uid gid)))
> +      (lambda ()
> +        (let ((pid (fork+exec-command command
> +                                      #:user (or user (%send-dbus-
> user))
> +                                      #:group (or group (%send-dbus-
> group))
> +                                      #:log-file temp-file)))
> +          (match (waitpid pid)
> +            ((_ . status)
> +             (let ((exit-status (status:exit-val status))
> +                   (output (call-with-port temp-port get-string-
> all)))
> +               (if (= 0 exit-status)
> +                   output
> +                   (error "the send-dbus command exited with: "
> +                          command exit-status output)))))))
Since output is unused in the error case, I think you can move the get-
string-all there.  It would make a difference if you were to e.g. close
temp-port before that.

I did not look at the rest of this patch, but 1+2 LGTM.

Regards,