RFR : JDK-8044122 MBean access to the PID

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Alan Bateman
On 10/10/2017 12:47, Ujwal Vangapally wrote:
> Kindly review the changes made.
>
> https://bugs.openjdk.java.net/browse/JDK-8044122
>
> webrev :
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>
> CSR : https://bugs.openjdk.java.net/browse/JDK-8189091
The term "PID" is not defined in this context so I think this will need
improved javadoc to define the term. In Process API, the term used is
"process ID" and we should try to keep the API consistent if we can.

-Alan.
Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
Thanks for the review Alan,

will make that change in next webrev.

Ujwal.


On 10/10/2017 5:27 PM, Alan Bateman wrote:

> On 10/10/2017 12:47, Ujwal Vangapally wrote:
>> Kindly review the changes made.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>
>> webrev :
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>
>> CSR : https://bugs.openjdk.java.net/browse/JDK-8189091
> The term "PID" is not defined in this context so I think this will
> need improved javadoc to define the term. In Process API, the term
> used is "process ID" and we should try to keep the API consistent if
> we can.
>
> -Alan.

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Harsha Wardhana B
In reply to this post by Ujwal Vangapally
Hi Ujwal,

Could you please add a test-case to validate your changes? You can spawn
a new process and it can exchange its pid to its parent via
System.out/in or via sockets.

Also, VMManagementImpl:145, the change from getProcessId to getVmPid
seems unnecessary.

-Harsha


On Tuesday 10 October 2017 05:17 PM, Ujwal Vangapally wrote:
Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
Thanks for the review Harsha.

kindly see my comments inline.

On 10/10/2017 5:51 PM, Harsha Wardhana B wrote:
> Hi Ujwal,
>
> Could you please add a test-case to validate your changes? You can
> spawn a new process and it can exchange its pid to its parent via
> System.out/in or via sockets.
>
will it be sufficient if I verify that by comparing it with
ProcessHandle.current().pid()
> Also, VMManagementImpl:145, the change from getProcessId to getVmPid
> seems unnecessary.
  I will revert it back if not required.

>
> -Harsha
>
>
> On Tuesday 10 October 2017 05:17 PM, Ujwal Vangapally wrote:
>> Kindly review the changes made.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>
>> webrev :
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>
>> CSR : https://bugs.openjdk.java.net/browse/JDK-8189091
>>
>> Thanks,
>>
>> Ujwal.
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Mandy Chung
In reply to this post by Ujwal Vangapally


On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
> Kindly review the changes made.
>
> https://bugs.openjdk.java.net/browse/JDK-8044122
>
> webrev :
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>

RuntimeMXBean.java
    @since is missing

    Process::pid is long rather than int. The javadoc for this method
should be consistent with Process::pid, as Alan points out.

VMManagementImpl.java
     I think getProcessId should probably be replaced to implement with
ProcessHandle.current().pid();

Please include an unit test for it.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

roger riggs
Hi Ujwal,

In the implementation RuntimeMXBean.java: 72:  Include a message
"getProcessId" in the throw new Unsupported...
In the text and @return change "PID" to "process ID" as Alan suggested.
66: the @implSpec should be on its own line so the text starts on a new
line to make the source more readable.

Adding a test for getProcessId() should fit into one of the existing
tests that spawns and then checks
the attributes of a vm.  Perhaps MXBeanInteropTest1.java

Roger



On 10/10/2017 1:20 PM, mandy chung wrote:

>
>
> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>> Kindly review the changes made.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>
>> webrev :
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>
>
> RuntimeMXBean.java
>    @since is missing
>
>    Process::pid is long rather than int. The javadoc for this method
> should be consistent with Process::pid, as Alan points out.
>
> VMManagementImpl.java
>     I think getProcessId should probably be replaced to implement with
> ProcessHandle.current().pid();
>
> Please include an unit test for it.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
Thanks for the review and suggestions Mandy, Roger.

