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

[Chris Plummer]

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".
> Do we change the issue type from bug to rfe?
Not sure. I suppose confusing argument handling could be considered a bug.
> 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/
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>