Re: [pmg-devel] [RFC pmg-log-tracker 1/2] rfc3339 timestamps: counter-offset timezone

[Dominik Csapak <d.csapak@proxmox.com>]

High level comment:

isn't the api simply adding the local offset from 'now' ?
so here we add the offset depending on when it was logged,
meaning if the offset changed from the log to now it's still
off to what it was previously?

e.g.

log at 05:00 with +02:00 (epoch at 03:00 UTC)
gets converted to epoch at 05:00 UTC

now it's +01:00 (e.g. after summer/wintertime change)
result from the api is epoch at 04:00 UTC


so i guess we should either add the current timezone offset (since
that's what the api does, or fix it 'correct' by adding the timezone
info directly in the log-tracker for lines where we don't have the
info, and drop it in the api...

otherwise the code looks ok with one nit inline

On 6/27/23 21:46, Stoiko Ivanov wrote:
> the old time-format had no timezone-information so the API treats the
> result as 'timezoned-epoch' and subtracts the timezone-offset
> 
> In order to support both formats add the timezone-offset to the
> already offset time - so that the subtraction in the API is canceled
> out.
> 
> This can luckily be dropped once we drop support for traditional
> syslog format.
> 
> inspired by parse_rfc3339_do in proxmox-time
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>   src/main.rs | 27 +++++++++++++++++++++++----
>   1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/src/main.rs b/src/main.rs
> index e7bffd8..6c4a555 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -2263,12 +2263,31 @@ fn parse_time_with_year(data: &'_ [u8]) -> Option<(time_t, &'_ [u8])> {
>       timestamp_buffer[0..year_time_len].copy_from_slice(year_time);
>       timestamp_buffer[year_time_len..timestamp_len].copy_from_slice(timezone);
>   
> -    match proxmox_time::parse_rfc3339(unsafe {
> +    let epoch = match proxmox_time::parse_rfc3339(unsafe {
>           std::str::from_utf8_unchecked(&timestamp_buffer[0..timestamp_len])
>       }) {
> -        Ok(ltime) => Some((ltime, data)),
> -        Err(_err) => None,
> -    }
> +        Ok(ltime) => ltime,
> +        Err(_err) => return None,
> +    };
> +
> +    // FIXME: the traditional syslog format does not have timezone information, so the api-backend
> +    // shifts the time by the offset (i.e. parse_time_no_year returns timestamps in local time)
> +    // readd the TZ-offset here to match the broken format
> +    // drop after legacy parsing is dropped.
> +    let tz = timezone[0];
> +    if tz == b'Z' {
> +        return Some((epoch, data));
> +    }
> +
> +    let hours = (timezone[1] as i32 - 48) * 10 + (timezone[2] as i32 - 48);
> +    let mins = (timezone[4] as i32 - 48) * 10 + (timezone[5] as i32 - 48);

nit: i guess it's copied but wouldn't it make more sense to use b'0' instead of 48 here?
at least it would it make more obvious what it does

> +    let offset = hours * 3600 + mins * 60;
> +    let epoch = match tz {
> +        b'+' => epoch + offset as i64,
> +        b'-' => epoch - offset as i64,
> +        _ => unreachable!(), // already checked in parse_rfc3339
> +    };
> +    Some((epoch, data))
>   }
>   
>   fn parse_time_no_year(data: &'_ [u8], cur_year: i64, cur_month: i64) -> Option<(time_t, &'_ [u8])> {