cr7154822 : Request for code review

[Daniel D. Daugherty]

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