RFR: 7164841: Improvements to the GC log file rotation
Thomas Schatzl
thomas.schatzl at oracle.com
Mon Sep 16 11:13:42 PDT 2013
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
More information about the hotspot-runtime-dev
mailing list