GNU bug report logs - #41180
[PATCH] Add cachefilesd service.

Previous Next

Full log

Message #14 received at 41180 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Jean-Baptiste Note <jean-baptiste.note <at> m4x.org>
Cc: 41180 <at> debbugs.gnu.org
Subject: Re: [bug#41180] [PATCH] Add cachefilesd service.
Date: Tue, 19 May 2020 14:12:03 +0200

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.