Thanks David. I have created change-set against hs-rt which is identical to jdk9/dev. Copyright year is already updated in hs-rt file. Just for reference I have uploaded the latest change-set in the below link. http://cr.openjdk.java.net/~csahu/8151509/webrev.02/
Thanks , Cheleswer -----Original Message----- From: David Holmes Sent: Monday, March 14, 2016 7:19 AM To: Cheleswer Sahu; Dmitry Dmitriev; Thomas Stüfe Cc: [email protected]; [email protected] Subject: Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly On 11/03/2016 9:38 PM, Cheleswer Sahu wrote: > Thanks Dmitry and Thomas for reviews. After adding space I will request for > the push. Consider this Reviewed. But this needs to pushed to hs-rt and currently the webrev is against jdk9/dev. Thanks, David > > > 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.open > jdk.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]"serviceability-dev@openjdk > .java.net; HYPERLINK > "mailto:[email protected]"hotspot-runtime-dev@openj > dk.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:[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.n > et/~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 > > > > > > > > >
