Subject: [bug#76063] [PATCH 1/3] services: Add ‘system-log’ Shepherd service.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 06 Feb 2025 23:26:40 +0900

Hello!

Ludovic Courtès <[email-masked]> writes:

> * gnu/services/shepherd.scm (gexp-or-integer?)
> (gexp-or-string?, gexp-or-string-or-false?): New procedures.
> (system-log-configuration): New record type.

It's not as apparent due to coming into existence via a syntax, but
there many new symbols not listed here (such as each public field
accessor).

> (shepherd-system-log-service-type): New variable.
> * doc/guix.texi (Shepherd Services): Document it.
>
> Change-Id: I1e29fe0977eb4f8026ee6a2d2f282e269c8c00b4
> ---
>  doc/guix.texi             |  55 +++++++++++++++++++
>  gnu/services/shepherd.scm | 108 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index bb5f29277f..f52d79d549 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -45757,6 +45757,61 @@ Shepherd Services
>  on the @code{timer} service.
>  @end defvar
>  
> +@cindex system log service, from Shepherd
> +@cindex syslogd, Shepherd system log service
> +@defvar shepherd-system-log-service-type
> +This service implements a @dfn{system log}, reading messages
> +applications send to @file{/dev/log} and writing them to files or
> +terminals according to user-defined rules.  It provides functionality
> +traditionally implemented by @command{syslogd} programs.
> +
> +The value of services of this type must be a
> +@code{system-log-configuration} record, as described below.
> +@end defvar
> +
> +@c %start of fragment

Neat that you finally embraced define-configuration :-).

Side-topic: one day (TM), I'd like to work on a system that'd generate
and splice/update these fragments automatically in our doc, avoiding the
laborious copy-pasting.  

[...]

