vtim: Work with the "YYYY-MM-DDThh:mm:ss[.s]TZD" date-time format #4338
Conversation
This should allow compilers and static analyzers to complain about buffers too small, and this emphasizes the difference between the format and parse operations and how they treat their respective string arguments.
The only effective change is the optional white space relying on the OWS syntax (SP / HTAB) from the HTTP grammar instead of SP only.
As per an old recommendation on date-time formatting for better interoperability. This format also has the convenient property of being chronologically sorted.
|
|
|
I cherry-picked the first commit thinking that it should be an undisputed (flw) improvement. Other than that I really do not have much of an opinion, other than that I never particularly liked the "ISO 8601-style" timestamps precisely because of the lack of whitespace, which does not seem to match well with how my brain parses strings. |
| VSB_printf(vsb, "%04d-%02d-%02dT%02d:%02d:%02d", | ||
| tm.tm_year + 1900, tm.tm_mon, tm.tm_mday, | ||
| tm.tm_hour, tm.tm_min, tm.tm_sec); | ||
|
|
There was a problem hiding this comment.
Why even bother with the VSB ?
You are printing a fixed-length format, use strftime() directly ?
There was a problem hiding this comment.
Because what follows is of variable length.
Just to be safe, even though this is currently working fine with the handful of test cases. Spotted by @daghf.
And this is why I predicted that out of 4 properties, only 3 were interesting. Joke aside, since 8.0 is around the corner, I went away and used this format for the ban list as an example, here is what I get on my machine: I think that despite the lack of whitespace, this kind of output is more human-readable than the decimal number we have today. |
daghf
left a comment
daghf
left a comment
There was a problem hiding this comment.
I reviewed and approved this elsewhere, so I might as well do it here. LGTM
|
@dridi I think the ban list is a particularly bad example, because we said that the "number" would officially be opaque, with the only requirement to define global ordering. |
|
I went ahead with the ban list as an example based on this: varnish-cache/include/tbl/cli_cmds.h Lines 52 to 71 in 7f7b030 This column is described as "Time the ban was issued" and there is no (in)formal JSON schema, so I deemed this format appropriate for the "time" field. If my example is wrong, it was made in good faith with due diligence. |
|
Re @dridi
you absolutely did. The detail of the ban id becoming opaque is buried in the VDD23Q1 notes and not even particularly explicit there:
Also, I should have written "would officially become opaque". |
4 participants
This is a format shared by a W3C note and an old IETF RFC, and still in use 5000+ RFCs later.
It's not being used anywhere in this patch series (I'm using it in a separate project) but I think it could be very useful in certain areas where we print timestamps, like the ban list, the first (non-prefix) field of
SLT_Timestamprecords, and possibly others not popping off the top of my head.Three interesting properties of this specific format:
I'm submitting this independently of any usage, but I thought the 8.0 release could be a good occasion to "break" the output of CLI responses, or the layout of VSL records, or anything else where we print timestamps.