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

[Dmitry Dmitriev]

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 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
>
> 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.stuefe at gmail.com]
>     *Sent:* Thursday, March 10, 2016 7:39 PM
>     *To:* Dmitry Dmitriev
>     *Cc:* Cheleswer Sahu; serviceability-dev at openjdk.java.net
>     <mailto:serviceability-dev at openjdk.java.net>;
>     hotspot-runtime-dev at openjdk.java.net
>     <mailto: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 <mailto: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
>         <mailto: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/>
>                 <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/2929b571/attachment.html>