cr7154822 : Request for code review

[David Buck]

Hi Dan,

Thanks for the review!

 > src/share/classes/sun/tools/jcmd/JCmd.java
 > line 145: would split("$") work here? My memory is rusty but I
 > think that catches all forms of line termination.

Good point, "$" should be more portable and easier to read. Will change.

 > test/sun/tools/jcmd/jcmd-big-script.sh
 > lines 46,61: I don't think '/dev/null' works on MKS and tests are
 > still executed on MKS. grep around the testbase and see
 > how other tests handle /dev/null.

Tests all passed on MKS already. Looks like /dev/null is supported.

[ UNIX-style /dev names for MKS Toolkit ]
http://www.mkssoftware.com/docs/man5/dev.5.asp

Strange, you can't "ls /dev" or "cd /dev", but you can "echo yourmom > 
/dev/null" without any problem.

 > line 48-49: please change to:
 > status="$?"
 > if [ "$status" != 0 ]; then
 > echo "jcmd command returned non-zero exit code (status=$status). Failed."

Thanks. will change.

 > line 54: please redirect output of 'grep' here to /dev/null or the
 > appropriate bit bucket...

Thanks, will change as well.

Cheers,
-Buck

On 03/29/12 23:28, Daniel D. Daugherty wrote:
> Resending... with David B on the reply...
>
>
> On 3/28/12 10:28 PM, 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/
>
> src/share/classes/sun/tools/jcmd/JCmd.java
> line 145: would split("$") work here? My memory is rusty but I
> think that catches all forms of line termination.
>
> test/sun/tools/jcmd/dcmd-big-script.txt
> No comments.
>
> test/sun/tools/jcmd/jcmd-big-script.sh
> lines 46,61: I don't think '/dev/null' works on MKS and tests are
> still executed on MKS. grep around the testbase and see
> how other tests handle /dev/null.
>
> line 48-49: please change to:
> status="$?"
> if [ "$status" != 0 ]; then
> echo "jcmd command returned non-zero exit code (status=$status). Failed."
>
> line 54: please redirect output of 'grep' here to /dev/null or the
> appropriate bit bucket...
>
> Dan
>
>>
>> All the default jprt tests and the jdk_tools tests were run and passed.
>>
>> Cheers,
>> -Buck
>