> diff --git a/gnu/services/shepherd.scm b/gnu/services/shepherd.scm
> index 328bfbedff..bc31285cfb 100644
> --- a/gnu/services/shepherd.scm
> +++ b/gnu/services/shepherd.scm
> @@ -31,6 +31,7 @@ (define-module (gnu services shepherd)
>    #:use-module ((guix diagnostics)
>                  #:select (define-with-syntax-properties formatted-message))
>    #:use-module (gnu services)
> +  #:use-module (gnu services configuration)
>    #:use-module (gnu services herd)
>    #:use-module (gnu packages admin)
>    #:use-module (ice-9 match)
> @@ -84,6 +85,17 @@ (define-module (gnu services shepherd)
>              shepherd-timer-service-type
>              shepherd-transient-service-type
>  
> +            system-log-configuration
> +            system-log-configuration?
> +            system-log-configuration-provision
> +            system-log-configuration-requirement
> +            system-log-configuration-message-destination
> +            system-log-configuration-date-format
> +            system-log-configuration-history-size
> +            system-log-configuration-max-silent-time
> +
> +            shepherd-system-log-service-type
> +
>              assert-valid-graph))
>  
>  ;;; Commentary:
> @@ -721,3 +733,99 @@ (define shepherd-transient-service-type
>  
>  This runs @command{rsync} in the background, as a service that you can inspect
>  with @command{herd status} and stop with @command{herd stop}.")))
> +
> +
> +;;;
> +;;; System log.
> +;;;
> +
> +(define (gexp-or-false? x)
> +  (or (gexp? x) (not x)))
> +
> +(define (gexp-or-integer? x)
> +  (or (gexp? x) (integer? x)))
> +
> +(define (gexp-or-string? x)
> +  (or (gexp? x) (string? x)))
> +
> +(define (gexp-or-string-or-false? x)
> +  (or (gexp-or-string? x) (not x)))
> +
> +(define-configuration system-log-configuration
> +  (provision
> +   (list-of-symbols '(system-log syslogd))
> +   "The name(s) of the system log service."
> +   empty-serializer)
> +  (requirement
> +   (list-of-symbols '(root-file-system))
> +   "Dependencies of the system log service."
> +   empty-serializer)
> +  (kernel-log-file
> +   (gexp-or-string-or-false #~(kernel-log-file))
> +   "File from which kernel messages are read, @file{/dev/kmsg} by default."
> +   empty-serializer)
> +  (message-destination
> +   (gexp-or-false #f)
> +   "This gexp must evaluate to a procedure that, when passed a log message,
> +returns the list of files to write it to; @code{#f} is equivalent to using
> +@code{(default-message-destination-procedure)}.  @pxref{System Log Service,,,
> +shepherd, The GNU Shepherd Manual}, for information on how to write that
> +procedure."

I think you should use just @xref here, since it's at the beginning of a
sentence and not within parentheses.

> +   empty-serializer)
> +  (date-format
> +   (gexp-or-string #~default-logfile-date-format)
> +   "String or string-valued gexp providing a specifying how to format

s/providing a //

> +timestamps in log file.  It must be a valid string for @code{strftime},
> +including delimiting space---e.g., @code{\"%c \"} for a format identical to
> +that of traditional syslogd implementations."

Perhaps add a reference to @samp{man 3 strftime}.

> +   empty-serializer)
> +  (history-size
> +   (gexp-or-integer #~(default-log-history-size))
> +   "Number of logging messages kept in memory for the purposes of making them
> +available to @command{herd status system-log}."
> +   empty-serializer)
> +  (max-silent-time
> +   (gexp-or-integer #~(default-max-silent-time))
> +   "Time after which a mark is written to log files if nothing was logged
> +during that time frame."
> +   empty-serializer))

It seems the whole above configuration is not to be serialized, so you
should use define-configuration/no-serialization, which would save some
boring repetition of 'empty-serializer'.

> +
> +(define shepherd-system-log-service-type
> +  (shepherd-service-type
> +   'shepherd-system-log
> +   (lambda (config)

I'd bind the record values here using 'match-record', which would make
the part below more concise.

> +     (shepherd-service
> +      (documentation "Shepherd's built-in system log (syslogd).")
> +      (provision (system-log-configuration-provision config))
> +      (requirement (system-log-configuration-requirement config))
> +      (modules '((shepherd service system-log)
> +                 ((shepherd support) #:select (default-logfile-date-format))
> +                 ((shepherd logger) #:select (default-log-history-size))))
> +      (free-form
> +       #~(system-log-service #:provision
> +                             '#$(system-log-configuration-provision config)
> +                             #:requirement
> +                             '#$(system-log-configuration-requirement config)
> +
> +                             ;; XXX: As of Shepherd 1.0.1,
> +                             ;; 'default-message-destination-procedure' is not
> +                             ;; exported, hence this conditional.

Perhaps check if we have shepherd >= 1.0.2 (?) and put the
forward-looking code in this case, with the other code path ready to be
removed after some time, if it's easy to check for the shepherd version.

> +                             #$@(match (system-log-configuration-message-destination
> +                                        config)
> +                                  (#f #~())
> +                                  (value #~(#:message-destination #$value)))
> +
> +                             #:date-format
> +                             #$(system-log-configuration-date-format
> +                                config)

Not really relevant here, but that makes me recall that Guile currently
lacks monotonic time support (time that can't move backward the
adjusting the system clock).  It could make funny log time stamps.

See in srfi-19.scm:

--8<---------------cut here---------------start------------->8---
;; -- we define it to be the same as TAI.
;;    A different implementation of current-time-monotonic
;;    will require rewriting all of the time-monotonic converters,
;;    of course.

(define (current-time-monotonic)
  ;; Guile monotonic and TAI times are the same.
  (let ((tai (current-time-tai)))
    (make-time time-monotonic
               (time-nanosecond tai)
               (time-second tai))))
--8<---------------cut here---------------end--------------->8---

which as far as I can see uses CLOCK_REALTIME and not CLOCK_MONOTONIC as
it relies on gettimeofday(2), which has this note in its documentation:

--8<---------------cut here---------------start------------->8---
       The time returned by gettimeofday() is affected by discontinuous  jumps
       in  the system time (e.g., if the system administrator manually changes
       the system time).  If you need a monotonically  increasing  clock,  see
       clock_gettime(2).
--8<---------------cut here---------------end--------------->8---

Perhaps we should open an issue in Guile to remember to tacle this
deficiency?

> +                             #:history-size
> +                             #$(system-log-configuration-history-size config)
> +                             #:max-silent-time
> +                             #$(system-log-configuration-max-silent-time
> +                                config)))))
> +   (system-log-configuration)
> +   (description
> +    "The Shepherd's @code{system-log} service plays the role of traditional
> +@command{syslogd} daemons, reading data logged by daemons to @file{/dev/log}

Perhaps s/daemons/services/ for the second one, to avoid repetition and
clarify these are not the same daemons.

> +and writing it to several files in @file{/var/log} according to user-provided
> +dispatching rules.")))

Perhaps, 'several files under the @file{/var/log} directory, [...]'

Other than these small things, it LGTM.

Reviewed-by: Maxim Cournoyer <maxim.cournoyer@gmail>

-- 
Thanks,
Maxim