RFR: 7164841: Improvements to the GC log file rotation

[Thomas Schatzl]

Hi,

On Mon, 2013-09-16 at 11:04 -0700, Yumin Qi wrote:
> 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.

Okay, I forgot that there is a restriction on the file name characters.
Still it would be nice to have a simple test without any of the special
characters - e.g. "test.log". Just for completeness.

> > 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