Y2038 support for the Zip format #2549
Conversation
| timestamps that may not be compatible with other Zip implementations. | ||
| .Pp | ||
| To note is that high-resolution timestamps are necessary to support | ||
| dates beyond 2038 so in that case they're always written. |
There was a problem hiding this comment.
We should not require an option to support dates beyond 2038.
There was a problem hiding this comment.
Looking at your implementation, you're not requiring this to be set for dates beyond 2038. This controls the writing of timestamps with accuracy finer than 1s.
The "compatible" comment is not relevant. Zip is pretty well designed in this regard; readers just ignore extensions they don't understand. Compatibility is only an issue when it might cause a reader to fail to read the entry at all.
There was a problem hiding this comment.
I'll change the documentation to remove that comment.
| * holds mtime, so we format it separately. Also, we write | ||
| * it only if we didn't write NTFS times. | ||
| */ | ||
| if (!times_written |
There was a problem hiding this comment.
This doesn't make sense to me. Some readers will only understand UT, some will only understand the NTFS field. Shouldn't we write both?
There was a problem hiding this comment.
Thinking of it, we probably should for the case we're using the NTFS field to support sub-second resolution but should we for the case we're supporting post-2038 dates? After all in the latter case we're using the NTFS field because the times won't fit in the UT field.
There was a problem hiding this comment.
Also, I was writing only the NTFS field out of the same kind of reasoning why we only write ux but not Ux for the UID/GID: we're already avoiding to write redundant extra fields for the other data we store there despite supporting them.
|
I think this should really be 3 separate PRs. One for consolidating the time functions. (That part looks pretty reasonable.) The added support for the NTFS timestamp field to Zip format should be its own PR so we can talk about the details of how that works. As you can see, I have some questions about that. (And the cleanup to libarchive/test/test_write_format_zip_large.c should be its own small PR.) |
|
I split the PR so there should be only the NTFS timestamp field support left in here. |
Adding support for NTFS time extra data to be able to read or write dates beyond Y2038. It also allows to write dates with better than second resolution.
|
|
||
| /* Check if time fits in 32-bits Unix time */ | ||
| char | ||
| fits_in_unix(int64_t secs) { |
There was a problem hiding this comment.
This does not belong here. "Unix time" is not 32 bits. It's whatever size the particular platform and/or format are expecting. I think this and the next function should be static functions inside the Zip support files, since they're only relevant in that context.
| /* Unix sec to DOS time. */ | ||
| uint32_t unix_to_dos(int64_t secs); | ||
| /* Check if time fits in DOS time */ | ||
| char fits_in_dos(int64_t secs); |
There was a problem hiding this comment.
As above, these are Zip-specific and don't belong here.
Adding Y2038 support to the Zip format reader and writer, through support of the NTFS time extra data.