[PATCH] Restore logging of OpenSMTPd to /var/log/maillog.

  • Done
  • quality assurance status badge
Details
5 participants
  • Andreas Enge
  • Felix Lechner
  • Ludovic Courtès
  • Maxim Cournoyer
  • Tomas Volf
Owner
unassigned
Submitted by
Felix Lechner
Severity
normal

Debbugs page

F
F
Felix Lechner wrote on 3 Apr 11:04 -0700
(address . guix-patches@gnu.org)(name . Felix Lechner)(address . felix.lechner@lease-up.com)
8f60a2b7cd79da8cf4853affbdb2db4e7394c68c.1743703468.git.felix.lechner@lease-up.com
Please see this thread for details:


Change-Id: I485e040d680ccb39fa62e49d2e6ea916f047972c
---
gnu/services/mail.scm | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

Toggle diff (22 lines)
diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index b9a61163d5..5a627dbbaf 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1742,8 +1742,11 @@ (define (opensmtpd-shepherd-service config)
(documentation "Run the OpenSMTPD daemon.")
(start (let ((smtpd (file-append package "/sbin/smtpd")))
#~(make-forkexec-constructor
- (list #$smtpd "-f" #$config-file)
- #:pid-file "/var/run/smtpd.pid")))
+ (list #$smtpd
+ "-d"
+ "-f" #$config-file)
+ #:pid-file "/var/run/smtpd.pid"
+ #:log-file "/var/log/maillog")))
(stop #~(make-kill-destructor))))))
(define %opensmtpd-accounts

base-commit: 52ce48e754a77675da69e14d8f0dfa5fb76539ef
--
2.48.1
A
A
Andreas Enge wrote on 3 Apr 11:55 -0700
Re: OpenSMTP logging
(name . Felix Lechner)(address . felix.lechner@lease-up.com)
Z-7ZsQh2I32yeLIC@jurong
Hello,

Am Thu, Apr 03, 2025 at 11:13:50AM -0700 schrieb Felix Lechner via:
Toggle quote (2 lines)
> P.S. I linked to my Debbugs clone since Mumi did not pick it up.

this was probably just a question of time; I can see it now on issues.

Toggle quote (3 lines)
> I can confirm that logging in OpenSMTPd is broken. I fixed it locally
> with the patch in Bug#77501. [1]

This looks good to me as someone who knows very little about services...
I have looked at the shepherd manual, and in
it says the following about make-fork-exec-constructor:
"Note: This will not work as expected if the process “daemonizes”
(forks); in that case, you will need to pass #:pid-file, as explained
below."

Now that you do not daemonize, is #:pid-file still needed or useful?

#:log-file "/var/log/maillog"
And is this the good file name? Not mail.log?

Andreas
F
F
Felix Lechner wrote on 3 Apr 12:39 -0700
(name . Andreas Enge)(address . andreas@enge.fr)
87tt75c9vh.fsf@lease-up.com
Hi Andreas,

On Thu, Apr 03 2025, Andreas Enge wrote:

Toggle quote (2 lines)
> This looks good to me

Thanks for taking a look!

Toggle quote (2 lines)
> is #:pid-file still needed or useful?

No, it isn't. It also occurred to me after I had sent in the patch, and
moved onto something else.

As a side note, I'm not sure we have the luxury of making such minor
improvements. Guix is already a system with a thousand warts. My patch
definitely makes Guix better.

Either way, I'd be happy to remove it if we can reach an acceptance deal
per below.

Toggle quote (2 lines)
> And is this the good file name?

It isn't, but I think /var/log/maillog has a long tradition with
Sendmail.

The filename should actually be configurable, but I won't submit such a
patch unless you promise to accept it. I already have a gazillion
rebases because my patches, which I carry in my custom branch, are
almost never accepted.

Kind regards
Felix
A
A
Andreas Enge wrote on 8 Apr 02:14 -0700
[PATCH] services: opensmtpd: Add logging to a file.
(address . 77501@debbugs.gnu.org)
2bd63e09a5664ecc3bb8fcee114e44db4e88bca6.1744103663.git.andreas@enge.fr
From: Felix Lechner <felix.lechner@lease-up.com>

* gnu/services/mail.scm (opensmtpd-shepherd-service)[start]:
Add a command line flag to not daemonize. Drop #:pid-file.
Add #:log-file.

Change-Id: I485e040d680ccb39fa62e49d2e6ea916f047972c
Signed-off-by: Andreas Enge <andreas@enge.fr>
---
Hello,

so just to be sure before I push, the patch below works for you?

Andreas


gnu/services/mail.scm | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

Toggle diff (21 lines)
diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index b9a61163d5..0d5af95986 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1742,8 +1742,10 @@ (define (opensmtpd-shepherd-service config)
(documentation "Run the OpenSMTPD daemon.")
(start (let ((smtpd (file-append package "/sbin/smtpd")))
#~(make-forkexec-constructor
- (list #$smtpd "-f" #$config-file)
- #:pid-file "/var/run/smtpd.pid")))
+ (list #$smtpd
+ "-d"
+ "-f" #$config-file)
+ #:log-file "/var/log/maillog")))
(stop #~(make-kill-destructor))))))
(define %opensmtpd-accounts

base-commit: 43d30332bf3b4de0003578a7b5b27472b11183c7
--
2.49.0
T
T
Tomas Volf wrote on 8 Apr 11:30 -0700
(name . Andreas Enge)(address . andreas@enge.fr)
87ldsapku5.fsf@wolfsden.cz
Andreas Enge <andreas@enge.fr> writes:

Toggle quote (15 lines)
> diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
> index b9a61163d5..0d5af95986 100644
> --- a/gnu/services/mail.scm
> +++ b/gnu/services/mail.scm
> @@ -1742,8 +1742,10 @@ (define (opensmtpd-shepherd-service config)
> (documentation "Run the OpenSMTPD daemon.")
> (start (let ((smtpd (file-append package "/sbin/smtpd")))
> #~(make-forkexec-constructor
> - (list #$smtpd "-f" #$config-file)
> - #:pid-file "/var/run/smtpd.pid")))
> + (list #$smtpd
> + "-d"
> + "-f" #$config-file)
> + #:log-file "/var/log/maillog")))

Probably out of scope but would be cool to have this configurable, I
personally prefer /var/log/mail.log. I assume you have enough on your
hands already, so will send a separate patch later.

Tomas

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.
A
A
Andreas Enge wrote on 8 Apr 11:41 -0700
(name . Tomas Volf)(address . ~@wolfsden.cz)
Z_Vt1Cre0xNX-EvW@jurong
Am Tue, Apr 08, 2025 at 08:30:26PM +0200 schrieb Tomas Volf:
Toggle quote (4 lines)
> Probably out of scope but would be cool to have this configurable, I
> personally prefer /var/log/mail.log. I assume you have enough on your
> hands already, so will send a separate patch later.

Sounds good, thanks! You can already start the bikeshedding discussion
about what the default value should be :)

Andreas
F
F
Felix Lechner wrote on 8 Apr 11:52 -0700
(address . 77501@debbugs.gnu.org)
87v7relc3x.fsf@lease-up.com
Hi Andreas & Tomas,

On Tue, Apr 08 2025, Andreas Enge wrote:

Toggle quote (2 lines)
> You can already start the bikeshedding

No bikeshedding needed! Given the emerging consensus, which includes a
senior committer, I will happily prepare a patch that makes the name of
the log file configurable.

Like both of you, I also find mail.log more agreeable.

Anyway, it's easy to change that default later, but it would be harder
to change the name of the option, which is part of the API.

Is "log-file" okay?

Kind regards
Felix
F
F
Felix Lechner wrote on 8 Apr 14:32 -0700
[PATCH] Log OpenSMTPd to /var/log/mail.log via a configurable option. (Closes: #77501)
(address . 77501@debbugs.gnu.org)
b00a02bb307e60e40478f87af7507fa05d019cb0.1744147946.git.felix.lechner@lease-up.com
Please also see this thread:


Change-Id: I485e040d680ccb39fa62e49d2e6ea916f047972c
---
Hi,

This was deployed briefly on production equipment and appears to work.

Kind regards
Felix

doc/guix.texi | 4 ++++
gnu/services/mail.scm | 10 +++++++---
2 files changed, 11 insertions(+), 3 deletions(-)

Toggle diff (53 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index bee80cd4e2..c2640e5063 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -29219,6 +29219,10 @@ Mail Services
users and daemons on the local machine, as well as permitting email to
remote servers. Run @command{man smtpd.conf} for more information.
+@item @code{log-file} (default: @code{"/var/log/mail.log"})
+The file location for the OpenSMTPD log file. Logging occurs in the
+foreground via the Shepherd, i.e. OpenSMTPD does not detach.
+
@item @code{setgid-commands?} (default: @code{#t})
Make the following commands setgid to @code{smtpq} so they can be
executed: @command{smtpctl}, @command{sendmail}, @command{send-mail},
diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index ee61887369..cf23f76bc7 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1719,6 +1719,8 @@ (define-record-type* <opensmtpd-configuration>
(default '())) ; list of symbols
(config-file opensmtpd-configuration-config-file
(default %default-opensmtpd-config-file))
+ (log-file opensmtpd-configuration-log-file
+ (default "/var/log/mail.log"))
(setgid-commands? opensmtpd-setgid-commands? (default #t)))
(define %default-opensmtpd-config-file
@@ -1734,15 +1736,17 @@ (define %default-opensmtpd-config-file
(define (opensmtpd-shepherd-service config)
(match-record config <opensmtpd-configuration>
- (package config-file shepherd-requirement)
+ (package config-file log-file shepherd-requirement)
(list (shepherd-service
(provision '(smtpd))
(requirement `(pam loopback ,@shepherd-requirement))
(documentation "Run the OpenSMTPD daemon.")
(start (let ((smtpd (file-append package "/sbin/smtpd")))
#~(make-forkexec-constructor
- (list #$smtpd "-f" #$config-file)
- #:pid-file "/var/run/smtpd.pid")))
+ (list #$smtpd
+ "-d"
+ "-f" #$config-file)
+ #:log-file #$log-file)))
(stop #~(make-kill-destructor))))))
(define %opensmtpd-accounts

base-commit: c88f98bb3ca2616baea6b1e452cc900cc9c87503
--
2.49.0
L
L
Ludovic Courtès wrote 6 days ago
(name . Felix Lechner)(address . felix.lechner@lease-up.com)
87lds9xc5p.fsf@gnu.org
Hi,

Felix Lechner <felix.lechner@lease-up.com> skribis:

Toggle quote (6 lines)
> Please also see this thread:
>
> https://lists.gnu.org/archive/html/help-guix/2025-04/msg00009.html
>
> Change-Id: I485e040d680ccb39fa62e49d2e6ea916f047972c

Could you add a conventional commit log?


[...]

Toggle quote (8 lines)
> #~(make-forkexec-constructor
> - (list #$smtpd "-f" #$config-file)
> - #:pid-file "/var/run/smtpd.pid")))
> + (list #$smtpd
> + "-d"
> + "-f" #$config-file)
> + #:log-file #$log-file)))

I believe #:pid-file should be kept, no?

I would also keep the (list …) bit on a single line.

Thanks,
Ludo’.
A
A
Andreas Enge wrote 6 days ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
Z_btfsKQ-ALbLA9p@jurong
Am Wed, Apr 09, 2025 at 11:22:58PM +0200 schrieb Ludovic Courtès:
Toggle quote (2 lines)
> Could you add a conventional commit log?

This was there in my version of the patch a bit further up.

Toggle quote (9 lines)
> > #~(make-forkexec-constructor
> > - (list #$smtpd "-f" #$config-file)
> > - #:pid-file "/var/run/smtpd.pid")))
> > + (list #$smtpd
> > + "-d"
> > + "-f" #$config-file)
> > + #:log-file #$log-file)))
> I believe #:pid-file should be kept, no?

This was dropped following my remark above:
"I have looked at the shepherd manual, and in
it says the following about make-fork-exec-constructor:
'Note: This will not work as expected if the process “daemonizes”
(forks); in that case, you will need to pass #:pid-file, as explained
below.'
Now that you do not daemonize, is #:pid-file still needed or useful?"

The "-d" flag passed to #$smtpd prevents it from "daemonizing". So we
concluded that the #:pid-file thing was not needed (maybe it is needed,
but then the documentation should be adapted).

Andreas
L
L
Ludovic Courtès wrote 6 days ago
(name . Andreas Enge)(address . andreas@enge.fr)
87v7rctiuf.fsf@gnu.org
Andreas Enge <andreas@enge.fr> skribis:

Toggle quote (4 lines)
> The "-d" flag passed to #$smtpd prevents it from "daemonizing". So we
> concluded that the #:pid-file thing was not needed (maybe it is needed,
> but then the documentation should be adapted).

Oh I see, understood.

Thanks for explaining!

Ludo'.
A
A
Andreas Enge wrote 6 days ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
Z_fKoOQ2NF5LCR7k@jurong
I have added a commit message and a copyright header, turned the list
into one line and pushed.

Thanks to all!

Andreas
Closed
M
M
Maxim Cournoyer wrote 2 days ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ikn7wom2.fsf@gmail.com
Hi Andreas,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (6 lines)
> Andreas Enge <andreas@enge.fr> skribis:
>
>> The "-d" flag passed to #$smtpd prevents it from "daemonizing". So we
>> concluded that the #:pid-file thing was not needed (maybe it is needed,
>> but then the documentation should be adapted).

From my understanding, a pid file is still useful for *synchronization*
reasons, whether the service daemonizes or not. It's a token that lets
the service notify the init system that "OK, I've done my basic startup,
I'm ready to receive client requests".

That's been progressively replaced by socket activation in the systemd
world (and we support that via make-systemd-constructor, though I've
found some issue with it when using TLS connections -- to be reported
separately).

--
Thanks,
Maxim
A
A
Andreas Enge wrote 45 hours ago
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
Z_zAIrQzy22oxLpN@jurong
Hello Maxim,

Am Mon, Apr 14, 2025 at 09:52:53AM +0900 schrieb Maxim Cournoyer:
Toggle quote (5 lines)
> From my understanding, a pid file is still useful for *synchronization*
> reasons, whether the service daemonizes or not. It's a token that lets
> the service notify the init system that "OK, I've done my basic startup,
> I'm ready to receive client requests".

would you like to update the documentation and re-add the pid file
for this service here?

Andreas
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 77501@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 77501
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch