Hi Max : > > - No need to "import java.security.Security".
Sure I can remove this, it is a leftover. > - In the updated read() method, I think there is no need to use an "int > offset" > parameter. "int lineNumber" is enough and you can modify it and return it > without a new local variable. > Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... } > In fact, I have a small concern on the hardcoded file name here, because > when verifying a signed jar, the META-INF/X.SF file is also parsed into a > Manifest object and if it's invalid similar exceptions will be thrown. I don't > have a nice solution now. Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 ) Or ( to mention that it is about a manifest from the jar ) <jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 ) Best regards, Matthias > -----Original Message----- > From: Weijun Wang <weijun.w...@oracle.com> > Sent: Freitag, 31. August 2018 04:32 > To: Baesken, Matthias <matthias.baes...@sap.com> > Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan > <sean.mul...@oracle.com>; Chris Hegarty <chris.hega...@oracle.com>; > security-...@openjdk.java.net; core-libs-dev@openjdk.java.net > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > Some more comments: > > Attributes.java > > - No need to "import java.security.Security". > > - In the updated read() method, I think there is no need to use an "int > offset" > parameter. "int lineNumber" is enough and you can modify it and return it > without a new local variable. > > - I feel like calling Attributes::getErrorPosition from Manifest a little > strange. > Maybe it's better to define the method in Manifest and call it from > Attributes. First, I think putting the method in a higher level object makes > more sense. Second, the position is for the whole Manifest and not an > Attributes section (I mean the line number is counted from the first line of > the manifest). > > > On Aug 30, 2018, at 10:50 PM, Baesken, Matthias > <matthias.baes...@sap.com> wrote: > > > > > > > > Hi Max, probably we should add the info about the MANIFEST.MF , for > example : > > change getErrorPosition to > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s > hare/classes/java/util/jar/Attributes.java.udiff.html > > > > > > static String getErrorPosition(String filename, final int lineNumber) { > > if (filename == null || !jarPathInExceptionText) { > > return "META-INF/MANIFEST.MF line:" + lineNumber; > > } > > > > final File file = new File(filename); > > return AccessController.doPrivileged(new PrivilegedAction<String>() { > > public String run() { > > return file.getAbsolutePath() + "!META-INF/MANIFEST.MF > > line:" + > lineNumber; > > I prefer "file:line" which is more compact and more commonly used. > > In fact, I have a small concern on the hardcoded file name here, because > when verifying a signed jar, the META-INF/X.SF file is also parsed into a > Manifest object and if it's invalid similar exceptions will be thrown. I don't > have a nice solution now. if we want to show the .SF name also, we will need > a public API because SignatureFileVerifier.java is inside sun.security.util. > Something like Manifest(InputStream,jarName,entryName)? > > Sorry for making this complicated. > > Thanks > Max > > > } > > ..... > > > > > > Best regards, Matthias > > > > > > > >> -----Original Message----- > >> From: Weijun Wang <weijun.w...@oracle.com> > >> Sent: Donnerstag, 30. August 2018 16:04 > >> To: Baesken, Matthias <matthias.baes...@sap.com> > >> Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan > >> <sean.mul...@oracle.com>; Chris Hegarty <chris.hega...@oracle.com>; > >> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net > >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > >> parsing of jar archives > >> > >> > >> > >>> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias > >> <matthias.baes...@sap.com> wrote: > >>> > >>>> - What will the output look like? Is it "/tmp/x.jar:100"? > >>>> > >>> > >>> Yes it look like this : > >>> > >>> line too long (/testdata/jars/file_with_long_line_1.jar:2) > >> > >> Is this a little misleading? I think you mean > >> > >> /testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2 > >> > >> Thanks > >> Max