RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
Hi Chris,
this is fine, too.
Best regards, Goetz.
> -Original Message-
> From: Langer, Christoph
> Sent: Montag, 28. Januar 2019 23:28
> To: Lindenmaier, Goetz
> Cc: core-libs-dev@openjdk.java.net
> Subject: RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE
> when the zip file is located in a custom file system
>
> Hi Goetz,
>
> > Patching the file myself really helped to see there are no real changes.
> >
> > For the missing patch of the constructor I understand it was only an
> > arbitrary
> > reordering of arguments you omitted. Thus the whole chunk disappeared.
> > This makes sense, and in case someone downports the other change he
> > should be able to cope with that.
>
> Well, there were actually 3 failing hunks:
>
> --- ZipFileSystem.java
> +++ ZipFileSystem.java
> @@ -316,8 +326,8 @@
> IndexNode inode = getInode(path);
> if (inode == null)
> return null;
> -e = new Entry(inode.name, inode.isdir); // pseudo directory
> -e.method = METHOD_STORED;// STORED for dir
> +// pseudo directory, uses METHOD_STORED
> +e = new Entry(inode.name, inode.isdir, METHOD_STORED);
> e.mtime = e.atime = e.ctime = zfsDefaultTimeStamp;
> }
> } finally {
>
> This one just had to find its place -> it is included in my change (line 329
> of new
> file).
>
> @@ -1087,8 +1061,9 @@
> if (pos + CENHDR + nlen > limit) {
> zerror("invalid CEN header (bad header size)");
> }
> -IndexNode inode = new IndexNode(cen, nlen, pos);
> +IndexNode inode = new IndexNode(cen, pos, nlen);
> inodes.put(inode, inode);
> +
> // skip ext and comment
> pos += (CENHDR + nlen + elen + clen);
> }
>
> This one was the reordering of arguments when calling the "IndexNode"
> constructor itself.
>
> @@ -1806,7 +1777,7 @@
> }
>
> // constructor for cenInit() (1) remove tailing '/' (2) pad leading
> '/'
> -IndexNode(byte[] cen, int nlen, int pos) {
> +IndexNode(byte[] cen, int pos, int nlen) {
> int noff = pos + CENHDR;
> if (cen[noff + nlen - 1] == '/') {
> isdir = true;
>
> This one was the reordering of arguments of the "IndexNode" constructor
> itself.
>
> I think it's better to also take over the reordering of arguments of the
> constructor. Here's an updated webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8034802.11u.1/
>
> The updates are in line 1054 and 1770 of the new ZipFileSystem.java.
>
> Best regards
> Christoh
RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
Hi Goetz,
> Patching the file myself really helped to see there are no real changes.
>
> For the missing patch of the constructor I understand it was only an arbitrary
> reordering of arguments you omitted. Thus the whole chunk disappeared.
> This makes sense, and in case someone downports the other change he
> should be able to cope with that.
Well, there were actually 3 failing hunks:
--- ZipFileSystem.java
+++ ZipFileSystem.java
@@ -316,8 +326,8 @@
IndexNode inode = getInode(path);
if (inode == null)
return null;
-e = new Entry(inode.name, inode.isdir); // pseudo directory
-e.method = METHOD_STORED;// STORED for dir
+// pseudo directory, uses METHOD_STORED
+e = new Entry(inode.name, inode.isdir, METHOD_STORED);
e.mtime = e.atime = e.ctime = zfsDefaultTimeStamp;
}
} finally {
This one just had to find its place -> it is included in my change (line 329 of
new file).
@@ -1087,8 +1061,9 @@
if (pos + CENHDR + nlen > limit) {
zerror("invalid CEN header (bad header size)");
}
-IndexNode inode = new IndexNode(cen, nlen, pos);
+IndexNode inode = new IndexNode(cen, pos, nlen);
inodes.put(inode, inode);
+
// skip ext and comment
pos += (CENHDR + nlen + elen + clen);
}
This one was the reordering of arguments when calling the "IndexNode"
constructor itself.
@@ -1806,7 +1777,7 @@
}
// constructor for cenInit() (1) remove tailing '/' (2) pad leading '/'
-IndexNode(byte[] cen, int nlen, int pos) {
+IndexNode(byte[] cen, int pos, int nlen) {
int noff = pos + CENHDR;
if (cen[noff + nlen - 1] == '/') {
isdir = true;
This one was the reordering of arguments of the "IndexNode" constructor itself.
I think it's better to also take over the reordering of arguments of the
constructor. Here's an updated webrev:
http://cr.openjdk.java.net/~clanger/webrevs/8034802.11u.1/
The updates are in line 1054 and 1770 of the new ZipFileSystem.java.
Best regards
Christoh
RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
Hi Christoph, Patching the file myself really helped to see there are no real changes. For the missing patch of the constructor I understand it was only an arbitrary reordering of arguments you omitted. Thus the whole chunk disappeared. This makes sense, and in case someone downports the other change he should be able to cope with that. Thus, looks good! Best regards, Götz > -Original Message- > From: Langer, Christoph > Sent: Friday, January 25, 2019 11:40 AM > To: Lindenmaier, Goetz ; core-libs- > d...@openjdk.java.net > Subject: RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE > when the zip file is located in a custom file system > > Hi Goetz, > > > > > And I see much more differences if I compare the diffs. > > > > Please explain. > > > > unfortunately, the udiffs of the webrev look different in quite some parts to > what you see when you look at the commit on the hg web server > (http://hg.openjdk.java.net/jdk/jdk/rev/ba51515b64e5), although those are > the same. There were only 3 rejects when I applied the change to jdk11u > (you could try this yourself). > > > This is a big change, and it seems a lot more changed than resolving > > Hunks. Some hunks are missing altogether. > > > > For example, > > http://hg.openjdk.java.net/jdk/jdk/rev/ba51515b64e5, > > ZipFileSystem 2341 > > 2.341-IndexNode inode = new IndexNode(cen, nlen, pos); > > 2.342+IndexNode inode = new IndexNode(cen, pos, nlen); > > Is missing in your change. > > The constructor IndexNode(byte[] cen, int noff, int nlen, int pos) (jdk11u) > vs. > IndexNode(byte[] cen, int pos, int nlen) (jdk11) was actually the part where > the failing hunks originated. There was another patch pre 8034802 in jdk12 > which changed signature: > IndexNode(byte[] cen, int noff, int nlen, int pos) -to IndexNode(byte[] cen, > int nlen, int pos). > 8034802 did this change then: > IndexNode(byte[] cen, int nlen, int pos) -> IndexNode(byte[] cen, int pos, int > nlen) > > I chose not to touch this part but maybe this change should change the > constructor from IndexNode(byte[] cen, int noff, int nlen, int pos) -> > IndexNode(byte[] cen, int noff, int pos, int nlen), though, to take over this > change of order in arguments. It's a package private constructor that is only > called in one spot. What do you think? > > Thanks & Best regards > Christoph
RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
Hi Alan, thanks for your view on this backport request even though you aren't involved in jdk11u any longer. > The support for zip files located in file systems created by custom > providers was essentially a new feature in JDK 12 so it may not have got > a lot of usage yet. There has been a lot of other improvements in zipfs > in recent builds too and it's come a long way from its days as a demo > file system. I don't have any involvement in JDK 11 updates but one > suggestion is to x10 the testing of zipfs with a view to shaking out > issues in the mainline before doing a refresh. That is, it might be > saner to a bulk update after some bake time rather than picking out > specific changes. I think doing a bulk change to take over all the fixes that have been done on jdk.zipfs in the recent past seems quite appealing. Especially since those fixes don't break the Java API and probably don't even have CSRs associated. For the moment, I need this change back in jdk11u because with the SapMachine 11 build of OpenJDK we'd like to ship the preliminary version of the POSIX permission feature to be used by a customer. And this fix is somehow a prerequisite because it enables the testcase we have there. Without the fix, a call to "Files.createFile(fs.getPath(name), PosixFilePermissions.asFileAttribute(perms));" would not work. Fixing this is a nice side effect of the change of 8034802. So we'd kill two birds with one stone here. As far as I can see, this fix is disjunct in its core from the other fixes made to jdk.zipfs. However, for the jdk11u OpenJDK community I'd volunteer to do a bulk update of jdk.zipfs in the near future. Preferrably after we've come to a solution on 8213031 (POSIX permissions). Thanks Christoph
RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
Hi Goetz, > > And I see much more differences if I compare the diffs. > > Please explain. > unfortunately, the udiffs of the webrev look different in quite some parts to what you see when you look at the commit on the hg web server (http://hg.openjdk.java.net/jdk/jdk/rev/ba51515b64e5), although those are the same. There were only 3 rejects when I applied the change to jdk11u (you could try this yourself). > This is a big change, and it seems a lot more changed than resolving > Hunks. Some hunks are missing altogether. > > For example, > http://hg.openjdk.java.net/jdk/jdk/rev/ba51515b64e5, > ZipFileSystem 2341 > 2.341-IndexNode inode = new IndexNode(cen, nlen, pos); > 2.342+IndexNode inode = new IndexNode(cen, pos, nlen); > Is missing in your change. The constructor IndexNode(byte[] cen, int noff, int nlen, int pos) (jdk11u) vs. IndexNode(byte[] cen, int pos, int nlen) (jdk11) was actually the part where the failing hunks originated. There was another patch pre 8034802 in jdk12 which changed signature: IndexNode(byte[] cen, int noff, int nlen, int pos) -to IndexNode(byte[] cen, int nlen, int pos). 8034802 did this change then: IndexNode(byte[] cen, int nlen, int pos) -> IndexNode(byte[] cen, int pos, int nlen) I chose not to touch this part but maybe this change should change the constructor from IndexNode(byte[] cen, int noff, int nlen, int pos) -> IndexNode(byte[] cen, int noff, int pos, int nlen), though, to take over this change of order in arguments. It's a package private constructor that is only called in one spot. What do you think? Thanks & Best regards Christoph
Re: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
On 25/01/2019 08:01, Lindenmaier, Goetz wrote: Hi Christoph, This is a big change, and it seems a lot more changed than resolving Hunks. The support for zip files located in file systems created by custom providers was essentially a new feature in JDK 12 so it may not have got a lot of usage yet. There has been a lot of other improvements in zipfs in recent builds too and it's come a long way from its days as a demo file system. I don't have any involvement in JDK 11 updates but one suggestion is to x10 the testing of zipfs with a view to shaking out issues in the mainline before doing a refresh. That is, it might be saner to a bulk update after some bake time rather than picking out specific changes. -Alan.
RE: RFR [11u backport]: 8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system
Hi Christoph, This is a big change, and it seems a lot more changed than resolving Hunks. Some hunks are missing altogether. For example, http://hg.openjdk.java.net/jdk/jdk/rev/ba51515b64e5, ZipFileSystem 2341 2.341-IndexNode inode = new IndexNode(cen, nlen, pos); 2.342+IndexNode inode = new IndexNode(cen, pos, nlen); Is missing in your change. And I see much more differences if I compare the diffs. Please explain. Best regards, Goetz. > -Original Message- > From: core-libs-dev On Behalf > Of Langer, Christoph > Sent: Thursday, January 24, 2019 5:41 PM > To: core-libs-dev@openjdk.java.net > Subject: [CAUTION] RFR [11u backport]: 8034802: (zipfs) newFileSystem > throws UOE when the zip file is located in a custom file system > > Hello, > > please help review this backport for 8034802 to jdk11u: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8034802 > Original Commit: http://hg.openjdk.java.net/jdk/jdk/rev/ba51515b64e5 > Original review thread: https://mail.openjdk.java.net/pipermail/core-libs- > dev/2018-September/055490.html > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8034802.11u/ > > The patch did not apply cleanly - I had to manually resolve 3 hunks. Jtreg > tests > for zipfs all pass. > > Thanks > Christoph
