RFR 8183123 : JDP packets have no processId context set

[Andrew Leonard]

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: andrew_m_leonard at uk.ibm.com 




From:   "Daniel D. Daugherty" <daniel.daugherty at oracle.com>
To:     Andrew Leonard <andrew_m_leonard at uk.ibm.com>, 
serviceability-dev at openjdk.java.net
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/
 
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: andrew_m_leonard at uk.ibm.com 

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170707/c0632be3/attachment.html>