Re: [PATCH] gnu: rottlog: rotate messages daily.

[Ludovic Courtès]

Hello Jan!

Jan Nieuwenhuizen <address@hidden> skribis:

> Ludovic Courtès writes:
>
>> I would add /var/log/{secure,shepherd.log}, but weekly is probably
>> enough.
>
> Ok.  I implemented this by changing the rottlog package that already
> modifies etc/weekly.

The current ‘tweak-rc-weekly’ phase “fixes a bug” in etc/weekly, but I
don’t think we should do customization in the package itself.  Instead,
it would be more appropriate to generate/provide the configuration files
that we need, without further modifying the package itself.  WDYT?

> I'm not sure if sending SIGHUP to syslog is ok for shepherd.log or
> that would need to be a kill 1?  See attached.

GuixSD runs syslogd from Inetutils, so it’s not PID 1, fortunately.  ;-)

However, shepherd.log is written by PID_1, indeed, and we shouldn’t send
SIGHUP to PID 1.  However, I don’t think anything bad will happen if
rottlog moves shepherd.log and Shepherd doesn’t reopen this file.

>>> +(define* (rottlog-service
>>
>> This can be omitted.  It’s enough to expose ‘rottlog-service-type’ and
>> ‘rottlog-configuration’.
>
> Hmm.  I removed all the defaulting here and then removed the function
> alltogether, but then I cannot seem to do
>
>   (services (cons* (mcron-service) (rottlog-service) %base-services)))
>
> anymore.  Is that right?

Yes.  Instead, you have to write:

  (services (cons* (service rottlog-service-type
                            (rottlog-configuration …))
                   …))

> I'm feeling a bit stubborn keeping this but I thought that's what we
> want and my test showed me that this is needed.  So I kept a minimal of
> this function.  Please just remove+enlighten me how to use it if this
> can really go :-)

I hope the above sheds some light on this.  WDYT of this idea?

> From e8e489db62337c6e8ef03e745a56938566e078c8 Mon Sep 17 00:00:00 2001
> From: Jan Nieuwenhuizen <address@hidden>
> Date: Wed, 14 Sep 2016 19:04:38 +0200
> Subject: [PATCH 1/2] gnu: update rottlog: install guix-specific etc/weekly.
>
> * gnu/packages/admin.scm (rottlog): Install guix-specific etc/weekly
> for rotating /var/log/{messages,secureshepherd.log}.

As discussed above, I think this is the wrong place for this.

> From 59213cce5d6d4e41f9b6f321e3fef056cccc7c22 Mon Sep 17 00:00:00 2001
> From: Jan Nieuwenhuizen <address@hidden>
> Date: Thu, 8 Sep 2016 01:20:43 +0200
> Subject: [PATCH 2/2] gnu: services: add rottlog.
>
> * gnu/services/admin.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * doc/guix.texi (Log Rotation): Document it.

[...]

> +  (periodic-rotations rottlog-periodic-rotations ; list of (name file) tuples
> +                      (default `(("weekly"
> +                                  ,(file-append rottlog "/etc/weekly")))))

Here I would suggest adding somewhere:


  (define %rotated-files
    '("/var/log/messages" …))

  (define (syslog-rotation-config file)
    (string-append file " {
    …
    kill -HUP …
    …")))

  (define (simple-rotation-config file)
    ;; Same as above, but without “kill -HUP”.
    …)

  (define %default-rotations
    `(("weekly" . ,(plain-file "rottlog.weekly"
                               (string-append (string-join
                                               (map syslog-rotation-config
                                                    '("/var/log/messages"…)))
                                              (simple-rotation-config
                                               "shepherd.log"))))))

Does that make sense?

> +  (jobs rottlog-jobs                             ; list of <mcron-job>
> +        (default
> +          (list #~(job
> +                   '(next-hour '(0))
> +                   (lambda ()
> +                     (system (string-append #$rottlog "/sbin/rottlog"))))
> +                #~(job
> +                   '(next-hour '(12))
> +                   (lambda ()
> +                     (system (string-append #$rottlog "/sbin/rottlog"))))))))

Please move (list …) to a global variable, to avoid code duplication
when the macro is expanded.

Use ‘system*’ instead of ‘system’ (the latter does “/bin/sh -c”, which
is unnecessary here.)

Also, this should use the ‘rottlog’ package of <rottlog-configuration>,
which cannot be referred to from the default value, which is a constant.
Thus, you may have to change the default to #f, and generate the default
value upon #f.

TIA!

Ludo’.