[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#41180] [PATCH] Add cachefilesd service.
From: |
Mathieu Othacehe |
Subject: |
[bug#41180] [PATCH] Add cachefilesd service. |
Date: |
Tue, 19 May 2020 14:12:03 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Hello,
Overall, this looks nice! A few comments below. Note that you can merge
this patch with the documentation patch. It would also be nice to add
the associated system tests.
> +(define-record-type* <cachefilesd-configuration>
> + cachefilesd-configuration make-cachefilesd-configuration
> + cachefilesd-configuration?
> +
> + ;; <package-path>
> + (cachefilesd cachefilesd-configuration-cachefilesd
> + (default cachefilesd))
> +
You could write something more concise here by removing empty lines and
adding the 'type' comment on the same line.
> + (let ((secctx #$(cachefilesd-configuration-secctx config)))
> + (if secctx (format port "secctx ~a" secctx)))
You can use 'when' for one arm if conditions.
> +
> + ;; XXX factor this
> + (format port "brun ~a%\n"
> + #$(number->string
> + (cachefilesd-configuration-brun config)))
It would indeed be nice to factor it, maybe by creating an association
table with the symbol name as CAR and the matching procedure as
CDR. Something like:
--8<---------------cut here---------------start------------->8---
'(("frun" . cachefilesd-configuration-frun)
("bcull" . cachefilesd-configuration-bcull))
--8<---------------cut here---------------end--------------->8---
then you could iterate on that list.
> + (if #$(cachefilesd-configuration-nocull? config)
> + (display "nocull\n" port))
Same as above. You can use 'when' or 'unless' instead of "(if test
stmt)".
> + ;; Make sure the cache directory and pid dir exists
"dir" -> "directory".
> + ;; XXX shepherd pid file handling: no idea how shepherd does it
> + ;; and if it's going to conflict with cachefilesd's
Shepherd documentation says:
--8<---------------cut here---------------start------------->8---
When PID-FILE is true, it must be the name of a PID file associated
with the process being launched; the return value is the PID once
that file has been created. If PID-FILE does not show up in less
than PID-FILE-TIMEOUT seconds, the service is considered as failing
to start.
--8<---------------cut here---------------end--------------->8---
So I think you can remove this comment.
Thanks,
Mathieu