[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR 8183123 : JDP packets have no processId context set
From: "Langer, Christoph" <christoph.langer () sap ! com>
Date: 2017-07-17 15:18:43
Message-ID: 86129da6205e41faa3b448f45da6724b () sap ! com
[Download RAW message or body]
Thanks, Dan for the review.
I pushed it [1] and added a 9-bp label to the bug. I'll try to downport thi=
s once jdk9u-dev is open.
Best regards
Christoph
[1] http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/04ad8f0efc06
From: Daniel D. Daugherty [mailto:daniel.daugherty@oracle.com]
Sent: Samstag, 15. Juli 2017 00:27
To: Langer, Christoph <christoph.langer@sap.com>
Cc: serviceability-dev@openjdk.java.net; Andrew Leonard <andrew_m_leonard@u=
k.ibm.com>
Subject: Re: RFR 8183123 : JDP packets have no processId context set
On 7/13/17 5:34 AM, Langer, Christoph wrote:
Hi Daniel,
here is the updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/818=
3123.1/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.1/>
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.jav=
a
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 fr=
om you?
Thanks
Christoph
From: Andrew Leonard [mailto:andrew_m_leonard@uk.ibm.com]
Sent: Freitag, 7. Juli 2017 15:20
To: daniel.daugherty@oracle.com<mailto:daniel.daugherty@oracle.com>
Cc: serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.j=
ava.net>; Langer, Christoph <christoph.langer@sap.com><mailto:christoph.lan=
ger@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 inten=
ded to just change that method implementation to use the ProcessHandle clas=
s, however the pid() method returns a "long" and the containing method retu=
rns 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 c=
hange the private method to return a Long object, and change the calling in=
stance appropriately. I also see if I look at the javadoc for ProcessHandle=
.pid() that it can in "theory" return UnsupportOperationException, so I hav=
e also handled that. I have a new webrev, which I will ask Christoph to upl=
oad...
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leonard@uk.ibm.com<mailto:andrew_m_leonard@uk.ibm.=
com>
From: "Daniel D. Daugherty" <daniel.daugherty@oracle.com<mailto:dani=
el.daugherty@oracle.com>>
To: Andrew Leonard <andrew_m_leonard@uk.ibm.com<mailto:andrew_m_leon=
ard@uk.ibm.com>>, serviceability-dev@openjdk.java.net<mailto:serviceability=
-dev@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/~clange=
r/webrevs/8183123.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.=
0/>
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.jav=
a
L137 return (int)ProcessHandle.current().pid();
The return type is Integer. Why not cast to "Integer" instead of "i=
nt"?
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 platfor=
m
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.curre=
nt().getPid().
- Update testcase to cover the failing scenario. Thus it fails without my p=
atch, 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@uk.ibm.com<mailto:andrew_m_leonard@uk.ibm.=
com>
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 74=
1598.
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 74=
1598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
[Attachment #3 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"> <head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<!--[if !mso]><style>v\:* {behavior:url(#default#VML);}
o\:* {behavior:url(#default#VML);}
w\:* {behavior:url(#default#VML);}
.shape {behavior:url(#default#VML);}
</style><![endif]--><style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
tt
{mso-style-priority:99;
font-family:"Courier New",serif;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
span.EmailStyle19
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body bgcolor="white" lang="DE" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US">Thanks, \
Dan for the review.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US">I \
pushed it [1] and added a 9-bp label to the bug. I’ll try to downport this once \
jdk9u-dev is open.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US">Best \
regards<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US">Christoph<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US">[1]
<a href="http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/04ad8f0efc06">http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/04ad8f0efc06</a><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span lang="EN-US" \
style="mso-fareast-language:EN-US"><o:p> </o:p></span></a></p> <span \
style="mso-bookmark:_MailEndCompose"></span> <div \
style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"> <div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span \
lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> \
Daniel D. Daugherty [mailto:daniel.daugherty@oracle.com] <br>
<b>Sent:</b> Samstag, 15. Juli 2017 00:27<br>
<b>To:</b> Langer, Christoph <christoph.langer@sap.com><br>
<b>Cc:</b> serviceability-dev@openjdk.java.net; Andrew Leonard \
<andrew_m_leonard@uk.ibm.com><br> <b>Subject:</b> Re: RFR 8183123 : JDP packets \
have no processId context set<o:p></o:p></span></p> </div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">On 7/13/17 5:34 AM, Langer, Christoph wrote:<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">Hi \
Daniel,</span><o:p></o:p></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">here \
is the updated webrev: <a \
href="http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.1/">http://cr.openjdk.java.net/~clanger/webrevs/8183123.1/</a></span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier \
New",serif"><br> \
<tt>src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java</tt><br>
<tt> No comments.</tt><br>
<br>
<tt>test/sun/management/jdp/JdpOnTestCase.java</tt><br>
<tt> No comments.</tt><br>
<br>
<tt>Thumbs up.</tt><br>
<br>
<tt>Dan</tt><br>
<br>
<br>
<br>
</span><o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">To \
me it looks ok, I’ll update copyright header when I submit. Thumbs up from \
you?</span><o:p></o:p></p> <p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">Thanks</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">Christoph</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" \
style="mso-fareast-language:EN-US"> </span><o:p></o:p></p> <div \
style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"> <div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" \
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span \
lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif"> \
Andrew Leonard [<a href="mailto:andrew_m_leonard@uk.ibm.com">mailto:andrew_m_leonard@uk.ibm.com</a>]
<br>
<b>Sent:</b> Freitag, 7. Juli 2017 15:20<br>
<b>To:</b> <a href="mailto:daniel.daugherty@oracle.com">daniel.daugherty@oracle.com</a><br>
<b>Cc:</b> <a href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>; \
Langer, Christoph <a \
href="mailto:christoph.langer@sap.com"><christoph.langer@sap.com></a><br> \
<b>Subject:</b> Re: RFR 8183123 : JDP packets have no processId context \
set</span><o:p></o:p></p> </div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span \
style="font-size:10.0pt;font-family:"Arial",sans-serif">Hi Daniel,</span> \
<br> <span style="font-size:10.0pt;font-family:"Arial",sans-serif">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...</span> <br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">Thanks</span> \
<br> <span style="font-size:10.0pt;font-family:"Arial",sans-serif">Andrew</span> \
<br> <br>
Andrew Leonard<br>
Java Runtimes Development<br>
IBM Hursley<br>
IBM United Kingdom Ltd<br>
Phone internal: 245913, external: 01962 815913<br>
internet email: <a href="mailto:andrew_m_leonard@uk.ibm.com">andrew_m_leonard@uk.ibm.com</a>
<br>
<br>
<br>
<br>
<br>
<span style="font-size:7.5pt;font-family:"Arial",sans-serif;color:#5F5F5F">From: \
</span><span \
style="font-size:7.5pt;font-family:"Arial",sans-serif">"Daniel D. \
Daugherty" <<a \
href="mailto:daniel.daugherty@oracle.com">daniel.daugherty@oracle.com</a>></span> \
<br> <span style="font-size:7.5pt;font-family:"Arial",sans-serif;color:#5F5F5F">To: \
</span><span \
style="font-size:7.5pt;font-family:"Arial",sans-serif">Andrew Leonard \
<<a href="mailto:andrew_m_leonard@uk.ibm.com">andrew_m_leonard@uk.ibm.com</a>>, \
<a href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a></span>
<br>
<span style="font-size:7.5pt;font-family:"Arial",sans-serif;color:#5F5F5F">Date: \
</span><span \
style="font-size:7.5pt;font-family:"Arial",sans-serif">05/07/2017 \
21:27</span> <br>
<span style="font-size:7.5pt;font-family:"Arial",sans-serif;color:#5F5F5F">Subject: \
</span><span \
style="font-size:7.5pt;font-family:"Arial",sans-serif">Re: RFR 8183123 : \
JDP packets have no processId context set</span> <o:p></o:p></p>
<div class="MsoNormal" align="center" style="text-align:center">
<hr size="2" width="100%" noshade="" style="color:#A0A0A0" align="center">
</div>
<p class="MsoNormal"><br>
<br>
<br>
On 6/29/17 7:57 AM, Andrew Leonard wrote: <br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">Hi \
All,</span> <span style="font-size:10.0pt;font-family:"Arial",sans-serif"> \
<br> Please can I get some review feedback for my changes for this issue: </span><a \
href="https://bugs.openjdk.java.net/browse/JDK-8183123"><span \
style="font-size:10.0pt;font-family:"Calibri",sans-serif">https://bugs.openjdk.java.net/browse/JDK-8183123</span></a>
<span style="font-size:10.0pt;font-family:"Calibri",sans-serif"><br>
The webrev patch has been uploaded here: </span><a \
href="http://cr.openjdk.java.net/%7Eclanger/webrevs/8183123.0/"><span \
style="font-size:10.0pt;font-family:"Calibri",sans-serif">http://cr.openjdk.java.net/~clanger/webrevs/8183123.0/</span></a>
<br>
<tt><span style="font-size:10.0pt;color:blue"> </span></tt><span \
style="font-size:10.0pt;font-family:"Courier New",serif"><br> \
<tt>src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java</tt><br>
<tt> L137 return \
(int)ProcessHandle.current().pid();</tt><br> \
<tt> The return type is Integer. Why not \
cast to "Integer" instead of "int"?</tt><br> <br>
<tt>test/sun/management/jdp/JdpOnTestCase.java</tt><br>
<tt> The test update just verifies that a non-NULL PROCESS_ID is \
found, but</tt><br> <tt> doesn't verify the format (integer) of the \
return. Of course, a platform</tt><br> <tt> independent format for \
PROCESS_ID might be problematic... For example,</tt><br> <tt> in \
some versions of Cygwin, I've seen negative values for PIDs...</tt><br> <br>
<tt>Thumbs up. If you change the cast I don't need to see a new webrev.</tt><br>
<tt> </tt><br>
<tt> </tt><br>
<tt>Dan</tt></span><u><span style="font-size:10.0pt;font-family:"Courier \
New",serif;color:blue"><br> </span></u><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif"><br>
Essentially the fix entails:</span> <span \
style="font-size:10.0pt;font-family:"Arial",sans-serif"> <br>
- Replacing invalid process id query logic with call to </span><tt><span \
style="font-size:10.0pt">ProcessHandle.current().getPid().</span></tt> <span \
style="font-size:10.0pt;font-family:"Arial",sans-serif"><br>
- Update testcase to cover the failing scenario. Thus it fails without my patch, and \
succeeds with it.</span> <br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif"><br>
Thanks</span> <span style="font-size:10.0pt;font-family:"Arial",sans-serif"><br>
Andrew</span> <br>
<br>
Andrew Leonard<br>
Java Runtimes Development<br>
IBM Hursley<br>
IBM United Kingdom Ltd<br>
Phone internal: 245913, external: 01962 815913<br>
internet email: <a href="mailto:andrew_m_leonard@uk.ibm.com">andrew_m_leonard@uk.ibm.com</a>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif"><br>
<br>
Unless stated otherwise above:<br>
IBM United Kingdom Limited - Registered in England and Wales with number 741598. <br>
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU</span>
<br>
<br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif"><br>
Unless stated otherwise above:<br>
IBM United Kingdom Limited - Registered in England and Wales with number 741598. <br>
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 \
3AU</span><o:p></o:p></p> </div>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic