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

Tue, 03 May 2016 07:07:08 -0700 

Hi Marcus,


That's good, but then we shouldn't limit it to SIZE_MAX. The overflow check in 
logFileOutput.cpp should be removed.


In Linux 32 bits system, we cannot open > 4GB size file because we do not pass 
O_LARGEFILE.
(we use fopen(3))

In manpage [1], we should use _FILE_OFFSET_BITS rather than O_LARGEFILE.
However, makefiles for Linux, this macro is used for ostream.cpp only.

Thus I think we should keep the limit of SIZE_MAX for LogFileOutput.


the question for the UL owners is whether the change in semantics is 
appropriate: previously the filesize was interpreted as a value in KB, whereas 
now, without a suffix, it is just bytes.


Is it no problem?


No I don't think that's a problem.


Thanks!


Yasumasa


[1] http://man7.org/linux/man-pages/man2/open.2.html


On 2016/05/03 22:55, Marcus Larsson wrote:



On 05/03/2016 03: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!
I changed type of _rotate_size and _current_size to julong in new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.05/


That's good, but then we shouldn't limit it to SIZE_MAX. The overflow check in 
logFileOutput.cpp should be removed.




the question for the UL owners is whether the change in semantics is 
appropriate: previously the filesize was interpreted as a value in KB, whereas 
now, without a suffix, it is just bytes.


Is it no problem?


No I don't think that's a problem.

Thanks,
Marcus




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 for the UL 
owners is whether the change in semantics is appropriate: previously the 
filesize was interpreted as a value in KB, whereas now, 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,
Marcus




Thanks,

Yasumasa


On 2016/04/21 18:37, Yasumasa Suenaga wrote:

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.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 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


     >>>>>>>>
     >>>>>>>

































					Reply via email to