RFR 8183123 : JDP packets have no processId context set

[Langer, Christoph]

Hi Daniel,

here is the updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183123.1/

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/20170713/3029815a/attachment.html>