'Re: PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
From:       Yasumasa Suenaga <yasuenag () gmail ! com>
Date:       2016-04-21 9:37:00
Message-ID: 57189F3C.8070300 () gmail ! com
[Download RAW message or body]

Hi David,

> 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

Thanks!
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 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@oracle.com
> > > > <mailto:david.holmes@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
> > > >  
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic