Thanks for reviewing!
Marcus
On 03/30/2016 06:16 PM, Gerard Ziemski wrote:
Look good.
Thank you for you patience with my review.
cheers
On Mar 30, 2016, at 9:17 AM, Marcus Larsson <marcus.lars...@oracle.com> wrote:
Hi Gerard,
On 03/22/2016 07:15 PM, Gerard Ziemski wrote:
hi Marcus,
Thank you for incorporating the feedback.
The only remaining comment I have is about the
“number_of_lines_with_substring_in_file()” function. As written, it resets the
entire algorithm to the beginning of the file when it can not fit the current
line, when I think the intention should be only to rewind only to the beginning
of that line.
May I suggest the following implementation instead:
static size_t number_of_lines_with_substring_in_file(const char* filename,
const char* substr) {
ResourceMark rm;
size_t ret = 0;
FILE* fp = fopen(filename, "r");
assert(fp, "error opening file %s: %s", filename, strerror(errno));
int buflen = 512;
char* buf = NEW_RESOURCE_ARRAY(char, buflen);
long int pos = 0;
while (fgets(buf, buflen, fp) != NULL) {
if (buf[strlen(buf) - 1] != '\n') {
// rewind to the beginning of the line
fseek(fp, pos, SEEK_SET);
// double the buffer size and try again
buf = REALLOC_RESOURCE_ARRAY(char, buf, buflen, 2*buflen);
buflen *= 2;
} else {
// get current file position
pos = ftell(fp);
if (strstr(buf, substr) != NULL) {
ret++;
}
}
}
fclose(fp);
return ret;
}
Alright, fixed.
New webrev:
http://cr.openjdk.java.net/~mlarsson/8146879/webrev.05/
Incremental:
http://cr.openjdk.java.net/~mlarsson/8146879/webrev.04-05/
Also fixed some failing tests caused by this patch in the previous round of
changes.
Thanks,
Marcus
cheers
On Mar 18, 2016, at 8:04 AM, Marcus Larsson <marcus.lars...@oracle.com> wrote:
Updated patch after feedback.
New webrev:
http://cr.openjdk.java.net/~mlarsson/8146879/webrev.04/
Incremental:
http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03-04/
Tested with internal VM tests through RBT.
Changes:
* Rotation filecount is now limited to 1000 files.
* Removed loop in os::compare_file_modified_times.
* Added a check before rotating/truncating an existing log file, and will only
do so if it's a regular file.
* Added test case to check that logging to a directory gives the intended error
message.
* Fixed test help method to handle arbitrary length log lines.
Thanks,
Marcus
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).
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
