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].
Arguments::atojulong(atomull) checks value range [2].
Thus I do not think we do not need to check value range in
LogFileOutput::parse_options().
> 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?)
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>:
> 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
> >>>>>>>>
> >>>>>>>
>
