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

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v13]
From:       Serguei Spitsyn <sspitsyn () openjdk ! java ! net>
Date:       2021-01-29 5:39:59
Message-ID: MQdzS56wOfmbxOF8ovgvAqlD6C7_qfbJxVCV9OZ2wro=.edfa23f7-6597-4b54-88df-3aadd0ff2013 () github ! com
[Download RAW message or body]

On Fri, 29 Jan 2021 05:14:31 GMT, Lin Zang <lzang@openjdk.org> wrote:

> > Hi Lin,
> > 
> > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
> > 
> > Thank you for the update.
> > 
> > The list of possible commands must include one more variant with no arguments:
> > +                     * Possible command:
> > +                     *     dumpheap gz=1 file;
> > +                     *     dumpheap gz=1;
> > +                     *     dumpheap file;
> > +                     *     dumpheap
> > 
> > The argument processing is still not right.
> > Why are there no checks for gzlevel < 0 and gzlevel > 9 ?
> > 
> > I'd suggest the following refactoring below:
> > 
> > /*
> > * Possible commands:
> > *     dumpheap gz=1 file
> > *     dumpheap gz=1
> > *     dumpheap file
> > *     dumpheap
> > *
> > * Use default filename if cntTokens == 0.
> > * Handle cases with cntTokens == 1 or 2.
> > */
> > 
> > if (cntTokens > 2) {
> > err.println("Too big number of options: " + cntTokens);
> > usage();
> > return;
> > }
> > if (cntTokens >= 1) { // parse first argument which is "gz=" option
> > String option  = t.nextToken();
> > gzlevel = parseHeapDumpCompressionLevel(option);
> > if (gzlevel <= 0 || gzlevel > 9) {
> > err.println("Invalid "gz=" option: " + option);
> > usage();
> > return;
> > }
> > filename = "heap.bin.gz";
> > }
> > if (cntTokens == 2) { // parse second argument which is filename
> > filename = t.nextToken();
> > if (filename.startsWith("gz=")) {
> > err.println("Filename should not start with "gz=": " + filename);
> > usage();
> > return;
> > }
> > }
> 
> Hi @sspitsyn,
> Thanks for your suggestion, the parseHeapDumpCompresslevel() inside tested the \
> gzlevel, but I think your code is much reasonable. I will make it in next update.  \
> BRs, Lin

You are right.
So, in my suggestion just replace this block:
                        if (gzlevel <= 0 || gzlevel > 9) {
                            err.println("Invalid "gz=" option: " + option);
                            usage();
                            return;
                        }
with:
                        if (gzlevel == 0) {
                            usage();
                            return;
                        }

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712


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