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: [email protected]; [email protected] 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 HYPERLINK "http://cr.openjdk.java.net/%7Ecsahu/8151509/webrev.01/"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; HYPERLINK "mailto:[email protected]"[email protected]; HYPERLINK "mailto:[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: HYPERLINK "http://cr.openjdk.java.net/%7Ecsahu/8151509/"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
