[pmg-devel] [RFC log-tracker] rfc3339: move timezone offset compatibility code to

[pmg-devel] [RFC log-tracker] rfc3339: move timezone offset compatibility code to

[Mira Limbeck]

old time format parsing code

The compatibility code was added to the new rfc3339 code path temporarily so
that the old code path would not be changed before the PMG 8 release.

Now move it to the old time format code to make sure the rfc3339 code path
works as expected. Since we have all the information we need (year, month,
day, hours, minutes, seconds, timezone), there's no need for a workaround in
this code path.

The change needs to be accompanied by one in pmg-api MailTracker.pmg to
keep the time displayed in the GUI the same for the old time format, and
correct for the new rfc3339 format.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
After talking to Dominik this might be best sent as an RFC for further
discussions. As such I haven't touched the tests yet.
This change requires changing all tests, including the new rfc3339 ones.

Any code that uses the pmg-log-tracker directly may behave differently
now, since the timestamps it outputs are different than before.

 src/main.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/main.rs b/src/main.rs
index e55f17b..67d804f 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -2255,11 +2255,11 @@ fn parse_time(
     cur_month: i64,
     timezone_offset: time_t,
 ) -> Option<(time_t, &'_ [u8])> {
-    parse_time_with_year(data, timezone_offset)
-        .or_else(|| parse_time_no_year(data, cur_year, cur_month))
+    parse_time_with_year(data)
+        .or_else(|| parse_time_no_year(data, cur_year, cur_month, timezone_offset))
 }
 
-fn parse_time_with_year(data: &'_ [u8], timezone_offset: time_t) -> Option<(time_t, &'_ [u8])> {
+fn parse_time_with_year(data: &'_ [u8]) -> Option<(time_t, &'_ [u8])> {
     let mut timestamp_buffer = [0u8; 25];
 
     let count = data.iter().take_while(|b| **b != b' ').count();
@@ -2287,12 +2287,12 @@ fn parse_time_with_year(data: &'_ [u8], timezone_offset: time_t) -> Option<(time
         std::str::from_utf8_unchecked(&timestamp_buffer[0..timestamp_len])
     }) {
         // TODO handle timezone offset in old code path instead
-        Ok(ltime) => Some((ltime + timezone_offset, data)),
+        Ok(ltime) => Some((ltime, data)),
         Err(_err) => None,
     }
 }
 
-fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64) -> Option<(time_t, &'_ [u8])> {
+fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64, timezone_offset: time_t) -> Option<(time_t, &'_ [u8])> {
     if data.len() < 15 {
         return None;
     }
@@ -2397,7 +2397,7 @@ fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64) -> Option<(
         _ => &data[1..],
     };
 
-    Some((ltime, data))
+    Some((ltime - timezone_offset, data))
 }
 
 type ByteSlice<'a> = &'a [u8];
-- 
2.39.2

[pmg-devel] [RFC api] MailTracker: remove timezone offset

[Mira Limbeck]

The timezone offset is moved to `pmg-log-tracker` in the
compatibility code path. This way the new rfc3339 timestamp, which is
already in UTC, can be used as is.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
This change only makes sense in combination with the log-tracker one.
Please DO NOT apply any of those 2 patches without the other one.

 src/PMG/API2/MailTracker.pm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/PMG/API2/MailTracker.pm b/src/PMG/API2/MailTracker.pm
