RFR: 8185855: Debug exception stacks should be clearer

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8185855: Debug exception stacks should be clearer

Seán Coffey
Looking to improve the stacktrace output made when debug mode is enabled
for java.security and sun.security classes. In the past, some of these
have led to confusion for end users. Best to add some context when we're
printing stacktrace for informational, debug, purposes.

https://bugs.openjdk.java.net/browse/JDK-8185855

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8185855/webrev/

The one minor functional change I've made is in
sun/security/util/AnchorCertificates.java

I figured that it's best to print some context around a stacktrace that
we're about to print rather than print it only in debug mode.

regards,
Sean.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8185855: Debug exception stacks should be clearer

Sean Mullan
On 12/5/17 9:27 AM, Seán Coffey wrote:

> Looking to improve the stacktrace output made when debug mode is enabled
> for java.security and sun.security classes. In the past, some of these
> have led to confusion for end users. Best to add some context when we're
> printing stacktrace for informational, debug, purposes.
>
> https://bugs.openjdk.java.net/browse/JDK-8185855
>
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8185855/webrev/
>
> The one minor functional change I've made is in
> sun/security/util/AnchorCertificates.java
> I figured that it's best to print some context around a stacktrace that
> we're about to print rather than print it only in debug mode.

I would probably put the e.printStackTrace inside the debug, seems odd
to have the trace dumped to stderr even if debug is not set.

Looks fine otherwise.

--Sean
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8185855: Debug exception stacks should be clearer

Anthony Scarpino
In reply to this post by Seán Coffey
On 12/05/2017 06:27 AM, Seán Coffey wrote:

> Looking to improve the stacktrace output made when debug mode is enabled
> for java.security and sun.security classes. In the past, some of these
> have led to confusion for end users. Best to add some context when we're
> printing stacktrace for informational, debug, purposes.
>
> https://bugs.openjdk.java.net/browse/JDK-8185855
>
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8185855/webrev/
>
> The one minor functional change I've made is in
> sun/security/util/AnchorCertificates.java
>
> I figured that it's best to print some context around a stacktrace that
> we're about to print rather than print it only in debug mode.
>
> regards,
> Sean.
>

I'm fine with the change.

Tony
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8185855: Debug exception stacks should be clearer

Seán Coffey
In reply to this post by Sean Mullan
Thanks for reviewing Sean. Not finding the cacerts file has always
resulted in the stacktrace going to stderr in the past. I guess there's
no harm putting it behind a debug flag. New diff for that file below.
I'll push later today if I hear nothing else.

diff --git
a/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
b/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
--- a/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
+++ b/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
@@ -187,6 +187,7 @@
              } catch (Exception e) {
                  // ignore, treat it like we have no keystore
                  if (debug != null) {
+                    debug.println("Debug info only. No keystore.");
                      e.printStackTrace();
                  }
                  return null;
@@ -261,7 +262,7 @@
                  loaded_one = true;
              } catch (Exception e) {
                  if (debug != null) {
-                    debug.println("error reading policy " + e);
+                    debug.println("Debug info only. Error reading
policy " + e);
                      e.printStackTrace();
                  }
                  // ignore that policy

Regards,
Sean.

On 05/12/17 20:41, Sean Mullan wrote:

> On 12/5/17 9:27 AM, Seán Coffey wrote:
>> Looking to improve the stacktrace output made when debug mode is
>> enabled for java.security and sun.security classes. In the past, some
>> of these have led to confusion for end users. Best to add some
>> context when we're printing stacktrace for informational, debug,
>> purposes.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8185855
>>
>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8185855/webrev/
>>
>> The one minor functional change I've made is in
>> sun/security/util/AnchorCertificates.java
>> I figured that it's best to print some context around a stacktrace
>> that we're about to print rather than print it only in debug mode.
>
> I would probably put the e.printStackTrace inside the debug, seems odd
> to have the trace dumped to stderr even if debug is not set.
>
> Looks fine otherwise.
>
> --Sean

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8185855: Debug exception stacks should be clearer

Seán Coffey
whoops - pasted the wrong file diffs in last email :

diff --git
a/src/java.base/share/classes/sun/security/util/AnchorCertificates.java
b/src/java.base/share/classes/sun/security/util/AnchorCertificates.java
--- a/src/java.base/share/classes/sun/security/util/AnchorCertificates.java
+++ b/src/java.base/share/classes/sun/security/util/AnchorCertificates.java
@@ -75,8 +75,8 @@
                  } catch (Exception e) {
                      if (debug != null) {
                          debug.println("Error parsing cacerts");
+                        e.printStackTrace();
                      }
-                    e.printStackTrace();
                  }
                  return null;
              }

Regards,
Sean.

On 06/12/17 10:20, Seán Coffey wrote:

> Thanks for reviewing Sean. Not finding the cacerts file has always
> resulted in the stacktrace going to stderr in the past. I guess
> there's no harm putting it behind a debug flag. New diff for that file
> below. I'll push later today if I hear nothing else.
>
> diff --git
> a/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
> b/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
> ---
> a/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
> +++
> b/src/java.base/share/classes/sun/security/provider/AuthPolicyFile.java
> @@ -187,6 +187,7 @@
>              } catch (Exception e) {
>                  // ignore, treat it like we have no keystore
>                  if (debug != null) {
> +                    debug.println("Debug info only. No keystore.");
>                      e.printStackTrace();
>                  }
>                  return null;
> @@ -261,7 +262,7 @@
>                  loaded_one = true;
>              } catch (Exception e) {
>                  if (debug != null) {
> -                    debug.println("error reading policy " + e);
> +                    debug.println("Debug info only. Error reading
> policy " + e);
>                      e.printStackTrace();
>                  }
>                  // ignore that policy
>
> Regards,
> Sean.
>
> On 05/12/17 20:41, Sean Mullan wrote:
>> On 12/5/17 9:27 AM, Seán Coffey wrote:
>>> Looking to improve the stacktrace output made when debug mode is
>>> enabled for java.security and sun.security classes. In the past,
>>> some of these have led to confusion for end users. Best to add some
>>> context when we're printing stacktrace for informational, debug,
>>> purposes.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8185855
>>>
>>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8185855/webrev/
>>>
>>> The one minor functional change I've made is in
>>> sun/security/util/AnchorCertificates.java
>>> I figured that it's best to print some context around a stacktrace
>>> that we're about to print rather than print it only in debug mode.
>>
>> I would probably put the e.printStackTrace inside the debug, seems
>> odd to have the trace dumped to stderr even if debug is not set.
>>
>> Looks fine otherwise.
>>
>> --Sean
>