From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id F0A8588392 for ; Wed, 5 Jan 2022 20:01:42 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E25CDC384 for ; Wed, 5 Jan 2022 20:01:42 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 8DFD7C373 for ; Wed, 5 Jan 2022 20:01:40 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5967845F19 for ; Wed, 5 Jan 2022 20:01:40 +0100 (CET) Date: Wed, 5 Jan 2022 20:01:37 +0100 From: Stoiko Ivanov To: Wolfgang Bumiller Cc: pmg-devel@lists.proxmox.com Message-ID: <20220105200137.4680770a@rosa.proxmox.com> In-Reply-To: <20220104135723.111515-1-w.bumiller@proxmox.com> References: <20220104135723.111515-1-w.bumiller@proxmox.com> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.262 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [main.rs, woboq.org, time.rs] Subject: Re: [pmg-devel] [PATCH log-tracker] drop 'time' dependency 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: , X-List-Received-Date: Wed, 05 Jan 2022 19:01:43 -0000 huge thanks for taking the time ;) to clean-up! On Tue, 4 Jan 2022 14:57:23 +0100 Wolfgang Bumiller wrote: > We're using a very old version of it and the functions we > actually use are available in glibc anyway. > > The only difference I found was that the result of > glibc's `strptime()`'s `%s` does *not* want an additional > `.to_local()` call anymore... > > ... which was weird anyway. since I'm always a bit cautious with "weird" in context of time(zone) handling (which I do consider a bit brittle) - I tried to find out why this was needed before and not anymore (took me quite a while) glibc's strptime - calls localtime_r on the parsed number: https://code.woboq.org/userspace/glibc/time/strptime_l.c.html#678 OTOH the time crate (in version 0.1.42) skips this step (or rather on the contrary actively calls `at_utc` on the argument (which calls sys::time_to_utc_tm with an tm struct with an offset of '0') in any case since sys::time_to_utc_tm and sys::time_to_local_tm only differ in the comment: ``` // FIXME: Add timezone logic ``` I am quite confident that your version is more correct (or at least matches glibc's behaviour better (might also explain why there's `at_utc` and `at` in the crate but not an explicit `at_local` ;) Additionally I did a bit of testing (and am really grateful for the tests we already have in the repository) - and in addition to the code - the output also looks correct With that: Reviewed-By: Stoiko Ivanov Tested-By: Stoiko Ivanov (I'm not directly applying it, in the hope that someone more experienced with the log-tracker could also throw a quick glance at this) small nit (maybe as hint to the person applying): > > As a minor optimization I pass the format string as `&CStr` > to avoid the memcpy. > > (The CString::new() call in `strptime()` only happens during > parameter parsing) > > Signed-off-by: Wolfgang Bumiller > --- > Cargo.toml | 1 - > src/main.rs | 75 ++++++++--------------- > src/time.rs | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 194 insertions(+), 51 deletions(-) > create mode 100644 src/time.rs > > diff --git a/Cargo.toml b/Cargo.toml > index a975d39..7c31157 100644 > --- a/Cargo.toml > +++ b/Cargo.toml > @@ -14,4 +14,3 @@ anyhow = "1" > clap = "2.32" > flate2 = "1.0" > libc = "0.2.48" > -time = "0.1.42" > diff --git a/src/main.rs b/src/main.rs > index fb06463..bf9783b 100644 > --- a/src/main.rs > +++ b/src/main.rs > @@ -15,6 +15,9 @@ use libc::time_t; > > use clap::{App, Arg}; > > +mod time; > +use time::{Tm, CAL_MTOD}; > + > fn main() -> Result<(), Error> { > let matches = App::new(clap::crate_name!()) > .version(clap::crate_version!()) > @@ -114,7 +117,7 @@ fn main() -> Result<(), Error> { > ) > .get_matches(); > > - let mut parser = Parser::new(); > + let mut parser = Parser::new()?; > parser.handle_args(matches)?; > > println!("# LogReader: {}", std::process::id()); > @@ -144,12 +147,12 @@ fn main() -> Result<(), Error> { > > println!( > "# Start: {} ({})", > - time::strftime("%F %T", &parser.start_tm)?, > + time::strftime(c_str!("%F %T"), &parser.start_tm)?, > parser.options.start > ); > println!( > "# End: {} ({})", > - time::strftime("%F %T", &parser.end_tm)?, > + time::strftime(c_str!("%F %T"), &parser.end_tm)?, > parser.options.end > ); > > @@ -1743,10 +1746,10 @@ struct Parser { > } > > impl Parser { > - fn new() -> Self { > - let ltime = time::now(); > + fn new() -> Result { > + let ltime = Tm::now_local()?; > > - Self { > + Ok(Self { > sentries: HashMap::new(), > fentries: HashMap::new(), > qentries: HashMap::new(), > @@ -1760,12 +1763,12 @@ impl Parser { > count: 0, > buffered_stdout: BufWriter::with_capacity(4 * 1024 * 1024, std::io::stdout()), > options: Options::default(), > - start_tm: time::empty_tm(), > - end_tm: time::empty_tm(), > + start_tm: Tm::zero(), > + end_tm: Tm::zero(), > ctime: 0, > string_match: false, > lines: 0, > - } > + }) > } > > fn free_sentry(&mut self, sentry_pid: u64) { > @@ -1976,40 +1979,38 @@ impl Parser { > } > > if let Some(start) = args.value_of("start") { > - if let Ok(res) = time::strptime(start, "%F %T") { > - self.options.start = mkgmtime(&res); > + if let Ok(res) = time::strptime(start, c_str!("%F %T")) { > + self.options.start = res.as_utc_to_epoch(); > self.start_tm = res; > - } else if let Ok(res) = time::strptime(start, "%s") { > - let res = res.to_local(); > - self.options.start = mkgmtime(&res); > + } else if let Ok(res) = time::strptime(start, c_str!("%s")) { > + self.options.start = res.as_utc_to_epoch(); > self.start_tm = res; > } else { > bail!("failed to parse start time"); > } > } else { > - let mut ltime = time::now(); > + let mut ltime = Tm::now_local()?; > ltime.tm_sec = 0; > ltime.tm_min = 0; > ltime.tm_hour = 0; > - self.options.start = mkgmtime(<ime); > + self.options.start = ltime.as_utc_to_epoch(); > self.start_tm = ltime; > } > > if let Some(end) = args.value_of("end") { > - if let Ok(res) = time::strptime(end, "%F %T") { > - self.options.end = mkgmtime(&res); > + if let Ok(res) = time::strptime(end, c_str!("%F %T")) { > + self.options.end = res.as_utc_to_epoch(); > self.end_tm = res; > - } else if let Ok(res) = time::strptime(end, "%s") { > - let res = res.to_local(); > - self.options.end = mkgmtime(&res); > + } else if let Ok(res) = time::strptime(end, c_str!("%s")) { > + self.options.end = res.as_utc_to_epoch(); > self.end_tm = res; > } else { > bail!("failed to parse end time"); > } > } else { > - let ltime = time::now(); > - self.options.end = mkgmtime(<ime); > - self.end_tm = ltime; > + // FIXME: just use libc::time(NULL) here this comment is addressed and can be removed > + self.options.end = unsafe { libc::time(std::ptr::null_mut()) }; > + self.end_tm = Tm::at_local(self.options.end)?; > } > > if self.options.end < self.options.start { > @@ -2171,30 +2172,6 @@ fn get_or_create_fentry( > } > } > > -fn mkgmtime(tm: &time::Tm) -> time_t { > - let mut res: time_t; > - > - let mut year = tm.tm_year as i64 + 1900; > - let mon = tm.tm_mon; > - > - res = (year - 1970) * 365 + CAL_MTOD[mon as usize]; > - > - if mon <= 1 { > - year -= 1; > - } > - > - res += (year - 1968) / 4; > - res -= (year - 1900) / 100; > - res += (year - 1600) / 400; > - > - res += (tm.tm_mday - 1) as i64; > - res = res * 24 + tm.tm_hour as i64; > - res = res * 60 + tm.tm_min as i64; > - res = res * 60 + tm.tm_sec as i64; > - > - res > -} > - > const LOGFILES: [&str; 32] = [ > "/var/log/syslog", > "/var/log/syslog.1", > @@ -2397,8 +2374,6 @@ fn parse_time<'a>( > Some((ltime, data)) > } > > -const CAL_MTOD: [i64; 12] = [0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334]; > - > type ByteSlice<'a> = &'a [u8]; > /// Parse Host, Service and PID at the beginning of data. Returns a tuple of (host, service, pid, remaining_text). > fn parse_host_service_pid(data: &[u8]) -> Option<(ByteSlice, ByteSlice, u64, ByteSlice)> { > diff --git a/src/time.rs b/src/time.rs > new file mode 100644 > index 0000000..5851496 > --- /dev/null > +++ b/src/time.rs > @@ -0,0 +1,169 @@ > +//! Time support library. > +//! > +//! Contains wrappers for `strftime`, `strptime`, `libc::localtime_r`. > + > +use std::ffi::{CStr, CString}; > +use std::fmt; > +use std::mem::MaybeUninit; > + > +use anyhow::{bail, format_err, Error}; > + > +/// Calender month index to *non-leap-year* day-of-the-year. > +pub const CAL_MTOD: [i64; 12] = [0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334]; > + > +/// Shortcut for generating an `&'static CStr`. > +#[macro_export] > +macro_rules! c_str { > + ($data:expr) => {{ > + let bytes = concat!($data, "\0"); > + unsafe { ::std::ffi::CStr::from_bytes_with_nul_unchecked(bytes.as_bytes()) } > + }}; > +} > + > +// Wrapper around `libc::tm` providing `Debug` and some helper methods. > +pub struct Tm(libc::tm); > + > +impl std::ops::Deref for Tm { > + type Target = libc::tm; > + > + fn deref(&self) -> &libc::tm { > + &self.0 > + } > +} > + > +impl std::ops::DerefMut for Tm { > + fn deref_mut(&mut self) -> &mut libc::tm { > + &mut self.0 > + } > +} > + > +// (or add libc feature 'extra-traits' but that's the only struct we need it for...) > +impl fmt::Debug for Tm { > + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { > + f.debug_struct("Tm") > + .field("tm_sec", &self.tm_sec) > + .field("tm_min", &self.tm_min) > + .field("tm_hour", &self.tm_hour) > + .field("tm_mday", &self.tm_mday) > + .field("tm_mon", &self.tm_mon) > + .field("tm_year", &self.tm_year) > + .field("tm_wday", &self.tm_wday) > + .field("tm_yday", &self.tm_yday) > + .field("tm_isdst", &self.tm_isdst) > + .field("tm_gmtoff", &self.tm_gmtoff) > + .field("tm_zone", &self.tm_zone) > + .finish() > + } > +} > + > +/// These are now exposed via `libc`, but they're part of glibc. > +mod imports { > + extern "C" { > + pub fn strftime( > + s: *mut libc::c_char, > + max: libc::size_t, > + format: *const libc::c_char, > + tm: *const libc::tm, > + ) -> libc::size_t; > + > + pub fn strptime( > + s: *const libc::c_char, > + format: *const libc::c_char, > + tm: *mut libc::tm, > + ) -> *const libc::c_char; > + } > +} > + > +impl Tm { > + /// A zero-initialized time struct. > + #[inline(always)] > + pub fn zero() -> Self { > + unsafe { std::mem::zeroed() } > + } > + > + /// Get the current time in the local timezone. > + pub fn now_local() -> Result { > + Self::at_local(unsafe { libc::time(std::ptr::null_mut()) }) > + } > + > + /// Get a local time from a unix time stamp. > + pub fn at_local(time: libc::time_t) -> Result { > + let mut out = MaybeUninit::::uninit(); > + Ok(Self(unsafe { > + if libc::localtime_r(&time, out.as_mut_ptr()).is_null() { > + bail!("failed to convert timestamp to local time"); > + } > + out.assume_init() > + })) > + } > + > + /// Assume this is an UTC time and convert it to a unix time stamp. > + /// > + /// Equivalent to `timegm(3)`, the gmt equivalent of `mktime(3)`. > + pub fn as_utc_to_epoch(&self) -> libc::time_t { > + let mut year = self.0.tm_year as i64 + 1900; > + let mon = self.0.tm_mon; > + > + let mut res: libc::time_t = (year - 1970) * 365 + CAL_MTOD[mon as usize]; > + > + if mon <= 1 { > + year -= 1; > + } > + > + res += (year - 1968) / 4; > + res -= (year - 1900) / 100; > + res += (year - 1600) / 400; > + > + res += (self.0.tm_mday - 1) as i64; > + res = res * 24 + self.0.tm_hour as i64; > + res = res * 60 + self.0.tm_min as i64; > + res = res * 60 + self.0.tm_sec as i64; > + > + res > + } > +} > + > +/// Wrapper around `strftime(3)` to format time strings. > +pub fn strftime(format: &CStr, tm: &Tm) -> Result { > + let mut buf = MaybeUninit::<[u8; 64]>::uninit(); > + > + let size = unsafe { > + imports::strftime( > + buf.as_mut_ptr() as *mut libc::c_char, > + 64, > + format.as_ptr(), > + &tm.0, > + ) > + }; > + if size == 0 { > + bail!("failed to format time"); > + } > + let size = size as usize; > + > + let buf = unsafe { buf.assume_init() }; > + > + std::str::from_utf8(&buf[..size]) > + .map_err(|_| format_err!("formatted time was not valid utf-8")) > + .map(str::to_string) > +} > + > +/// Wrapper around `strptime(3)` to parse time strings. > +pub fn strptime(time: &str, format: &CStr) -> Result { > + let mut out = MaybeUninit::::uninit(); > + > + let time = CString::new(time).map_err(|_| format_err!("time string contains nul bytes"))?; > + > + let end = unsafe { > + imports::strptime( > + time.as_ptr() as *const libc::c_char, > + format.as_ptr(), > + out.as_mut_ptr(), > + ) > + }; > + > + if end.is_null() { > + bail!("failed to parse time string {:?}", time); > + } > + > + Ok(Tm(unsafe { out.assume_init() })) > +}