On 05/04/2016 03:43 PM, Yasumasa Suenaga wrote:
Thanks, Marcus!into something like if (!success || values > SIZE_MAX) {Also, I think you probably need a static_cast here: 201 _rotate_size = value;I applied them to new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.07/
Looks good! Thanks for fixing this. Marcus
Thanks, Yasumasa On 2016/05/04 21:38, Marcus Larsson wrote:Hi, On 05/04/2016 02:29 PM, Yasumasa Suenaga wrote:Hi David, Marcus, Thank you for your comment. I uploaded new webrev. Could you review it again? http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.06/Can you join these two cases: 193 if (!success) { 194 break; 195 } else if (value > SIZE_MAX) { into something like if (!success || values > SIZE_MAX) {so that we properly fail to configure the output and print the error message when atojulong fails as well?Also, I think you probably need a static_cast here: 201 _rotate_size = value; to avoid compiler warnings on 32-bit platforms. Thanks, MarcusThanks, Yasumasa On 2016/05/04 15:49, Marcus Larsson wrote:Hi, On 05/04/2016 03:45 AM, David Holmes wrote:On 4/05/2016 10:19 AM, Yasumasa Suenaga wrote:Hi David, Marcus,I would not have done that but instead used a temporary julong for the parse result, then range check, then assign to the actual _rotate_sizeetc with a cast.Thanks.However, I guess we will encounter error when we access to > 2GB size file.(Sorry, 4GB is incorrect.)That would seem to be a different issue.I do not investigate for it yet. So I'm not sure whether it is correct. In terms of this issue, should I keep range check and use julong temporaryvalue for _rotate_size?I would go this route yes. Use the Arguments code to do the parsing; apply the same range check as exists today (which may or may not be sufficient, but that is a different issue), then assign.I think that would be better too. Thanks, MarcusThanks, DavidYasumasa On 2016/05/04 5:40, David Holmes wrote:On 3/05/2016 11:49 PM, Yasumasa Suenaga wrote:This seems broken to me, we're passing &_rotate_size (a size_t*) to atojulong() which takes a julong*. If sizeof(julong) > sizeof(size_t)then that's a problem, no?Thanks, Marcus!Yes good catch. I would have hoped the compiler would have complainedabout that on 32-bit.I changed type of _rotate_size and _current_size to julong in new webrev:I would not have done that but instead used a temporary julong for the parse result, then range check, then assign to the actual _rotate_sizeetc with a cast. That is less disruptive than changing the existing types IMHO. Thanks, Davidhttp://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.05/the question for the UL owners is whether the change in semantics is appropriate: previously the filesize was interpreted as a value inKB, whereas now, without a suffix, it is just bytes.Is it no problem? Thanks, Yasumasa On 2016/05/03 21:41, Marcus Larsson wrote:Hi, On 05/03/2016 02:25 PM, David Holmes wrote:Adding in the runtime team as they now own UL.I've reviewed the change from a coding perspective - the question forthe UL owners is whether the change in semantics is appropriate:previously the filesize was interpreted as a value in KB, whereasnow, without a suffix, it is just bytes. Thanks, David On 3/05/2016 9:44 PM, Yasumasa Suenaga wrote:PING: Could you review and sponsor it?http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.04/This seems broken to me, we're passing &_rotate_size (a size_t*) to atojulong() which takes a julong*. If sizeof(julong) > sizeof(size_t)then that's a problem, no? Thanks, MarcusThanks, Yasumasa On 2016/04/21 18:37, Yasumasa Suenaga wrote:Hi David,So it just registered with me that currently filesize is interpretedas a value in KB. With this change it will be in bytes - that meanstests will need fixing eg: hotspot/test/serviceability/logging/TestLogRotation.javaThanks! I've fixed it in new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.04/ Following jtreg tests are passed: - hotspot/test/gc/logging - hotspot/test/runtime/logging - hotspot/test/serviceability/logging Yasumasa On 2016/04/21 14:43, David Holmes wrote: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 greaterthan 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 interpretedas a value in KB. With this change it will be in bytes - that meanstests will need fixing eg: hotspot/test/serviceability/logging/TestLogRotation.java That change in semantics may not be desirable, but I'll leave that tothe 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 itwas someinternally enforced maximum file size limit. So this is just anoverflow 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 greaterthan 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 thepossible options for filesize. Thanks, DavidI 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#l8042016/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 surprisedthat the>> various log file options don't seem to be documentedanywhere in the >> -Xlog:help output. > > I updated help message in new webrev.Thanks, I had missed that example usage buried in there,but am stillsurprised none of these "options" for the handling the fileare 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 withk/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 workwith jdk9/hs not >> hs-rt. >> >> That aside: >> >> src/share/vm/runtime/arguments.cpp >>>> I don't think you need to add the Arguments:: to theatomull 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 surprisedthat the>> various log file options don't seem to be documentedanywhere 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 . Sobuild 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>>>>>>>> >>>>>>>
