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