kindly see my comments inline.


On 10/10/2017 11:25 PM, Roger Riggs wrote:

> Hi Ujwal,
>
> In the implementation RuntimeMXBean.java: 72:  Include a message
> "getProcessId" in the throw new Unsupported...
> In the text and @return change "PID" to "process ID" as Alan suggested.
> 66: the @implSpec should be on its own line so the text starts on a
> new line to make the source more readable.
>
> Adding a test for getProcessId() should fit into one of the existing
> tests that spawns and then checks
> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
I will make changes as suggested.

>
> Roger
>
>
>
> On 10/10/2017 1:20 PM, mandy chung wrote:
>>
>>
>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>> Kindly review the changes made.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>
>>> webrev :
>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>
>>
>> RuntimeMXBean.java
>>    @since is missing
>>
I will add it.
>>    Process::pid is long rather than int. The javadoc for this method
>> should be consistent with Process::pid, as Alan points out.
will do it.
>>
>> VMManagementImpl.java
>>     I think getProcessId should probably be replaced to implement
>> with ProcessHandle.current().pid();
>>
you mean it would be better to use ProcessHandle.current().pid(); in
RuntimeImpl.java instead of jvm.getVmPid();

kindly clarify.
>> Please include an unit test for it.
>>
will it be sufficient to add it to existing test MXBeanInteropTest1.java

              System.out.println("getName\t\t"
                      + runtime.getName());
+            System.out.println("getPid\t\t"
+                    + runtime.getPid());
              System.out.println("getSpecName\t\t"
                      + runtime.getSpecName());

>> Mandy
>

Ujwal
Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
kindly see the new webrev incorporating review comments.

webrev:
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/

Thanks,

Ujwal.


On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:

> Thanks for the review and suggestions Mandy, Roger.
>
> kindly see my comments inline.
>
>
> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>> Hi Ujwal,
>>
>> In the implementation RuntimeMXBean.java: 72:  Include a message
>> "getProcessId" in the throw new Unsupported...
>> In the text and @return change "PID" to "process ID" as Alan suggested.
>> 66: the @implSpec should be on its own line so the text starts on a
>> new line to make the source more readable.
>>
>> Adding a test for getProcessId() should fit into one of the existing
>> tests that spawns and then checks
>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
> I will make changes as suggested.
>>
>> Roger
>>
>>
>>
>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>
>>>
>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>> Kindly review the changes made.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>
>>>> webrev :
>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>
>>>
>>> RuntimeMXBean.java
>>>    @since is missing
>>>
> I will add it.
>>>    Process::pid is long rather than int. The javadoc for this method
>>> should be consistent with Process::pid, as Alan points out.
> will do it.
>>>
>>> VMManagementImpl.java
>>>     I think getProcessId should probably be replaced to implement
>>> with ProcessHandle.current().pid();
>>>
> you mean it would be better to use ProcessHandle.current().pid(); in
> RuntimeImpl.java instead of jvm.getVmPid();
>
> kindly clarify.
>>> Please include an unit test for it.
>>>
> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>
>              System.out.println("getName\t\t"
>                      + runtime.getName());
> +            System.out.println("getPid\t\t"
> +                    + runtime.getPid());
>              System.out.println("getSpecName\t\t"
>                      + runtime.getSpecName());
>
>>> Mandy
>>
>
> Ujwal

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

roger riggs
Hi Ujwal,

Looks fine.

Please correct the copyright date in ProcessIdTest.  A new file usually
has only the current year.

Thanks, Roger


On 10/20/17 2:07 AM, Ujwal Vangapally wrote:

> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
Thanks for the review Roger, I will correct the copyright date.

Ujwal.


On 10/20/2017 7:37 PM, Roger Riggs wrote:

