cr7154822 : Request for code review (original) (raw)

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Mar 29 07:19:30 PDT 2012

On 3/29/12 5:49 AM, David Buck wrote:

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 arglengthmax 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?

I'm not fond of the idea that the client side "knows" the VM limit. Yes, I recognize that would improve the user experience, but it makes the code more fragile. I don't think there is a client-side query for determining the buffer length. Perhaps you should query Alan Bateman for advice since the attach-on-demand stuff was his creating way back when... He might have some advice...

> 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.

Hmmm... But what should happen when a blank line is sent and do we have a test for that... :-)

Dan

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/viewbug.do?bugid=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 jdktools tests were run and passed. Cheers, -Buck

More information about the serviceability-dev mailing list