Hi, first of all, I suggest to use "jarDetails" instead of "jarPath" as category name. Because with this contribution we add the notion of jar file plus line of manifest to Exceptions occurring when parsing jar manifests. And if there were further Exception details to be added in the area of jar files, they could go under a category "jarDetails" as well. Would you agree? Then, in Manifest.java, the field "jarPathInExceptionText" could be renamed to "detailedExceptions".
As for the code, I have the following remarks: src/java.base/share/classes/java/util/jar/Manifest.java: You could further clean up the ordering of includes by sorting them alphabetically and add a blank line between lines 34/35. Line 52: I suggest an indentation of 8 chars Please use jarFilename as final. You'll have to initialize jarFilename in each constructor then or initialize it to null in the default constructor and call this constructor from all the other ctors except for the one taking the jarFilename as param. src/java.base/share/classes/sun/net/util/SocketExceptions.java please add an empty line between 32 and 33 Line 39: I suggest an indentation of 8 chars src/java.base/share/classes/sun/security/util/SecurityProperties.java Line 35: change comment opener to /** (from /*), then it's a real Javadoc Furthermore I suggest to change the wording to "Returns the value of the security property propName, which can be overridden by a system property of the same name" Best regards Christoph > -----Original Message----- > From: Baesken, Matthias > Sent: Dienstag, 11. September 2018 13:29 > To: Weijun Wang <weijun.w...@oracle.com> > Cc: Langer, Christoph <christoph.lan...@sap.com>; Sean Mullan > <sean.mul...@oracle.com>; security-dev@openjdk.java.net; core-libs- > d...@openjdk.java.net > Subject: RE: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > > I don't have a strong opinion on making Manifest.jarFilename final > > Hi, just making it final leads to compile errors anyway. > > Best regards, Matthias > > > > -----Original Message----- > > From: Weijun Wang <weijun.w...@oracle.com> > > Sent: Dienstag, 11. September 2018 13:04 > > To: Baesken, Matthias <matthias.baes...@sap.com> > > Cc: Langer, Christoph <christoph.lan...@sap.com>; Sean Mullan > > <sean.mul...@oracle.com>; security-dev@openjdk.java.net; core-libs- > > d...@openjdk.java.net > > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > > parsing of jar archives > > > > Attributes.java: > > > > - Line 377: Too long, add a break. > > > > Otherwise fine. > > > > I don't have a strong opinion on making Manifest.jarFilename final or a > > different property name. > > > > Thanks > > Max > > > > > On Sep 11, 2018, at 5:07 PM, Baesken, Matthias > > <matthias.baes...@sap.com> wrote: > > > > > > Hello, please check the new webrev : > > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.10/ > > > > > > I kept the jarPath category name . > > > > > > Best regards, Matthias > > > > > > > > >> -----Original Message----- > > >> From: Langer, Christoph > > >> Sent: Montag, 10. September 2018 21:30 > > >> To: Weijun Wang <weijun.w...@oracle.com>; Baesken, Matthias > > >> <matthias.baes...@sap.com> > > >> Cc: Sean Mullan <sean.mul...@oracle.com>; security- > > >> d...@openjdk.java.net; core-libs-...@openjdk.java.net > > >> Subject: RE: [RFR] 8205525 : Improve exception messages during > manifest > > >> parsing of jar archives > > >> > > >> Hi, > > >> > > >>>> do you think we need property > jdk.includeInExceptions=jar<File/Path> > > at > > >>> all, if we don't resolve the absolute path? > > >>> > > >>> I think so. File path is still sensitive. > > >>> > > >>> In fact, I tend to believe people usually use absolute paths for JAR > > >>> files > > (or > > >>> maybe made absolute by using a file:// URL somewhere inside JDK). > Do > > >> you > > >>> really see relative jar paths while testing this code change? > > >> > > >> I agree. > > >> > > >>>> small remark to the code: > > >>>> src/java.base/share/classes/sun/security/util/SecurityProperties.java > > >>>> 36 public static String privilegeGetOverridable(String propName) { > > >>>> > > >>>> Should that method really be public? At the moment it doesn't seem > to > > >> be > > >>> used outside of SecurityProperties. > > >>> > > >>> I like it to be public. There are quite some other such system/security > > >>> properties (Ex: jdk.serialFilter) that can make use of this method. > > >> > > >> Ok, maybe it should be named "priviledgedGetOverridable" then. > > >> > > >> @Matthias: > > >> Further small cleanups, as you touch the files: > > >> src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can > > >> remove "import java.util.Iterator;" > > >> > > >> src/java.base/share/classes/sun/net/util/SocketExceptions.java: > > >> line 41: indentation is ridiculously long, I think it should be 8 chars > > >> > > >> src/java.base/share/classes/sun/security/util/SecurityProperties.java: > > >> Indentation of lines 38-45 is too deep, should be 12. > > >> The 2 methods could use a brief Javadoc. > > >> > > >> Best regards > > >> Christoph > > > >
