cr7154822 : Request for code review

[David Buck]

Hi!

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

I talked with Dan over IM about this, and feel that it is not worth 
pursuing. There is no obvious interface for a client to dynamically 
query the limit. And when an exception is thrown, there is no obvious 
way for the client to distinguish between having sent a string that was 
too large and some other IO error. Hard coding this value into the 
client is just asking for trouble. Ideally, jcmd should be 
inter-operable with multiple versions of the JVM.

Thanks for all the feedback. Will rebuild / re-test and get a new 
version out ASAP.

Cheers,
-Buck

On 03/29/12 23:29, Daniel D. Daugherty wrote:
> Resending... with David B on the reply...
>
>
> 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 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?
>
> 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/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
>>>
>
>