<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Calvin,<br>
Thanks for the review,<br>
See embedded.<br>
<br>
<div class="moz-cite-prefix">On 8/16/2013 6:26 PM, Calvin Cheung
wrote:<br>
</div>
<blockquote cite="mid:520ED141.6050108@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Couple of minor comments in
ostream.cpp:<br>
<br>
1) <br>
<span class="new"> 479 char time_msg[256];<br>
480 char time_str[64];<br>
481 char renamed_file_name[256];<br>
<br>
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.<br>
<br>
In the same file, there's <br>
static const int buffer_length = 32;<br>
in outputStream::date_stamp().<br>
<br>
I'm thinking you can have a #define for the size for time_str
and use the same #define in </span><br>
<span class="new"><span class="new">outputStream::date_stamp().<br>
<br>
</span></span></div>
</blockquote>
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.<br>
<blockquote cite="mid:520ED141.6050108@oracle.com" type="cite">
<div class="moz-cite-prefix"><span class="new"><span class="new">
2)<br>
int jio_snprintf(char *str, size_t count, const char *fmt,
...)<br>
<br>
The return value of jio_snprintf() isn't checked. <br>
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.<br>
<br>
</span></span></div>
</blockquote>
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. <br>
<blockquote cite="mid:520ED141.6050108@oracle.com" type="cite">
<div class="moz-cite-prefix"><span class="new"><span class="new">
3)<br>
</span></span><br>
<span class="new"><span class="new"><span class="new">516 if
(access(renamed_file_name, NOT_WINDOWS(F_OK)
WINDOWS_ONLY(0)) == 0) {</span> <br>
</span></span><br>
<span class="new"><span class="new">543 if
(access(renamed_file_name, NOT_WINDOWS(F_OK)
WINDOWS_ONLY(0)) == 0) {<br>
<br>
You can use ERROR_SUCCESS instead of 0 for the WINDOWS_ONLY
part.<br>
<br>
</span></span></div>
</blockquote>
Using <span class="new"><span class="new"> 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++:<br>
/* File attribute constants for _findfirst() */<br>
<br>
#define _A_NORMAL 0x00 /* Normal file - No read/write
restrictions */<br>
#define _A_RDONLY 0x01 /* Read only file */<br>
#define _A_HIDDEN 0x02 /* Hidden file */<br>
#define _A_SYSTEM 0x04 /* System file */<br>
#define _A_SUBDIR 0x10 /* Subdirectory */<br>
#define _A_ARCH 0x20 /* Archive file */<br>
<br>
But I would like to use '0' here to not make the symbol
complicate for reading.<br>
<br>
Thanks<br>
Yumin<br>
</span></span>
<blockquote cite="mid:520ED141.6050108@oracle.com" type="cite">
<div class="moz-cite-prefix"><span class="new"><span class="new">
Calvin<br>
</span></span><br>
On 8/15/2013 8:35 AM, Yumin Qi wrote:<br>
</div>
<blockquote cite="mid:520CF52B.6050000@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
Hi, <br>
<br>
Can I have your review for this small changes?<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/">http://cr.openjdk.java.net/~minqi/7164841/webrev00/</a><br>
<br>
This is for a enhancement to add head/tail message to the
logging files to assist reading GC output.<br>
1. modified prompt message if invalid arguments used for log
rotating;<br>
2. add time and file name message to log file head/tail.<br>
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<br>
On Windows, there is no F_OK (existing test) definition,
F_OK is defined as "0" and for _access of VC++, it just
describes:<br>
<br>
<table style="border-collapse: collapse; padding: 0px; width:
1306px; border: 1px solid rgb(187, 187, 187); color: rgb(0, 0,
0); font-family: 'Segoe UI', 'Lucida Grande', Verdana, Arial,
Helvetica, sans-serif; font-size: 13px; font-style: normal;
font-variant: normal; font-weight: normal; letter-spacing:
normal; line-height: 17px; orphans: auto; text-align: start;
text-indent: 0px; text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px; -webkit-text-stroke-width:
0px;">
<tbody>
<tr>
<th style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; background-color: rgb(237, 237,
237); color: rgb(112, 112, 112); text-align: left;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;"><span class="parameter" style="font-style:
italic;">mode</span><span
class="Apple-converted-space"> </span>value</p>
</th>
<th style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; background-color: rgb(237, 237,
237); color: rgb(112, 112, 112); text-align: left;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">Checks file for</p>
</th>
</tr>
<tr>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">00</p>
</td>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">Existence only</p>
</td>
</tr>
<tr>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">02</p>
</td>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">Write-only</p>
</td>
</tr>
<tr>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">04</p>
</td>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">Read-only</p>
</td>
</tr>
<tr>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">06</p>
</td>
<td style="border: 1px solid rgb(187, 187, 187); margin:
10px; padding: 10px 8px; color: rgb(42, 42, 42);
vertical-align: top;">
<p style="color: rgb(42, 42, 42); margin-top: 0px;
margin-bottom: 0px; padding-bottom: 0px; line-height:
18px;">Read and write</p>
</td>
</tr>
</tbody>
</table>
<br>
<a moz-do-not-send="true"
href="http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx">http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx</a><br>
The definition are consistent with unistd.h. <br>
<br>
Test: JPRT and jtreg.<br>
<br>
Thanks<br>
Yumin<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>