> Hi Ujwal,
>
> Looks fine.
>
> Please correct the copyright date in ProcessIdTest.  A new file
> usually has only the current year.
>
> Thanks, Roger
>
>
> On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
>> kindly see the new webrev incorporating review comments.
>>
>> webrev:
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>>
>> Thanks,
>>
>> Ujwal.
>>
>>
>> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>>> Thanks for the review and suggestions Mandy, Roger.
>>>
>>> kindly see my comments inline.
>>>
>>>
>>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>>> Hi Ujwal,
>>>>
>>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>>> "getProcessId" in the throw new Unsupported...
>>>> In the text and @return change "PID" to "process ID" as Alan
>>>> suggested.
>>>> 66: the @implSpec should be on its own line so the text starts on a
>>>> new line to make the source more readable.
>>>>
>>>> Adding a test for getProcessId() should fit into one of the
>>>> existing tests that spawns and then checks
>>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>>> I will make changes as suggested.
>>>>
>>>> Roger
>>>>
>>>>
>>>>
>>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>>
>>>>>
>>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>>> Kindly review the changes made.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>>
>>>>>> webrev :
>>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 
>>>>>>
>>>>>>
>>>>>
>>>>> RuntimeMXBean.java
>>>>>    @since is missing
>>>>>
>>> I will add it.
>>>>>    Process::pid is long rather than int. The javadoc for this
>>>>> method should be consistent with Process::pid, as Alan points out.
>>> will do it.
>>>>>
>>>>> VMManagementImpl.java
>>>>>     I think getProcessId should probably be replaced to implement
>>>>> with ProcessHandle.current().pid();
>>>>>
>>> you mean it would be better to use ProcessHandle.current().pid(); in
>>> RuntimeImpl.java instead of jvm.getVmPid();
>>>
>>> kindly clarify.
>>>>> Please include an unit test for it.
>>>>>
>>> will it be sufficient to add it to existing test
>>> MXBeanInteropTest1.java
>>>
>>>              System.out.println("getName\t\t"
>>>                      + runtime.getName());
>>> +            System.out.println("getPid\t\t"
>>> +                    + runtime.getPid());
>>>              System.out.println("getSpecName\t\t"
>>>                      + runtime.getSpecName());
>>>
>>>>> Mandy
>>>>
>>>
>>> Ujwal
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Mandy Chung
In reply to this post by Ujwal Vangapally
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:

> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>

Reply | Threaded
Open this post in threaded view
|

RE: RFR : JDK-8044122 MBean access to the PID

Langer, Christoph
In reply to this post by Ujwal Vangapally
Hi Ujwal,

just a minor thing: there's one space too much that sneaked in in line 72 "    public default  long getPid() {" of src/java.management/share/classes/java/lang/management/RuntimeMXBean.java.

Best regards
Christoph

> -----Original Message-----
> From: serviceability-dev [mailto:serviceability-dev-
> [hidden email]] On Behalf Of Ujwal Vangapally
> Sent: Freitag, 20. Oktober 2017 11:08
> To: [hidden email]; Mandy Chung
> <[hidden email]>; Roger Riggs <[hidden email]>
> Subject: Re: RFR : JDK-8044122 MBean access to the PID
>
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
> > Thanks for the review and suggestions Mandy, Roger.
> >
> > kindly see my comments inline.
> >
> >
> > On 10/10/2017 11:25 PM, Roger Riggs wrote:
> >> Hi Ujwal,
> >>
> >> In the implementation RuntimeMXBean.java: 72:  Include a message
> >> "getProcessId" in the throw new Unsupported...
> >> In the text and @return change "PID" to "process ID" as Alan suggested.
> >> 66: the @implSpec should be on its own line so the text starts on a
> >> new line to make the source more readable.
> >>
> >> Adding a test for getProcessId() should fit into one of the existing
> >> tests that spawns and then checks
> >> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
> > I will make changes as suggested.
> >>
> >> Roger
> >>
> >>
> >>
> >> On 10/10/2017 1:20 PM, mandy chung wrote:
> >>>
> >>>
> >>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
> >>>> Kindly review the changes made.
> >>>>
> >>>> https://bugs.openjdk.java.net/browse/JDK-8044122
> >>>>
> >>>> webrev :
> >>>>
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
> >>>>
> >>>
> >>> RuntimeMXBean.java
> >>>    @since is missing
> >>>
> > I will add it.
> >>>    Process::pid is long rather than int. The javadoc for this method
> >>> should be consistent with Process::pid, as Alan points out.
> > will do it.
> >>>
> >>> VMManagementImpl.java
> >>>     I think getProcessId should probably be replaced to implement
> >>> with ProcessHandle.current().pid();
> >>>
> > you mean it would be better to use ProcessHandle.current().pid(); in
> > RuntimeImpl.java instead of jvm.getVmPid();
> >
> > kindly clarify.
> >>> Please include an unit test for it.
> >>>
> > will it be sufficient to add it to existing test MXBeanInteropTest1.java
> >
> >              System.out.println("getName\t\t"
> >                      + runtime.getName());
> > +            System.out.println("getPid\t\t"
> > +                    + runtime.getPid());
> >              System.out.println("getSpecName\t\t"
> >                      + runtime.getSpecName());
> >
> >>> Mandy
> >>
> >
> > Ujwal

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Bernd Eckenfels-4
In reply to this post by Mandy Chung
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)

