PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
David Holmes
david.holmes at oracle.com
Thu Apr 21 05:43:28 UTC 2016
Hi Yasumasa,
On 20/04/2016 7:15 PM, Yasumasa Suenaga wrote:
> Hi David,
>
>> ... 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.
>
> Oh... I understood.
> I've fixed and uploaded new webrev. Could you review again?
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.03/
So it just registered with me that currently filesize is interpreted as
a value in KB. With this change it will be in bytes - that means tests
will need fixing eg:
hotspot/test/serviceability/logging/TestLogRotation.java
That change in semantics may not be desirable, but I'll leave that to
the owners of this code to decide (and I hope they jump in soon!)
I note that in the existing range check:
if (value == SIZE_MAX || value > SIZE_MAX / K) {
the first clause is redundant. So your change seems okay.
Thanks,
David
-----
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/20 15:04, David Holmes wrote:
>> 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.holmes at oracle.com
>>> <mailto:david.holmes at 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
>>>
>>> >>>>>>>>
>>> >>>>>>>
>>>
More information about the serviceability-dev
mailing list