RFR 8183123 : JDP packets have no processId context set

[Daniel D. Daugherty]

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:andrew_m_leonard at uk.ibm.com]
> *Sent:* Freitag, 7. Juli 2017 15:20
> *To:* daniel.daugherty at oracle.com
> *Cc:* serviceability-dev at openjdk.java.net; Langer, Christoph 
> <christoph.langer at sap.com>
> *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: andrew_m_leonard at uk.ibm.com 
> <mailto:andrew_m_leonard at uk.ibm.com>
>
>
>
>
> From: "Daniel D. Daugherty" <daniel.daugherty at oracle.com 
> <mailto:daniel.daugherty at oracle.com>>
> To: Andrew Leonard <andrew_m_leonard at uk.ibm.com 
> <mailto:andrew_m_leonard at uk.ibm.com>>, 
> serviceability-dev at openjdk.java.net 
> <mailto: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/ 
> <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: andrew_m_leonard at uk.ibm.com 
> <mailto: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/20170714/14ff4662/attachment.html>