On 20/04/2016 3:25 PM, Yasumasa Suenaga wrote:
Hi David,
> You still removed the size bounds checks:
>
> 190 size_t value = parse_value(value_str);
> 191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
> 192 errstream->print_cr("Invalid option: %s must be in range
[0, "
> 193 SIZE_FORMAT "]", FileSizeOptionKey,
SIZE_MAX / K);
> 194 success = false;
SIZE_MAX is defined as ULONG_MAX in stdint.h [1].
Ah I hadn't realized this was an external value, I thought it was some
internally enforced maximum file size limit. So this is just an
overflow check really, and ...
Arguments::atojulong(atomull) checks value range [2].
... we already do an overflow check in here, but ...
Thus I do not think we do not need to check value range in
LogFileOutput::parse_options().
... on 32-bit size_t and julong are not the same size so we would
still need to ensure we don't specify a filesize that is greater than
SIZE_MAX on 32-bit.
> Thanks, I had missed that example usage buried in there, but am still
> surprised none of these "options" for the handling the file are
> explicitly documented.
I do not know how we can documented about it.
(Is it internal process in Oracle?)
No I just meant that amongst all that help text you already modified,
there is nothing, that I could see, that actually describes the
possible options for filesize.
Thanks,
David
I can help for it if I can
Thanks,
Yasumasa
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/stdint.h;h=442762728b899aa8ec219299692fce5953d796b0;hb=HEAD#l259
[2]
http://hg.openjdk.java.net/jdk9/hs/hotspot/file/8005261869c9/src/share/vm/runtime/arguments.cpp#l804
2016/04/20 11:24 "David Holmes" <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>:
Hi Yasumasa,
On 19/04/2016 11:50 PM, Yasumasa Suenaga wrote:
> Hi David,
>
> Thank you for your comment.
>
> I uploaded new webrev. Could you review again?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.02/
You still removed the size bounds checks:
190 size_t value = parse_value(value_str);
191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
192 errstream->print_cr("Invalid option: %s must be in
range [0, "
193 SIZE_FORMAT "]",
FileSizeOptionKey,
SIZE_MAX / K);
194 success = false;
>> Which makes me wonder if atomull needs renaming - does the
"m" mean
>> memory? atojulong would seem more appropriate regardless.
>
> I renamed to atojulong() in new webrev.
>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere
in the
>> -Xlog:help output.
>
> I updated help message in new webrev.
Thanks, I had missed that example usage buried in there, but am
still
surprised none of these "options" for the handling the file are
explicitly documented.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/19 10:14, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 19/04/2016 12:06 AM, Yasumasa Suenaga wrote:
>>> PING:
>>>
>>> I've sent review request for JDK-8153073.
>>> Could you review it?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>
>>> If this patch is merged, user can set logfile size with k/m/g.
>>
>> Your webrev seems out of date with respect to the current
code - the
>> logfile size processing is done in
LogFileOutput::parse_options not
>> configure_rotation. And of course you now need to work with
jdk9/hs not
>> hs-rt.
>>
>> That aside:
>>
>> src/share/vm/runtime/arguments.cpp
>>
>> I don't think you need to add the Arguments:: to the atomull
calls when
>> you are executing in Arguments code - lines 2643, 2660
>>
>> This comment could be updated to delete "memory"
>>
>> 788 // Parses a memory size specification string.
>>
>> Which makes me wonder if atomull needs renaming - does the
"m" mean
>> memory? atojulong would seem more appropriate regardless.
>>
>> ---
>>
>> src/share/vm/logging/logFileOutput.cpp
>>
>> You removed the size checking logic.
>>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere
in the
>> -Xlog:help output.
>>
>> Thanks,
>> David
>> -----
>>
>>>
>>> Please review it.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/04/11 18:28, Yasumasa Suenaga wrote:
>>>> PING: Could you review it?
>>>> We need more reviewer.
>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2016/03/31 22:33, Yasumasa Suenaga wrote:
>>>>> CC'ed to serviceability-dev.
>>>>>
>>>>> Could you review it?
>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2016/03/31 18:24, Yasumasa Suenaga wrote:
>>>>>> Hi Marcus,
>>>>>>
>>>>>>> You're missing an include of arguments.hpp in
logFileOutput.cpp.
>>>>>>
>>>>>> arguments.hpp is included in precompiled.hpp . So build was
succeeded.
>>>>>> However, it should be included in logFileOutput.cpp .
>>>>>>
>>>>>> I uploaded a new webrev. Could you review again?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2016/03/31 16:48, Marcus Larsson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 03/30/2016 04:09 PM, Yasumasa Suenaga wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> This request review is related to [1].
>>>>>>>>
>>>>>>>> I want to set filesize option with k/m/g as below:
>>>>>>>>
-Xlog:gc=trace:file=gc.log:time:filecount=5,filesize=10m
>>>>>>>>
>>>>>>>> Memory size option (e.g. -Xmx) can be set with k/m/g .
>>>>>>>> I think we can use option parser in arguments.cpp .
>>>>>>>>
>>>>>>>> I uploaded webrev. Could you review it?
>>>>>>>>
>>>>>>>>
http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.00/
>>>>>>>
>>>>>>> You're missing an include of arguments.hpp in
logFileOutput.cpp.
>>>>>>>
>>>>>>> Apart from that, this looks good to me.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Marcus
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> [1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018704.html
>>>>>>>>
>>>>>>>