index b8b25ad..ffda4b5 100644
--- a/src/PMG/API2/MailTracker.pm
+++ b/src/PMG/API2/MailTracker.pm
@@ -43,8 +43,6 @@ my $run_pmg_log_tracker = sub {
 
     my $logids = {};
 
-    my $timezone = tz_local_offset();;
-
     if (defined(my $id = $includelog)) {
 	if ($id =~ m/^Q([a-f0-9]+)R([a-f0-9]+)$/i) {
 	    $logids->{$1} = 1;
@@ -128,7 +126,7 @@ my $run_pmg_log_tracker = sub {
 	    } elsif ($line =~ m/^TO:([0-9A-F]+):([0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]+)>\s+\((\S+)\)$/) {
 		my $new = {
 		    size => $entry->{size} // 0,
-		    time => hex($1) - $timezone,
+		    time => hex($1),
 		    qid => $2,
 		    dstatus => $3,
 		    from => $4,
@@ -174,7 +172,7 @@ my $run_pmg_log_tracker = sub {
 	    } elsif ($line =~ m/^TO:([0-9A-F]+):(T[0-9A-F]+L[0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]*)>$/) {
 		my $e = {};
 		$e->{client} = $entry->{client} if defined($entry->{client});
-		$e->{time} = hex($1) - $timezone;
+		$e->{time} = hex($1);
 		$e->{id} = $2;
 		$e->{dstatus} = $3;
 		$e->{from} = $4;
-- 
2.39.2

Re: [pmg-devel] [RFC log-tracker] rfc3339: move timezone offset compatibility code to

[Dominik Csapak]

On 7/18/23 18:01, Mira Limbeck wrote:
> old time format parsing code
> 
> The compatibility code was added to the new rfc3339 code path temporarily so
> that the old code path would not be changed before the PMG 8 release.
> 
> Now move it to the old time format code to make sure the rfc3339 code path
> works as expected. Since we have all the information we need (year, month,
> day, hours, minutes, seconds, timezone), there's no need for a workaround in
> this code path.
> 
> The change needs to be accompanied by one in pmg-api MailTracker.pmg to
> keep the time displayed in the GUI the same for the old time format, and
> correct for the new rfc3339 format.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> After talking to Dominik this might be best sent as an RFC for further
> discussions. As such I haven't touched the tests yet.
> This change requires changing all tests, including the new rfc3339 ones.
> 
> Any code that uses the pmg-log-tracker directly may behave differently
> now, since the timestamps it outputs are different than before.
> 

as we already talked off-list, i think we basically have 3 ways forward with this:

* wait for the 9.x release and hope we don't forget to do this
* make the change opt-in and reverse it with 9.x (or maybe remove the old code path then)
   with this, we could change it in the api now, and just leave the cli interface
   compatible by default
* define it as a bug, so we can simply fix it, regardless if the behaviour is now different

i'm either for 2 or 3, but of those either is fine with me (3 is ofc less work)

any input on that @stoiko?

Re: [pmg-devel] [RFC api] MailTracker: remove timezone offset

[Stoiko Ivanov]

Thanks for addressing this and for the patches!

gave it a spin with a few logs from a longer period of time.
in general it looks ok (with my rudimentary tests)

one issue is that the starttime and endtime parameters you can provide to
the API call seem to not have been shifted by the offset.
(i.e. when searching for mails sent in CEST the start-end times in the
results are shifted by 2h based on the entry)



On Tue, 18 Jul 2023 18:01:01 +0200
Mira Limbeck <m.limbeck@proxmox.com> wrote:

> The timezone offset is moved to `pmg-log-tracker` in the
> compatibility code path. This way the new rfc3339 timestamp, which is
> already in UTC, can be used as is.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> This change only makes sense in combination with the log-tracker one.
> Please DO NOT apply any of those 2 patches without the other one.
> 
>  src/PMG/API2/MailTracker.pm | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PMG/API2/MailTracker.pm b/src/PMG/API2/MailTracker.pm
> index b8b25ad..ffda4b5 100644
> --- a/src/PMG/API2/MailTracker.pm
> +++ b/src/PMG/API2/MailTracker.pm
> @@ -43,8 +43,6 @@ my $run_pmg_log_tracker = sub {
>  
>      my $logids = {};
>  
> -    my $timezone = tz_local_offset();;
> -
>      if (defined(my $id = $includelog)) {
>  	if ($id =~ m/^Q([a-f0-9]+)R([a-f0-9]+)$/i) {
>  	    $logids->{$1} = 1;
> @@ -128,7 +126,7 @@ my $run_pmg_log_tracker = sub {
>  	    } elsif ($line =~ m/^TO:([0-9A-F]+):([0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]+)>\s+\((\S+)\)$/) {
>  		my $new = {
>  		    size => $entry->{size} // 0,
> -		    time => hex($1) - $timezone,
> +		    time => hex($1),
>  		    qid => $2,
>  		    dstatus => $3,
>  		    from => $4,
> @@ -174,7 +172,7 @@ my $run_pmg_log_tracker = sub {
>  	    } elsif ($line =~ m/^TO:([0-9A-F]+):(T[0-9A-F]+L[0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]*)>$/) {
>  		my $e = {};
>  		$e->{client} = $entry->{client} if defined($entry->{client});
> -		$e->{time} = hex($1) - $timezone;
> +		$e->{time} = hex($1);
>  		$e->{id} = $2;
>  		$e->{dstatus} = $3;
>  		$e->{from} = $4;

Re: [pmg-devel] [RFC log-tracker] rfc3339: move timezone offset compatibility code to

[Stoiko Ivanov]

Thanks for the patch!

looks ok - but as talked off-list and also hinted in the api-patch reply I
sent - if possible we could maybe get rid of the issue that the
start+endtime provided in the GUI is interpreted differently than the
current option, which is yet again different to what I'd expect:
Testcase is '2023-09-21 13:52' as starttime '2023-09-21 15:00' as endtime
* with both patches applied the first mail shown is from 15:52 (offset +2h)
* with the currently released version the first mail is shown is from
  14:52 (offset +1h)
* I personally would expect no offset ;)
potentially we could not consider the client's timezone too much - and
define that the results are based on the server's timezone (or more
correctly on what's actually written in the logs)
If we want to consider the timezone of the client we should account for DST
i.e. if the client is currently (in February) in CET, that they would've
been in CEST in September - but I think that might be quite too much of a
hassle - and the break of displaying one time in the information line vs.
another in the actual logs sounds confusing

please adapt the tests for the next version

two tiny nits inline:

On Tue, 18 Jul 2023 18:01:00 +0200
Mira Limbeck <m.limbeck@proxmox.com> wrote:

> old time format parsing code
if possible - get this information also into the subject line (70
character limit)

> 
>..snip..
> @@ -2287,12 +2287,12 @@ fn parse_time_with_year(data: &'_ [u8], timezone_offset: time_t) -> Option<(time
>          std::str::from_utf8_unchecked(&timestamp_buffer[0..timestamp_len])
>      }) {
>          // TODO handle timezone offset in old code path instead
this patch fixes the TODO :) - get rid of it
> -        Ok(ltime) => Some((ltime + timezone_offset, data)),
> +        Ok(ltime) => Some((ltime, data)),
>          Err(_err) => None,
>      }
>  }
>  
> -fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64) -> Option<(time_t, &'_ [u8])> {
> +fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64, timezone_offset: time_t) -> Option<(time_t, &'_ [u8])> {
>      if data.len() < 15 {
>          return None;
>      }
> @@ -2397,7 +2397,7 @@ fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64) -> Option<(
>          _ => &data[1..],
>      };
>  
> -    Some((ltime, data))
> +    Some((ltime - timezone_offset, data))
>  }
>  
>  type ByteSlice<'a> = &'a [u8];