PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Yasumasa Suenaga
yasuenag at gmail.com
Wed Apr 20 09:15:15 UTC 2016
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/
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