cr7154822 : Request for code review

[David Buck]

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
>