Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v13]

[Lin Zang]

On Fri, 29 Jan 2021 05:33:19 GMT, Serguei Spitsyn  wrote:

> 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;
> }
> ```

Thanks, just found there can be two prints of same error for option like gz=abc,
I will change back:)

-

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

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v13]

[Serguei Spitsyn]

On Fri, 29 Jan 2021 05:14:31 GMT, Lin Zang  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