Hi Frederic,
Thank you for the review!
> I have a question about the 1024 bytes limit.
> Have you tested this limit on the different platforms?
I tested on Solaris, Linux and Windows. They were all 1024.
> Because jcmd uses the attachAPI to send commands to
> the target JVM and each platform uses a different mechanism
> to implement the attachAPI (Solaris->door, Linux->socket,
> Windows->Pipe). I can see the 1024 bytes limit on the
> Windows implementation, but I don't see hard coded values
> on other platforms. Do you know what is the limit for
> each platform?
The 1024 byte limit is hard-coded into hotspot. See arg_length_max in
hotspot/src/share/vm/services/attachListener.hpp.
> If the limit is the same for all platforms, then the fix
> could be improved to check that each line is smaller than the
> limit before to send it to the target JVM, and properly
> report an error if it isn't. Right now, each platform
> throws an IOException with a platform dependent message.
> Instead, detecting lines exceeding the limit and printing
> a clear message like:
>
> "line 84: Line too long"
>
> would improve the user experience.
That should be doable. Are we OK with hard-coding the current VM limit
into the client side code?
> src/share/classes/sun/tools/jcmd/JCmd.java
> Copyright years should be "2011, 2012" instead of "2012"
Sorry, not sure what I was thinking. Fixed.
> test/sun/tools/jcmd/dcmd-big-script.txt
> The empty line at the end of the line might cause the
> VM to complain about an unknown diagnostic command
After the fix, blank lines no longer throw that error, but still, the
blank line was unintentional. I removed it.
Cheers,
-Buck
On 03/29/12 19:34, Frederic Parain wrote:
Hi David,
I have a question about the 1024 bytes limit.
Have you tested this limit on the different platforms?
Because jcmd uses the attachAPI to send commands to
the target JVM and each platform uses a different mechanism
to implement the attachAPI (Solaris->door, Linux->socket,
Windows->Pipe). I can see the 1024 bytes limit on the
Windows implementation, but I don't see hard coded values
on other platforms. Do you know what is the limit for
each platform?
If the limit is the same for all platforms, then the fix
could be improved to check that each line is smaller than the
limit before to send it to the target JVM, and properly
report an error if it isn't. Right now, each platform
throws an IOException with a platform dependent message.
Instead, detecting lines exceeding the limit and printing
a clear message like:
"line 84: Line too long"
would improve the user experience.
A few comments on the code:
src/share/classes/sun/tools/jcmd/JCmd.java
Copyright years should be "2011, 2012" instead of "2012"
test/sun/tools/jcmd/dcmd-big-script.txt
The empty line at the end of the line might cause the
VM to complain about an unknown diagnostic command
test/sun/tools/jcmd/jcmd-big-script.sh
No comment
Regards,
Fred
On 03/29/12 06:28 AM, David Buck wrote:
Hi!
Please review my fix for the following bug :
[ Bug ID: 7154822 forward port fix for Bug 13645891 to JDK8 jcmd (1024
byte file size limit issue) ]
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7154822
The issue is there is an arbitrary limit in the size of a script file
you pass to the jcmd command (via the -f option) of 1024 bytes. The
solution is for jcmd to break up the input file into individual lines
and send them one at a time to the jvm.
A similar fix has already been done for JRockit's jrcmd command and will
be released in R28.2.3.
[ jdk ]
http://cr.openjdk.java.net/~dbuck/7154822/webrev.00/
All the default jprt tests and the jdk_tools tests were run and passed.
Cheers,
-Buck
