RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Calvin,
   Thanks for the review,
   See embedded.

On 8/16/2013 6:26 PM, Calvin Cheung wrote:
> Couple of minor comments in ostream.cpp:
>
> 1)
>   479   char time_msg[256];
>  480   char time_str[64];
>  481   char renamed_file_name[256];
>
> I think you can replace 256 with MAX_PATH. However, we define MAX_PATH 
> as (2 * K) in unix platforms. So it maybe too large of a buffer for 
> this purpose. On windows, I think it is defined as 256.
>
> In the same file, there's
>     static const int buffer_length = 32;
> in outputStream::date_stamp().
>
> I'm thinking you can have a #define for the size for time_str and use 
> the same #define in
> outputStream::date_stamp().
>
I considered using MAX_PATH but it is too big for this purpose so take 
current solution. For time string we may use buffer_length, I will 
change that.
> 2)
> int jio_snprintf(char *str, size_t count, const char *fmt, ...)
>
> The return value of jio_snprintf() isn't checked.
> It could return -1 from calling _vsnprintf(). However, I don't see the 
> return status being check in the existing code. So I'm not sure if it 
> is a must check.
>
This return value only indicates the writing of chars finished and 
number of chars is less or equals to count (buffer not over flowed). If 
copy up to count and string truncated it is still OK, the buffer will 
contain partial strings but not all. So usually we don't check its 
return value.
> 3)
>
> 516 if (access(renamed_file_name, NOT_WINDOWS(F_OK) WINDOWS_ONLY(0)) 
> == 0) {
>
> 543 if (access(renamed_file_name, NOT_WINDOWS(F_OK) WINDOWS_ONLY(0)) 
> == 0) {
>
> You can use ERROR_SUCCESS instead of 0 for the WINDOWS_ONLY part.
>
Using ERROR_SUCCESS will mislead reading code since it is usually used 
for a return value from Windows API call. In fact there is definitions 
in io.h of Visual C++:
/* File attribute constants for _findfirst() */

#define _A_NORMAL       0x00    /* Normal file - No read/write 
restrictions */
#define _A_RDONLY       0x01    /* Read only file */
#define _A_HIDDEN       0x02    /* Hidden file */
#define _A_SYSTEM       0x04    /* System file */
#define _A_SUBDIR       0x10    /* Subdirectory */
#define _A_ARCH         0x20    /* Archive file */

But I would like to use '0' here to not make the symbol complicate for 
reading.

Thanks
Yumin
> Calvin
>
> On 8/15/2013 8:35 AM, Yumin Qi wrote:
>> Hi,
>>
>>   Can I have your review for this small changes?
>> http://cr.openjdk.java.net/~minqi/7164841/webrev00/ 
>> <http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/>
>>
>>    This is for a enhancement to add head/tail message to the logging 
>> files to assist reading GC output.
>>    1. modified prompt message if invalid arguments used for log rotating;
>>    2. add time and file name message to log file head/tail.
>>    3. for easily identify which log file is current, use file name 
>> like <filename>.n.current, after it reaches maximum size, rename it 
>> to <filename>.n
>>         On Windows, there is no F_OK (existing test) definition, F_OK 
>> is defined as "0" and for _access of VC++, it just describes:
>>
>> modevalue
>>
>> 	
>>
>> Checks file for
>>
>> 00
>>
>> 	
>>
>> Existence only
>>
>> 02
>>
>> 	
>>
>> Write-only
>>
>> 04
>>
>> 	
>>
>> Read-only
>>
>> 06
>>
>> 	
>>
>> Read and write
>>
>>
>> http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx
>> The definition are consistent with unistd.h.
>>
>>     Test: JPRT and jtreg.
>>
>>    Thanks
>>    Yumin
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130816/3314ee0c/attachment.htm>