Gruss
Bernd
--
http://bernd.eckenfels.net

From: serviceability-dev <[hidden email]> on behalf of mandy chung <[hidden email]>
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Mandy Chung
The permission check should be done separately, if needed.  Otherwise, the frames in the stack when this method is called must have the security permission granted.

Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally

Thanks for the review Mandy, Roger, Harsha, Christoph.

kindly see the new webrev incorporating review comments.

webrev: http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.02/

csr : https://bugs.openjdk.java.net/browse/JDK-8189091

Ujwal.


On 10/24/2017 10:50 PM, mandy chung wrote:
The permission check should be done separately, if needed.  Otherwise, the frames in the stack when this method is called must have the security permission granted.

Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>



Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Bernd Eckenfels-4
In reply to this post by Mandy Chung
Hello,

Yes the frames must have the security granted, isnt that the whole purpose of a security permission check?

With the current state of the patch unprivileged application code can read the PID. If this is intentional I would simply remove the access check completely instead.


From: serviceability-dev <[hidden email]> on behalf of mandy chung <[hidden email]>
Sent: Tuesday, October 24, 2017 7:20:27 PM
To: [hidden email]
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
The permission check should be done separately, if needed.  Otherwise, the frames in the stack when this method is called must have the security permission granted.

Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

roger riggs
In reply to this post by Ujwal Vangapally
Hi Ujwal,

In RuntimeMXBean:

Please add @throws UnsupportedOperationException if the process id is not available

Otherwise, looks fine.

Thanks, Roger


On 10/26/2017 4:29 AM, Ujwal Vangapally wrote:

Thanks for the review Mandy, Roger, Harsha, Christoph.

kindly see the new webrev incorporating review comments.

webrev: http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.02/

csr : https://bugs.openjdk.java.net/browse/JDK-8189091

Ujwal.


On 10/24/2017 10:50 PM, mandy chung wrote:
The permission check should be done separately, if needed.  Otherwise, the frames in the stack when this method is called must have the security permission granted.

Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>




Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Mandy Chung
In reply to this post by Bernd Eckenfels-4


On 10/26/17 1:53 AM, Bernd Eckenfels wrote:
Hello,

Yes the frames must have the security granted, isnt that the whole purpose of a security permission check?

With the current state of the patch unprivileged application code can read the PID. If this is intentional I would simply remove the access check completely instead.


Process::pid does not do any security check (see javadoc).  It's ProcessHandle::current that requires RuntimePermission("manageProcess") and getting it in a privileged context is correct and it's implementation detail.   We must ensure not leaking ProcessHandle.

