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:[email protected]]
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; [email protected];
[email protected]
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:[email protected]"[email protected]> 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:[email protected]"[email protected]> 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
