[systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @

[Zbigniew Jędrzejewski-Szmek]

On Sun, Mar 23, 2014 at 02:27:04PM -0400, Dave Reisner wrote:
> On Sun, Mar 23, 2014 at 06:30:02PM +0100, Tollef Fog Heen wrote:
> > ]] Dave Reisner 
> > 
> > > On Sun, Mar 23, 2014 at 05:27:08PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
> > > > On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote:
> > > > > Also adds a few tests for the absolute cases of parse_timestamp.
> > > > Yeah, that looks useful.
> > > > 
> > > > You don't test negative values. Maybe you could an example with a negative
> > > > value to the documentation and tests?
> > > 
> > > Negative epoch values? What would this represent?
> > 
> > Time before the epoch?
> > 
> > $ LANG=C date -d @-10000
> > Wed Dec 31 22:13:20 CET 1969
> > 
> > So date(1) already understands this.
> 
> Uggh, do we really need to document this? I don't see why/where it would
> actually be useful.
Well, you don't need to document this explictly, i.e. there's no need to
talk about negative values, since they're not really useful, but I think
it should be mentioned that the '@' is followed by a signed integer.

> FWIW, parse_timestamp sort of already handles
> negative values with the exception of anything that results in a
> timestamp of -1 -- mktime can't distinguish between a real timestamp of
> -1 and an invalid struct tm which it can't convert.
> 
>   $ journalctl --since='1969-12-31 18:59:59'
>   Failed to parse timestamp: 1969-12-31 18:59:59
> 
> But then there's also some inconsistent behavior which parse_timestamp
> already exposes when dealing with absolutes:
> 
>   $ journalctl --since='1969-12-31 18:59:58'
>   -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. --
>   <EOF>
> 
>   $ journalctl --since='-100 years'
>   -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. --
>   <logs follow>
> 
> I don't think this is going to work out so well when the return type
> involed (usec_t) is unsigned...
Are you sure? In your patch you use safe_atoi (not safe_atou). And on my
machine, time_t is defined as __time_t, which is __TIME_T_TYPE, __SYSCALL_SLONG_TYPE,
which looks signed to me. Actually, it seems that this safe_atoi should
be replaced by assert_cc(sizeof(time_t)==sizeof(long)) and safe_atoli.

Zbyszek