From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 7C5641FF191 for ; Tue, 9 Sep 2025 18:21:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A1E6BEF90; Tue, 9 Sep 2025 18:21:10 +0200 (CEST) Date: Tue, 9 Sep 2025 18:21:06 +0200 From: Stoiko Ivanov To: Christoph Heiss Message-ID: <20250909182106.4e5e37d5@rosa.proxmox.com> In-Reply-To: <20250901121229.833136-1-c.heiss@proxmox.com> References: <20250901121229.833136-1-c.heiss@proxmox.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1757434842837 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.064 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pmg-devel] applied: [PATCH pmg-api] dkim: local mail: fix `Date` header formatting under different locales X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: pmg-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pmg-devel-bounces@lists.proxmox.com Sender: "pmg-devel" Thanks for tackling this so quickly and for the through analysis in the forum! adding tests is very welcome! I applied the patch after some quick positive tests and looking it through, but caught an issue right after pushing (see comment inline). To not keep our public branch with the issue I pushed a follow-up for this. One meta-observation - I think it'd would have helped me if the new tests were added in a separate commit. comments/nits inline: On Mon, 1 Sep 2025 14:12:27 +0200 Christoph Heiss wrote: > This was reported in the community forum to cause problems if the > (shell) locale was set to something different than 'C' or 'en_US.UTF-8' > [0]. while first skimming over your patch I asked myself why this is needed, as I thought we set this somewhere for our daemons (which in case of a timer-triggered service (as pmgreport/pmgspamreport, wouldn't have helped either) - seems I was wrong. > Date: Mo, 25 Aug 2025 00:01:04 +0200 nit: Tuesday, Wednesday, Thursday, Saturday might be more easily noticable in this case > > instead of the correct format of: > > Date: Mon, 25 Aug 2025 00:01:04 +0200 > > ..snip.. > +=head3 format_date_header > + > +Returns a RFC2822-formatted timestamp for usage in the Date header. nit: writing RFC5322 might save a few people checking that both agree on the date-time format (they do ;) > + > +sub format_date_header { > + # ensure that we always use the right locale for formatting `Date` headers > + my $old_locale = setlocale(POSIX::LC_TIME, 'C') // 'C'; seems `setlocale` only returns the old-locale when called without a second argument (it's mentioned implicitly(I'd even say a bit hidden) in `perldoc POSIX`, a bit more noticable to me in `man 3 setlocale`): ``` If locale is NULL, the current locale is only queried, not modified ``` > + my $date = strftime('%a, %d %b %Y %T %z', @_); > + setlocale(POSIX::LC_TIME, 'C'); this would not restore the locale to the previously set value (I assume that was the intention) setlocale(POSIX::LC_TIME, $old_locale); would accomplish that (tested by printing the date a few times in this function > + > + return $date; > +} > + > 1; > diff --git a/src/tests/Makefile b/src/tests/Makefile > index dc35796..68f77e4 100644 > --- a/src/tests/Makefile > +++ b/src/tests/Makefile > @@ -4,7 +4,8 @@ export PERLLIB = .. > > all: > > -check: > +.PHONY: check nit: thanks for the cleanup, and fine to add these things in the patch touching the Makefile - but please mention them in the commit-message >..snip.. > > +}; > + > +subtest 'format_date_header works with other locales' => sub { > + # also check correctness under some other locale > + my $old_locale = setlocale(POSIX::LC_TIME); here saving the old locale and resetting after the tests seems correct...) > ..snip.. _______________________________________________ pmg-devel mailing list pmg-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel