RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

[Cheleswer Sahu]

Hi Thomas, Dmitry,

 

Thanks for your review comments.  My answers are below for your review comments 

 

1873       if( 0 != ret % sizeof(prmap_t)){
1874         break;
1875       }



>> For this it has been thought that mostly read() will return the desired number of bytes, but only in case if something goes wrong and read() will  not able to read the data, it will return lesser number of bytes, which contains the partial data of  “prmap_t” structure. The reason could be like file is corrupted, in such cases we don’t want to read anymore and feel it’s safe to skip the rest of file. 

 

2) Just interesting, do you really need to set memory to 0 by memset?

>>  I thought this it is good to have a clean buffer every time we read something into it, but it’s really not that much required as we are reading a binary data. So I am removing this line from the code.

 

For rest of the comments I have made correction in the code. The new webrev is available in the below location

http://cr.openjdk.java.net/~csahu/8151509/webrev.01/

 

 

Regards,

Cheleswer

 

From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] 
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

 

(Sorry, pressed Send button too early)

Just wanted to add that 

1873       if( 0 != ret % sizeof(prmap_t)){
1874         break;
1875       }

may be a bit harsh, as it skips the entire mapping in case read() stopped reading in a middle of a record. You could just continue to read until you read the rest of the record.

Kind Regards, Thomas

 

On Thu, Mar 10, 2016 at 3:07 PM, Thomas Stüfe <HYPERLINK "mailto:thomas.stuefe at gmail.com"thomas.stuefe at gmail.com> wrote:

Hi Cheleswer,

 

thanks for fixing this.

 

Some more issues:

 

- 1866 char *mbuff = (char *) calloc(read_chunk, sizeof(prmap_t) + 1);

 

Why the "+1"? It is unnecessary and causes the allocation to be 200 bytes larger than necessary.

 

- 1880 st->print("Warning: Address: " PTR_FORMAT ", Size: %dK, ",p->pr_vaddr, p->pr_size/1024);

 

Format specifier for Size is wrong.%d is int, but p->pr_size is size_t. Theoretical truncation for mappings larger than 4g*1024.

(But I know this coding was there before)

 

Beside those points, I think both points of Dmitry are valid.

 

Also, I find 

 

 

Kind Regards, Thomas

 

On Thu, Mar 10, 2016 at 1:45 PM, Dmitry Dmitriev <HYPERLINK "mailto:dmitry.dmitriev at oracle.com"dmitry.dmitriev at oracle.com> wrote:

Hi Cheleswer,

Looks good, but I have questions/comments about other code in this function:
1) I think that "::close(fd);" should be inside "if (fd >= 0) {".
2) Just interesting, do you really need to set memory to 0 by memset?

Thanks,
Dmitry


On 10.03.2016 13:43, Cheleswer Sahu wrote:


Hi,

Please review the code changes for https://bugs.openjdk.java.net/browse/JDK-8151509.

Webrev link: http://cr.openjdk.java.net/~csahu/8151509/ <http://cr.openjdk.java.net/%7Ecsahu/8151509/>

Bug Brief:

In check_addr0(),  pointer ”p” is not updated correctly, because of this it was reading only first two entries from buffer.

Regards,

Cheleswer

 

 

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160311/181550cf/attachment.html>