Re: RFR 9: 8077350 Process API Updates Implementation Review (Due 4/23)

[Roger Riggs]

Hi Thomas,

Good point and more robust.

Thanks, Roger


On 4/21/2015 11:49 AM, Thomas Stüfe wrote:

Hi Roger,

small remark, I see you at a number of places using the same pattern when reading information from /proc:

 342     ret = fread(&psinfo, 1, (sizeof psinfo), fp);
 343     fclose(fp);
 344     if (ret < 0) {
 345         return;
 346     }

A better way would be to check for the whole size you intended to read:

 342     ret = fread(&psinfo, 1, (sizeof psinfo), fp);
 343     fclose(fp);
 344     if (ret < sizeof(psinfo)) {
 345         return;
 346     }

/proc may get stale while you read it, so make sure fread actually filled the whole output structure, otherwise you may act on partially random memory content. This is not theoretical :) I had exactly this problem on Solaris when reading structures (in our cases /proc/self/lstatus) from /proc.

Kind Regards, Thomas

On Fri, Apr 17, 2015 at 9:12 PM, Roger Riggs <roger.ri...@oracle.com <mailto:roger.ri...@oracle.com>> wrote:

    The webrev for ProcessAPI updates has been updated to reflect
    recent comments.
    Please  review and comment by April 23rd.

    The updates include:
     - Renaming Process/ProcessHandle supportsDestroyForcibly to
    supportsNormalTermination
       and updating related descriptions
    - ProcessHandle.destroy/destroyForcible descriptions have more
    consistent descriptions
    - ProcessHandle.destroy now returns ProcessHandle to enable fluent
    use.
    - Corrected description of default implementation ProcessHandle.onExit

    The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/
    <http://cr.openjdk.java.net/%7Erriggs/ph-apidraft/>

    The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph
    <http://cr.openjdk.java.net/%7Erriggs/webrev-ph>

    Issue: JDK-8077350
    <https://bugs.openjdk.java.net/browse/JDK-8077350> Process API
    Updates Implementation

    The code is in the jdk9 sandbox on branch JDK-8046092-branch.

    Two issues raised have been filed to be fixed after the current
    commit:
     - JDK-8078099 <https://bugs.openjdk.java.net/browse/JDK-8078099>
    (process) ProcessHandle should uniquely identify processes
     - JDK-8078108 <https://bugs.openjdk.java.net/browse/JDK-8078108>
    (process) ProcessHandle.isAlive should be robust