Re: RFR: 8146879: Add option for handling existing log files in UL
hi Marcus,
I’m still going through the webrev, but I thought I would report my feedback so
far.
#1 File src/os/posix/vm/os_posix.cpp
How about instead of:
+int os::compare_file_modified_times(const char* file1, const char* file2) {
+ struct stat st[2];
+ struct timespec filetime[2];
+
+ for (int i = 0; i < 2; i++) {
+const char* file = (i == 0 ? file1 : file2);
+int ret = os::stat(file, [i]);
+assert(ret == 0, "failed to stat() file '%s': %s", file, strerror(errno));
+#ifdef __APPLE__
+filetime[i] = st[i].st_mtimespec;
+#else
+filetime[i] = st[i].st_mtim;
+#endif
+ }
+
+ int diff = filetime[0].tv_sec - filetime[1].tv_sec;
+ if (diff == 0) {
+return filetime[0].tv_nsec - filetime[1].tv_nsec;
+ }
+ return diff;
+}
we have something like this, which doesn’t use arrays or a loop mimicking
inline function:
static inline struct timespec get_timespec(const char* file) {
struct stat st;
int ret = stat(file, );
assert(ret == 0, "failed to stat() file '%s': %s", file, strerror(errno));
#ifdef __APPLE__
return st.st_mtimespec;
#else
return st.st_mtim;
#endif
}
int compare_file_modified_times(const char* file1, const char* file2) {
struct timespec filetime1 = get_timespec(file1);
struct timespec filetime2 = get_timespec(file2);
int diff = filetime1.tv_sec - filetime2.tv_sec;
if (diff == 0) {
return filetime1.tv_nsec - filetime2.tv_nsec;
}
return diff;
}
Similarly we should use static inline function instead of a loop in
src/os/windows/vm/os_windows.cpp
---
#2 File src/share/vm/logging/log.cpp
Doesn’t this function produce an error or at least a warning on a product build
without asserts?
+static void delete_file(const char* filename) {
+ if (!file_exists(filename)) {
+return;
+ }
+ int ret = remove(filename);
+ assert(ret == 0, "failed to remove file '%s': %s", filename,
strerror(errno));
+}
---
#3 File src/share/vm/logging/log.cpp
The number_of_lines_with_substring_in_file() function will not count the
substrings that happen to lay across the boundary at sizeof(buf). For example
with:
char buf[16];
and file consisting of “12345678901234gerard1234567890” it will return 0 for
number_of_lines_with_substring_in_file(file, “gerard")
---
#4 File src/share/vm/logging/log.cpp
Should we clarify that we are rounding the results of log10 down by explicitly
using floor and explicitly casting it to uint?
static uint number_of_digits_new(uint number) {
double d = static_cast(number);
uint res = 1 + static_cast(floor(log10(d)));
return res;
}
cheers
> On Mar 11, 2016, at 8:21 AM, Marcus Larsson wrote:
>
> Third time's the charm.
>
> Webrev:
> http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/
>
> This patch makes log file rotation the default. Default thresholds are 5
> rotated files with a target size of 20MiB. Truncating behavior can be
> achieved by setting filecount to 0 (-Xlog::myfile.log::filecount=0).
>
> If a log file already exists during log file initialization it will be
> rotated. If any of the target file names (file.0 to file.4 in the default
> case) are available, that filename will be used for the existing log. If all
> names are taken the VM will attempt to overwrite the oldest file.
>
> This should prevent unlimited log file creations and avoid accidental loss of
> log files from previous runs. The default thresholds (5 files, 20MiB each) is
> just a suggestion. If you think it should be higher/lower let me know.
>
> Tested with included internal VM tests through RBT.
>
> Thanks,
> Marcus
>
> On 2016-03-01 15:05, Marcus Larsson wrote:
>> Hi,
>>
>> After some offline discussions I'm withdrawing this patch. I will instead
>> investigate if I can achieve similar behavior using log rotation as the
>> default.
>>
>> Thanks,
>> Marcus
>>
>> On 03/01/2016 12:11 PM, Marcus Larsson wrote:
>>> Hi again,
>>>
>>> Taking a different approach to this.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/
>>>
>>> Existing files will now by default be renamed/archived with a .X suffix
>>> where X is the lowest number such that the resulting file name is available
>>> (jvm.log becomes jvm.log.0). A mode option for controlling this behavior
>>> has been added as well. It can be set to archive, append, or truncate (i.e.
>>> -Xlog::jvm.log::mode=truncate).
>>>
>>> Tested with included jtreg test through JPRT.
>>>
>>> Thanks,
>>> Marcus
>>>
>>> On 01/14/2016 04:00 PM, Marcus Larsson wrote:
Hi,
Please review the following patch to make sure UL truncates existing log
files before writing to them. Since files are opened in append mode,
truncation isn't done automatically, so instead the patch adds an attempt
to remove the
RFR: JDK-8151653: Hotspot build does not respect --enable-openjdk-only
Hello, When building hotspot with closed sources present and configuring with --enable-openjdk-only, various closed parts are included in the build anyway, at least on Windows. This needs to be fixed in preparation for the new hotspot build for build output comparisons to be meaningful between the old and new. The major culprit here, which applies to all platforms, is the trace source generation. The base trace.xml uses XInclude to explicitly and unconditionally include closed xml files if present. I'm fixing this by introducing a closed version of trace.xml which includes the open and closed parts, while the open trace.xml only includes the open parts. The rest of the changes are just for Windows, making sure the OPENJDK variable is propagated into the nmake build and making all conditionals on including closed source also check that variable. Bug: https://bugs.openjdk.java.net/browse/JDK-8151653 Webrev: http://cr.openjdk.java.net/~erikj/8151653/webrev.hotspot.01/index.html /Erik
Re: RFR: 8146879: Add option for handling existing log files in UL
Hi Marcus, Two small things. src/share/vm/runtime/os.hpp: + static int compare_file_modified_times(const char* file1, const char* file2); Is this so generic that we should have it os.[h,c]pp? E.g. if we want to compare ctime, etc.. ? On 03/11/2016 03:21 PM, Marcus Larsson wrote: Third time's the charm. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/ This patch makes log file rotation the default. Default thresholds are 5 rotated files with a target size of 20MiB. Truncating behavior can be achieved by setting filecount to 0 (-Xlog::myfile.log::filecount=0). src/share/vm/logging/logFileOutput.hpp: + static const size_t DefaultRotationFileSize = 2097152; // 20MiB Missing a 0 here, I prefer to write this as 20*1024*1024 If a log file already exists during log file initialization it will be rotated. If any of the target file names (file.0 to file.4 in the default case) are available, that filename will be used for the existing log. If all names are taken the VM will attempt to overwrite the oldest file. This should prevent unlimited log file creations and avoid accidental loss of log files from previous runs. The default thresholds (5 files, 20MiB each) is just a suggestion. If you think it should be higher/lower let me know. Tested with included internal VM tests through RBT. I also functional tested this and it works as intended. There is an issue regarding removing the current log file. Discussed in side-channel it will be handled outside this CS since it not directly related. Otherwise looks good! /Robbin Thanks, Marcus On 2016-03-01 15:05, Marcus Larsson wrote: Hi, After some offline discussions I'm withdrawing this patch. I will instead investigate if I can achieve similar behavior using log rotation as the default. Thanks, Marcus On 03/01/2016 12:11 PM, Marcus Larsson wrote: Hi again, Taking a different approach to this. New webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/ Existing files will now by default be renamed/archived with a .X suffix where X is the lowest number such that the resulting file name is available (jvm.log becomes jvm.log.0). A mode option for controlling this behavior has been added as well. It can be set to archive, append, or truncate (i.e. -Xlog::jvm.log::mode=truncate). Tested with included jtreg test through JPRT. Thanks, Marcus On 01/14/2016 04:00 PM, Marcus Larsson wrote: Hi, Please review the following patch to make sure UL truncates existing log files before writing to them. Since files are opened in append mode, truncation isn't done automatically, so instead the patch adds an attempt to remove the log file before opening it. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8146879 Testing: Included test through JPRT Thanks, Marcus
Re: RFR: 8146879: Add option for handling existing log files in UL
Hi, On 2016-03-11 15:35, Bengt Rutisson wrote: Hi Marcus, On 2016-03-11 15:21, Marcus Larsson wrote: Third time's the charm. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/ I had a quick look at the code changes. It is not really my area of the code, so I'll leave to someone else to formally review it. However, I downloaded the patch a played a bit with the logging. This is much more like the way I would like it! Thanks! So, from a functional perspective this looks good to me. Thanks for the feedback! Marcus Thanks, Bengt This patch makes log file rotation the default. Default thresholds are 5 rotated files with a target size of 20MiB. Truncating behavior can be achieved by setting filecount to 0 (-Xlog::myfile.log::filecount=0). If a log file already exists during log file initialization it will be rotated. If any of the target file names (file.0 to file.4 in the default case) are available, that filename will be used for the existing log. If all names are taken the VM will attempt to overwrite the oldest file. This should prevent unlimited log file creations and avoid accidental loss of log files from previous runs. The default thresholds (5 files, 20MiB each) is just a suggestion. If you think it should be higher/lower let me know. Tested with included internal VM tests through RBT. Thanks, Marcus On 2016-03-01 15:05, Marcus Larsson wrote: Hi, After some offline discussions I'm withdrawing this patch. I will instead investigate if I can achieve similar behavior using log rotation as the default. Thanks, Marcus On 03/01/2016 12:11 PM, Marcus Larsson wrote: Hi again, Taking a different approach to this. New webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/ Existing files will now by default be renamed/archived with a .X suffix where X is the lowest number such that the resulting file name is available (jvm.log becomes jvm.log.0). A mode option for controlling this behavior has been added as well. It can be set to archive, append, or truncate (i.e. -Xlog::jvm.log::mode=truncate). Tested with included jtreg test through JPRT. Thanks, Marcus On 01/14/2016 04:00 PM, Marcus Larsson wrote: Hi, Please review the following patch to make sure UL truncates existing log files before writing to them. Since files are opened in append mode, truncation isn't done automatically, so instead the patch adds an attempt to remove the log file before opening it. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8146879 Testing: Included test through JPRT Thanks, Marcus
RFR: 8150823: UL handles disabling logs incorrectly
Hi, Please review the following patch to fix a problem with disabling log outputs using -Xlog:disable or disable_logging() when there are multiple file outputs configured. The patch also resolves an issue with log file lookups that could cause multiple outputs to be created for the same log file. Log output removal now iterates backwards to not invalidate the loop index, and file outputs are always prepended with "file=" during lookups and listings. Webrev: http://cr.openjdk.java.net/~mlarsson/8150823/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8150823 Testing: Included tests through RBT Thanks, Marcus
Re: RFR: 8146879: Add option for handling existing log files in UL
Hi Marcus, On 2016-03-11 15:21, Marcus Larsson wrote: Third time's the charm. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/ I had a quick look at the code changes. It is not really my area of the code, so I'll leave to someone else to formally review it. However, I downloaded the patch a played a bit with the logging. This is much more like the way I would like it! Thanks! So, from a functional perspective this looks good to me. Thanks, Bengt This patch makes log file rotation the default. Default thresholds are 5 rotated files with a target size of 20MiB. Truncating behavior can be achieved by setting filecount to 0 (-Xlog::myfile.log::filecount=0). If a log file already exists during log file initialization it will be rotated. If any of the target file names (file.0 to file.4 in the default case) are available, that filename will be used for the existing log. If all names are taken the VM will attempt to overwrite the oldest file. This should prevent unlimited log file creations and avoid accidental loss of log files from previous runs. The default thresholds (5 files, 20MiB each) is just a suggestion. If you think it should be higher/lower let me know. Tested with included internal VM tests through RBT. Thanks, Marcus On 2016-03-01 15:05, Marcus Larsson wrote: Hi, After some offline discussions I'm withdrawing this patch. I will instead investigate if I can achieve similar behavior using log rotation as the default. Thanks, Marcus On 03/01/2016 12:11 PM, Marcus Larsson wrote: Hi again, Taking a different approach to this. New webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/ Existing files will now by default be renamed/archived with a .X suffix where X is the lowest number such that the resulting file name is available (jvm.log becomes jvm.log.0). A mode option for controlling this behavior has been added as well. It can be set to archive, append, or truncate (i.e. -Xlog::jvm.log::mode=truncate). Tested with included jtreg test through JPRT. Thanks, Marcus On 01/14/2016 04:00 PM, Marcus Larsson wrote: Hi, Please review the following patch to make sure UL truncates existing log files before writing to them. Since files are opened in append mode, truncation isn't done automatically, so instead the patch adds an attempt to remove the log file before opening it. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8146879 Testing: Included test through JPRT Thanks, Marcus
Re: RFR: 8146879: Add option for handling existing log files in UL
Third time's the charm. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/ This patch makes log file rotation the default. Default thresholds are 5 rotated files with a target size of 20MiB. Truncating behavior can be achieved by setting filecount to 0 (-Xlog::myfile.log::filecount=0). If a log file already exists during log file initialization it will be rotated. If any of the target file names (file.0 to file.4 in the default case) are available, that filename will be used for the existing log. If all names are taken the VM will attempt to overwrite the oldest file. This should prevent unlimited log file creations and avoid accidental loss of log files from previous runs. The default thresholds (5 files, 20MiB each) is just a suggestion. If you think it should be higher/lower let me know. Tested with included internal VM tests through RBT. Thanks, Marcus On 2016-03-01 15:05, Marcus Larsson wrote: Hi, After some offline discussions I'm withdrawing this patch. I will instead investigate if I can achieve similar behavior using log rotation as the default. Thanks, Marcus On 03/01/2016 12:11 PM, Marcus Larsson wrote: Hi again, Taking a different approach to this. New webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/ Existing files will now by default be renamed/archived with a .X suffix where X is the lowest number such that the resulting file name is available (jvm.log becomes jvm.log.0). A mode option for controlling this behavior has been added as well. It can be set to archive, append, or truncate (i.e. -Xlog::jvm.log::mode=truncate). Tested with included jtreg test through JPRT. Thanks, Marcus On 01/14/2016 04:00 PM, Marcus Larsson wrote: Hi, Please review the following patch to make sure UL truncates existing log files before writing to them. Since files are opened in append mode, truncation isn't done automatically, so instead the patch adds an attempt to remove the log file before opening it. Webrev: http://cr.openjdk.java.net/~mlarsson/8146879/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8146879 Testing: Included test through JPRT Thanks, Marcus
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
> ]
> *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
> 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
>
>
>
>
>
>
>
>
>
>
>
RFR: JDK-8151709: jhsdb should show help message in SALauncher.
Hi all, jhsdb shows error message in each tool implemantation as below: -- $ jhsdb jstack -aaa Usage: jstack [option] (to connect to a live java process) or jstack [option] (to connect to a core file) or jstack [option] [server_id@] (to connect to a remote debug server) where option must be one of: -l to print java.util.concurrent locks -m to print both java and native frames (mixed mode) -h | -help to print this help message -- If we run SA tool via jhsdb, we should get help message of SALauncher as below: -- $ jhsdb jstack -aaa --locks to print java.util.concurrent locks --mixed to print both java and native frames (mixed mode) --exe executable image name --core path to coredump --pid pid of process to attach -- I uploaded webrev. Could you review it? http://cr.openjdk.java.net/~ysuenaga/JDK-8151709/webrev.00/ I cannot access JPRT. So I need a Sponsor. Thanks, Yasumasa
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/
;
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
> 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
> 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/
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/
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/
> 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
>
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/
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/
