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 <lzang at 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