Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
On 14/03/2016 7:15 PM, Cheleswer Sahu wrote:
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/
Looks good.
Thanks,
David
Thanks ,
Cheleswer
-Original Message-
From: David Holmes
Sent: Monday, March 14, 2016 7:19 AM
To: Cheleswer Sahu; Dmitry Dmitriev; 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
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: 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
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:thomas.stu...@gmail.com]
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; HYPERLINK
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk
.java.net; HYPERLINK
"mailto:hotspot-runtime-...@openjdk.java.net"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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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: 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
RE: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
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: 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
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: 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
>
> 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:thomas.stu...@gmail.com]
> Sent: Thursday, March 10, 2016 7:39 PM
> To: Dmitry Dmitriev
> Cc: Cheleswer Sahu; HYPERLINK
> "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk
> .java.net; HYPERLINK
> "mailto:hotspot-runtime-...@openjdk.java.net"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 "mailto:thomas.stu...@gmail.com"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 "mailto:dmitry.dmitr...@oracle.com"dmitry.dmit
Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
PS. Copyright year needs updating.
David
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.
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
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:thomas.stu...@gmail.com]
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; HYPERLINK
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
HYPERLINK
"mailto:hotspot-runtime-...@openjdk.java.net"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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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: 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
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: 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
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:thomas.stu...@gmail.com]
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; HYPERLINK
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
HYPERLINK
"mailto:hotspot-runtime-...@openjdk.java.net"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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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: 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
Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
Dmitry is right, at least I am only a committer, not a "R"eviewer.
On Fri, Mar 11, 2016 at 12:40 PM, Dmitry Dmitriev <
dmitry.dmitr...@oracle.com> wrote:
> 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/
>
>
>
>
>
> Regards,
>
> Cheleswer
>
>
>
> *From:* Thomas Stüfe [mailto:thomas.stu...@gmail.com
> <thomas.stu...@gmail.com>]
> *Sent:* Thursday, March 10, 2016 7:39 PM
> *To:* Dmitry Dmitriev
> *Cc:* Cheleswer Sahu; 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
>
>
>
> (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>
> 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>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>
> 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
>
>
>
>
>
>
>
>
>
>
>
Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
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
RE: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
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
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:thomas.stu...@gmail.com]
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; HYPERLINK
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net;
HYPERLINK
"mailto:hotspot-runtime-...@openjdk.java.net"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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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: 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
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;
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
RE: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
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.stu...@gmail.com]
Sent: Thursday, March 10, 2016 7:39 PM
To: Dmitry Dmitriev
Cc: Cheleswer Sahu; 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
(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 mailto:thomas.stu...@gmail.com"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 mailto:dmitry.dmitr...@oracle.com"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/>
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
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
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> 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
>>>
>>>
>>
>
Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
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 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
>>
>>
>
Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
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/
RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly
Hi, Please review the code changes for https://bugs.openjdk.java.net/browse/JDK-8151509. Webrev link: http://cr.openjdk.java.net/~csahu/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
