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;
}
cheers
> On Mar 18, 2016, at 8:04 AM, Marcus Larsson <[email protected]> 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
>>>>
>>>
>>
>