Mandy


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Tuesday, October 24, 2017 7:20:27 PM
To: [hidden email]
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
The permission check should be done separately, if needed.  Otherwise, the frames in the stack when this method is called must have the security permission granted.

Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>



Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8044122 MBean access to the PID

Ujwal Vangapally
In reply to this post by roger riggs

Hi Roger,

made changes as suggested.

webrev : http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.03/

Thanks,
Ujwal.

On 10/26/2017 6:54 PM, Roger Riggs wrote:
Hi Ujwal,

In RuntimeMXBean:

Please add @throws UnsupportedOperationException if the process id is not available

Otherwise, looks fine.

Thanks, Roger


On 10/26/2017 4:29 AM, Ujwal Vangapally wrote:

Thanks for the review Mandy, Roger, Harsha, Christoph.

kindly see the new webrev incorporating review comments.

webrev: http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.02/

csr : https://bugs.openjdk.java.net/browse/JDK-8189091

Ujwal.


On 10/24/2017 10:50 PM, mandy chung wrote:
The permission check should be done separately, if needed.  Otherwise, the frames in the stack when this method is called must have the security permission granted.

Mandy

On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
Hello,

When running this privileged it means one can bypass the permission by using the MBean, is that intentional? (Besides it is already available as the JMVID)


From: serviceability-dev [hidden email] on behalf of mandy chung [hidden email]
Sent: Friday, October 20, 2017 4:42:00 PM
To: Ujwal Vangapally; Roger Riggs
Cc: serviceability-dev
Subject: Re: RFR : JDK-8044122 MBean access to the PID
 
Process::pid may throw SecurityException.  You have to wrap the call
with doPrivileged.   Process::pid can throw UOE on platform that doesn't
support this operation.  RuntimeMXBean::getPid should specify when the
platform does not support this operation.

ProcessIdTest - can you also check if getName contains the pid matching
the value of getPid()?

Mandy

On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
> kindly see the new webrev incorporating review comments.
>
> webrev:
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/
>
> Thanks,
>
> Ujwal.
>
>
> On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> Thanks for the review and suggestions Mandy, Roger.
>>
>> kindly see my comments inline.
>>
>>
>> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>>> Hi Ujwal,
>>>
>>> In the implementation RuntimeMXBean.java: 72:  Include a message
>>> "getProcessId" in the throw new Unsupported...
>>> In the text and @return change "PID" to "process ID" as Alan suggested.
>>> 66: the @implSpec should be on its own line so the text starts on a
>>> new line to make the source more readable.
>>>
>>> Adding a test for getProcessId() should fit into one of the existing
>>> tests that spawns and then checks
>>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> I will make changes as suggested.
>>>
>>> Roger
>>>
>>>
>>>
>>> On 10/10/2017 1:20 PM, mandy chung wrote:
>>>>
>>>>
>>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>>>>> Kindly review the changes made.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/
>>>>>
>>>>>
>>>>
>>>> RuntimeMXBean.java
>>>>    @since is missing
>>>>
>> I will add it.
>>>>    Process::pid is long rather than int. The javadoc for this
>>>> method should be consistent with Process::pid, as Alan points out.
>> will do it.
>>>>
>>>> VMManagementImpl.java
>>>>     I think getProcessId should probably be replaced to implement
>>>> with ProcessHandle.current().pid();
>>>>
>> you mean it would be better to use ProcessHandle.current().pid(); in
>> RuntimeImpl.java instead of jvm.getVmPid();
>>
>> kindly clarify.
>>>> Please include an unit test for it.
>>>>
>> will it be sufficient to add it to existing test MXBeanInteropTest1.java
>>
>>              System.out.println("getName\t\t"
>>                      + runtime.getName());
>> +            System.out.println("getPid\t\t"
>> +                    + runtime.getPid());
>>              System.out.println("getSpecName\t\t"
>>                      + runtime.getSpecName());
>>
>>>> Mandy
>>>
>>
>> Ujwal
>





12