Hi Sherman,
The code changes look good to me, I was thinking about the test and
here are some observations:
test/java/util/zip/TestExtraTime.java
a. you have TZ of Asia/Shanghai, I am wondering if it would help to add
a TZ which
has DST for any DST computation issues that we might encounter.
Also perhaps a tests which zips a file in one TimeZone and test in another
TimeZone ?
If these are being addressed by SQE that is fine.
b. this test sets the default TimeZone, this could cause weird issues
for other downstream
tests, specifically when jtreg runs in the samevm or agentvm mode, so
either we run this test in a
discrete VM or reset the TimeZone using a finally.
Thanks
Kumar
Kumar, Alan,
Here is the latest webrev of the changes based on the latest CCC.
http://cr.openjdk.java.net/~sherman/8015666/webrev
-Sherman
On 07/02/2013 11:29 AM, Xueming Shen wrote:
On 06/28/2013 07:47 AM, Kumar Srinivasan wrote:
Some nits while reading the changes:
1. ZipEntry.java
a. typo:
+ * Sets the laste access time of the entry.
b. extra space
+ case EXTID_ZIP64 :
2. ZipOutputStream.java
I think it would be nice to have the flags 0x1, 0x2 and 0x4 defined
as constants, this will also help a casual reader as to what this
means.
Besides my previous concern with finish(), everything else appears
to be ok.
Kumar,
I have the dostime "cached" in XEntry, so the writeCEN() and
writeLOC() will
always write out the same local msdos time. The cache should help the
perf
a little, as the javaToDosTime() now only invoked once for the same
entry.
Nothing needs to be updated in unpack side now. (I took a look at the
API,
it appears there is no way to do anything on unpack side to workaround
this issue, without the possibility of breaking someone's code)
http://cr.openjdk.java.net/~sherman/8015666/webrev/
-Sherman
Kumar
On 06/27/2013 10:04 AM, Kumar Srinivasan wrote:
Hi Sherman,
I started looking at this, my initial comment, the Unpacker.unpack
does not close its output and we allow multiple pack files to be
concatenated,
I am assuming out.finish() will allow further jar files to be
appended ?
or would this cause a problem ?
No, out.finish() will not allow further entry appending. Then, it
appears
we need to have a different approach to "finish" the
Jar/ZipOutputStream.
What need to be done here is that either out.close/finish() need to be
invoked under the UTC locale as well (to output the time stamps in cen
table correctly). This is another "incompatible" change of the
previous
change, in which the msdosTime<->javaTime conversion no longer
occurs during the ZipEntry.set/getTime(), but during the read and
write
the ZipEntry from/to the zip file.
-Sherman
Kumar
Hi,
The zip time related changes[1] I pushed back last month appears
to have the compatibility risk of breaking existing code. The main
idea in that changeset is to use the more accurate and timezone
insensitive utc time stored in the extra field for the
ZipEntry.set/getTime()
if possible. However it turns out the reality is that the code
out there
might have already had some interesting workaround/hack solution
to workaround the problem that the time stamp stored in the
"standard'
zip entry header is a MS-DOS standard date/time, which is a "local
date/time" and sensitive to timezone, in which, if the zip is
archived
in time zone A (our implementation converts the "java" time to dos
time by using the default tz A) and then transferred/un-archived in
a different zone B (use default tz B to convert back to java
time), we
have a time stamp mess up. The "workaround" from pack200 for this
issue when pack/unpacking a jar file is to
"specify/recommend/suggest"
in its spec that the "time zone" in a jar file entry is assumed
to be "UTC",
so the pack/unpack200 implementation set the "default time" to utc
before the pack/unpack and set it back to the original after
that. It worked
"perfectly" for a roundtrip pack/unpacking, until the changeset
[2], in
which ZipEntry.getTime() (packing) returns a real utc time and
the following
ZipEntry.setTime() (unpacking), then mess up the MS-DOS date/time
entry, this is the root cause of this regression.
Given the facts that
(1) there are actually two real physical time stamps in a zip
file header,
one is in the date/time fields, which is
MS-DOS-local-date/time-with-2-
seconds-granularity , one is in the extra data field, which is
UTC-1-second
-granularity
(2) and there are applications over there that have dependency on
the
MS-DOS date/time stamp.
I'm proposing the following approach to add the functionality of
supporting
the "utc-date/time-with-1-second granularity" and keep the old
behavior
of the get/setTime() of the ZipEntry.
(1) keep the time/setTime()/getTime() for the MS-DOS standard
date/time.
To set via the old setTime() will only store the time into
zip's standard
date/time field, which is in MS-DOS date/time. And getTime()
only returns
the date/time from that field, when read from the zip
file/stream.
(2) add mtime/set/getLastModifiedTime() to work on the UTC time
fields,
and the last modified time set via the new method will also
set the "time",
and the getLastModifiedTime() also returns the "time", if
the UTC time
stamp fields are not set in the zip file header. The idea is
that for the new
application, the recommendation is to use
ZipEntry.set/getLastModifiedTime()
for better/correct time stamp, but the existing apps keep
the same behavior.
(3) jar and ZipOutputStream are updated to use the
set/getLastModifiedTime().
(4) Pack/unpack continues to use the set/getTime(), so the
current workaround
continues work. I will leave this to Kuma to decide how it
should be handled
going forward. (there are two facts need to be considered
here, a) the
existing jar file might not have the utc time instored, and
b) all "extra" data
are wiped out during the pack/unpacking process)
(5) additionally add another pair of atime/get/setLastAccessTime and
ctime/get/setCreationTime().
(6) The newly added 3 pairs of the m/a/ctime get/set methods use
the "new"
nio FileTime, instead of the "long". This may add some
additional cost of
conversion when working with them, but may also help improve
the
performance if the time stamps are directly from nio file
system when
get/set XYZTime. Good/bad?
http://cr.openjdk.java.net/~sherman/8015666/webrev/
Comment, option and suggestion are appreciated.
-Sherman
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/90df6756406f
