[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8177763: Getting an hprof dump via jcmd could benefit from stronger option checking
From: Gary Adams <gary.adams () oracle ! com>
Date: 2018-08-30 18:38:48
Message-ID: 5B8839B8.9030609 () oracle ! com
[Download RAW message or body]
I found os::file_separator() that can be used in an absolute
pathname check. There is an isAbsolute function in the
libinstrument native code, but the code we're modifying
is in libjvm.
Here's a quick prototype of an absolute path check...
diff --git a/src/hotspot/share/services/diagnosticCommand.cpp
b/src/hotspot/share/services/diagnosticCommand.cpp
--- a/src/hotspot/share/services/diagnosticCommand.cpp
+++ b/src/hotspot/share/services/diagnosticCommand.cpp
@@ -504,13 +504,30 @@
}
void HeapDumpDCmd::execute(DCmdSource source, TRAPS) {
+ char full_dump_pathname[JVM_MAXPATHLEN];
+ if ((strncmp(_filename.value(), os::file_separator(), 1) == 0)
+#if defined(_WINDOWS)
+ || (strchr(_filename.value(), ':') != NULL)
+#endif
+ ) {
+ // Absolute path
+ strcpy(full_dump_pathname, _filename.value());
+ } else {
+ // Relative path
+ sprintf (full_dump_pathname,"%s%s%s",
+ os::get_current_directory(NULL, 0),
+ os::file_separator(),
+ _filename.value());
+ }
+
// Request a full GC before heap dump if _all is false
// This helps reduces the amount of unreachable objects in the dump
// and makes it easier to browse.
HeapDumper dumper(!_all.value() /* request GC if _all is false*/);
int res = dumper.dump(_filename.value());
if (res == 0) {
- output()->print_cr("Heap dump file created");
+ output()->print_cr("Heap dump file created [%s]",
+ full_dump_pathname);
} else {
// heap dump failed
ResourceMark rm;
@@ -518,7 +535,8 @@
if (error == NULL) {
output()->print_cr("Dump failed - reason unknown");
} else {
- output()->print_cr("%s", error);
+ output()->print_cr("%s [%s]", error,
+ full_dump_pathname);
}
}
}
On 8/29/18, 3:49 PM, Chris Plummer wrote:
> On 8/29/18 12:42 PM, gary.adams@oracle.com wrote:
>> 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.
> Maybe we should just document the preferred way.
>>
>>>> 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.
> JFR is key=value and we didn't have that previously?
>
> Chris
>>
>>>> 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/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic