bug#47849: [PATCH] Add a jami-daemon service.

[Maxim Cournoyer]

Hi Leo,

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> 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.

Good catch!

>> +                    ,@(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.

I tried, but it seemed it had a bigger cognitive load to follow (or,
and=>, compose, list, cut all in the same expression), so ended up
keeping it as is.

> 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?

the %send-dbus-bus is a parameter; if we were to provide it as a default
value, it would be evaluated at the time the module is read (IIUC), but
I want its value to be read at run time, when the procedure is called
instead.

>> +          (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.

It's actually used as the last argument of the 'error' call :-).  I
added it as it was helpful to debug stderr messages from dbus-send.

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

Thanks a lot for having a look!  It's been long in the making.

Pushed as commit 69dcc24c9f.

Closing.

Maxim