RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Thomas,

   Thanks. See embedded.

On 9/16/2013 10:34 AM, Thomas Schatzl wrote:
> Hi Yumin,
>
>    sorry for the delays...
>
> On Thu, 2013-09-12 at 15:57 -0700, Yumin Qi wrote:
>> Thomas,
>>
>>    This is the new version:
>>    http://cr.openjdk.java.net/~minqi/7164841/webrev05
>>    
>>    The change is adding internal tests after make_log_name_internal
>> added to implement the original make_log_name function.
>>
>>    Please let me know if it is OK. JPRT task is under building/testing,
>> so far it is good.
> Looks good.
>
> Some minor things:
> - can you add a test for a string not containing any of the markers?
> (E.g. I used
>
>    // %.log
>    jio_snprintf(i_result, sizeof(char)*FILENAMEBUFLEN, "%%.log", pid);
>    o_result = make_log_name_internal("%.log", NULL, pid, tms);
>    assert(strcmp(i_result, o_result)==0, "failed on testing make_log_name(\"%.log\", NULL)");
>    FREE_C_HEAP_ARRAY(char, o_result, mtInternal);
>
>    // empty string
>    jio_snprintf(i_result, sizeof(char)*FILENAMEBUFLEN, "", pid);
>    o_result = make_log_name_internal("", NULL, pid, tms);
>    assert(strcmp(i_result, o_result)==0, "failed on testing make_log_name(\"\", NULL)");
>    FREE_C_HEAP_ARRAY(char, o_result, mtInternal);
>
>    // %%p.log
>    jio_snprintf(i_result, sizeof(char)*FILENAMEBUFLEN, "%%pid%u.log", pid);
>    o_result = make_log_name_internal("%%p.log", NULL, pid, tms);
>    assert(strcmp(i_result, o_result)==0, "failed on testing make_log_name(\"%%p.log\", NULL)");
>    FREE_C_HEAP_ARRAY(char, o_result, mtInternal);
Those tests will not fail with current code --- we did not check the 
validity of characters here instead they are check in function 
is_filename_valid(const char *filename) which in arguments.cpp.
> which pass btw)
>
> - in jni.cpp, the new include of ostream.hpp should probably be added to
> the ones private to the testing code (e.g. jni.cpp line ~5040).
> - in the string constant beginning in line 2886 I would prefer having if
> no line started with a space - but that's really picky.
OK, arguments.cpp:2868 ? Will move the space to end of constant in line 2867
> - in the tests the comparison of the result of strcmp to equal 0 should
> probably have space before and after the "=="
Will change
> Thanks,
> Thomas
>
>
>