RFR: 7164841: Improvements to the GC log file rotation

Thomas Schatzl thomas.schatzl at oracle.com
Mon Sep 16 17:34:17 UTC 2013


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

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.
- in the tests the comparison of the result of strcmp to equal 0 should
probably have space before and after the "=="

Thanks,
Thomas






More information about the hotspot-gc-dev mailing list