RFR: JDK-8177763: Getting an hprof dump via jcmd could benefit from stronger option checking

[gary.adams at oracle.com]

On 8/29/18 3:12 PM, Chris Plummer wrote:
> On 8/29/18 11:57 AM, Gary Adams wrote:
>> Here are the items I'd like to discuss.
>>
>>    1. The current help doc only mention <filename>
>>        as a positional argument.
>>        The original bug was posted with an expectation
>>         that <filename=value> should be accepted.
>>        Do we document that we support either form
>>        in the help message?
> That's a good question. Or should we even support both forms. I think 
> other dcmds require filename=<filename>. If we decide to not use your 
> new support for positional arguments specified with key=<value>, then 
> we should do a better job of detecting when this mistake is made and 
> give an appropriate error. I think currently (without your changes) if 
> you specify filename=foo,  you get a file named "filename=foo".
Actually, you get the file named "filename".
The dcmd parsing already split the "key=value" parameter,
but since only a positional argument was expected it took
the <key> portion.

I like supporting either form of the argument.
It's backward compatible if people are already using it correctly.
And it's forward compatible with the syntax of the JFR key=value
expectations.

>> Do we change the issue type from bug to rfe?
> Not sure. I suppose confusing argument handling could be considered a 
> bug.
Actually it's not confusing. It matches what the help text describes.
The confusion was introduced when the JFR commands introduced
a different approach to parameters.

>> Do we need a CSR if the dcmd syntax is changed?
> I think yes.
>>
>>   2. Do we have a native function that will declare a
>>       path as absolute or relative to the current directory?
>>       Worried a little about windows paths here.
> I don't know. There probably is one to canonicalize a path.
>
> Chris
>>
>>   3. Looks like minimal heap_dump testing here:
>> open/test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpTest.java
>> ...
>>
>> On 8/29/18, 2:04 PM, Chris Plummer wrote:
>>> Hi Gary,
>>>
>>> Ok. Overall looks good. A couple of comments:
>>>
>>> You now output the path of the created file, but in a kind of 
>>> strange way. You print the CWD, and then the specified path, whether 
>>> relative or full. So it's kind of confusing because if the user 
>>> specifies a full path, they'll still see the CWD printed out. Why 
>>> not just print the full path (after combining with CWD if needed), 
>>> or maybe omit the CWD off when a full path was specified. If 
>>> determining if CWD ends up getting used is difficult, maybe just 
>>> make it more clear what you are printing is the CWD, and it is not 
>>> necessarily part of the actual filename path.
>>>
>>> What about writing some tests cases. It would probably be pretty 
>>> easy to update some of the existing test cases to use key=<value> 
>>> where previously they just allowed <value>.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>>
>>> On 8/29/18 8:10 AM, Gary Adams wrote:
>>>> Sure no problem.
>>>>
>>>> I have two workspaces I use called jdk-jdb and jdk-jdk.
>>>> I have the changes in the jdk-jdb workspace for the heap
>>>> filename updates.
>>>>
>>>> In the jdk-jdb workspace a launch a process that just waits for input.
>>>>
>>>>    jdk/bin/java -cp ~/classes my
>>>>
>>>> In the jdk-jdk workspace I issue the jcmd to talk to the java process
>>>> running in the jdk-jdb workspace. The updated output includes
>>>> "[<current-directory>,<heap dump filename>]"
>>>>
>>>> ...
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump filename=foo
>>>> 2328:
>>>> Heap dump file created 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,foo]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump filename
>>>> 2328:
>>>> Heap dump file created 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,filename]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ffoo2
>>>> 2328:
>>>> Heap dump file created 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,ffoo2]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ffoo2
>>>> 2328:
>>>> File exists [/export/users/gradams/ws/jdk-jdb/build/linux-x64,ffoo2]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ~/ffoo2
>>>> 2328:
>>>> Heap dump file created 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/home/gradams/ffoo2]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ~/ffoo2
>>>> 2328:
>>>> File exists 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/home/gradams/ffoo2]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump /tmp/ffoo2
>>>> 2328:
>>>> Heap dump file created 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/tmp/ffoo2]
>>>>
>>>> $  jdk/bin/jcmd 2328 GC.heap_dump /tmp/ffoo2
>>>> 2328:
>>>> File exists 
>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/tmp/ffoo2]
>>>>
>>>> $ pwd
>>>> /home/gradams/scratch/ws/jdk-jdk/build/linux-x64
>>>>
>>>>
>>>> On 8/29/18, 2:21 AM, Chris Plummer wrote:
>>>>> Hi Gary,
>>>>>
>>>>> What would be really useful are some before/after examples that 
>>>>> demonstrate what has changed from the users point of view.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 8/28/18 5:26 AM, Gary Adams wrote:
>>>>>> This message may have been lost in the vacation email backlog, so 
>>>>>> I'm
>>>>>> sending it again.
>>>>>>
>>>>>> On 8/9/18, 2:19 PM, Gary Adams wrote:
>>>>>>> Thee are several reported problems using jcmd for obtaining heap 
>>>>>>> dumps.
>>>>>>>
>>>>>>> Some users are confused by the positional argument for a filename
>>>>>>> when a similar jfr command uses "key=value" syntax.
>>>>>>>
>>>>>>> Some users are confused by the basic nature of the command, 
>>>>>>> where the jvm
>>>>>>> executing the heap dump is running in a different current 
>>>>>>> directory than
>>>>>>> the jcmd itself.
>>>>>>>
>>>>>>> This is a proposed fix to report the current directory and the 
>>>>>>> filename
>>>>>>> when the heap dump is successfully written or if an error 
>>>>>>> occurs. There
>>>>>>> is also a proposed fix to handle the case when the user provided
>>>>>>> "key=value" inputs. If the key matches the argument name, then the
>>>>>>> user intended the value to be used.
>>>>>>>
>>>>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8177763
>>>>>>>   Webrev: http://cr.openjdk.java.net/~gadams/8177763/webrev.00/
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>