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

[Thomas Stüfe]

Hi Cheleswer,

this version looks fine too me. Thank you for doing this.

Kind Regards, Thomas

On Fri, Mar 11, 2016 at 10:31 AM, Cheleswer Sahu <cheleswer.sahu at oracle.com>
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/
>
>
>
>
>
> 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 <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 <
> 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/673bf24c/attachment.html>