On 7/13/17 5:34 AM, Langer, Christoph wrote:
Hi Daniel,
here is the updated webrev:
http://cr.openjdk.java.net/~clanger/webrevs/8183123.1/
<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.1/>
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
No comments.
test/sun/management/jdp/JdpOnTestCase.java
No comments.
Thumbs up.
Dan
To me it looks ok, I’ll update copyright header when I submit. Thumbs
up from you?
Thanks
Christoph
*From:*Andrew Leonard [mailto:[email protected]]
*Sent:* Freitag, 7. Juli 2017 15:20
*To:* [email protected]
*Cc:* [email protected]; Langer, Christoph
<[email protected]>
*Subject:* Re: RFR 8183123 : JDP packets have no processId context set
Hi Daniel,
Thank you for the review. You actually make a good observation, I had
intended to just change that method implementation to use the
ProcessHandle class, however the pid() method returns a "long" and the
containing method returns an Integer, so it needs "lossy" casting to
an (int) before "boxing" to an Integer. Thinking again about this,
given this is just a private method within this class that is only
called from one place, it seems cleaner to change the private method
to return a Long object, and change the calling instance
appropriately. I also see if I look at the javadoc for
ProcessHandle.pid() that it can in "theory" return
UnsupportOperationException, so I have also handled that. I have a new
webrev, which I will ask Christoph to upload...
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: [email protected]
<mailto:[email protected]>
From: "Daniel D. Daugherty" <[email protected]
<mailto:[email protected]>>
To: Andrew Leonard <[email protected]
<mailto:[email protected]>>,
[email protected]
<mailto:[email protected]>
Date: 05/07/2017 21:27
Subject: Re: RFR 8183123 : JDP packets have no processId context set
------------------------------------------------------------------------
On 6/29/17 7:57 AM, Andrew Leonard wrote:
Hi All,
Please can I get some review feedback for my changes for this issue:
https://bugs.openjdk.java.net/browse/JDK-8183123
The webrev patch has been uploaded here:
http://cr.openjdk.java.net/~clanger/webrevs/8183123.0/
<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.0/>
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
L137 return (int)ProcessHandle.current().pid();
The return type is Integer. Why not cast to "Integer" instead
of "int"?
test/sun/management/jdp/JdpOnTestCase.java
The test update just verifies that a non-NULL PROCESS_ID is found, but
doesn't verify the format (integer) of the return. Of course, a
platform
independent format for PROCESS_ID might be problematic... For example,
in some versions of Cygwin, I've seen negative values for PIDs...
Thumbs up. If you change the cast I don't need to see a new webrev.
Dan_
_
Essentially the fix entails:
- Replacing invalid process id query logic with call to
ProcessHandle.current().getPid().
- Update testcase to cover the failing scenario. Thus it fails without
my patch, and succeeds with it.
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: [email protected]
<mailto:[email protected]>
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with
number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with
number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
