Cheleswer, thank you! But I think you need a "R"eviewer for that change
before push.
Dmitry
On 11.03.2016 14:38, Cheleswer Sahu wrote:
Thanks Dmitry and Thomas for reviews. After adding space I will
request for the push.
Regards,
Cheleswer
*From:*Dmitry Dmitriev
*Sent:* Friday, March 11, 2016 5:00 PM
*To:* Cheleswer Sahu; Thomas Stüfe
*Cc:* serviceability-dev@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
*Subject:* Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer
is not updated correctly
Hi Cheleswer,
Please, add space between SIZE_FORMAT and " because C++11 requires a
space between literal and identifier. Not need a new webrev for that.
Thanks,
Dmitry
On 11.03.2016 12:31, Cheleswer Sahu wrote:
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/
<http://cr.openjdk.java.net/%7Ecsahu/8151509/webrev.01/>
Regards,
Cheleswer
*From:*Thomas Stüfe [mailto:thomas.stu...@gmail.com]
*Sent:* Thursday, March 10, 2016 7:39 PM
*To:* Dmitry Dmitriev
*Cc:* Cheleswer Sahu; serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>;
hotspot-runtime-...@openjdk.java.net
<mailto:hotspot-runtime-...@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
<thomas.stu...@gmail.com <mailto:thomas.stu...@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
<dmitry.dmitr...@oracle.com
<mailto:dmitry.dmitr...@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/>
<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
