cr7154822 : Request for code review

[Daniel D. Daugherty]

On 3/29/12 9:04 PM, David Buck wrote:
> Hi!
>
> I would like to submit a new version of the fix for review:
>
> http://cr.openjdk.java.net/~dbuck/7154822/webrev.01/

src/share/classes/sun/tools/jcmd/JCmd.java
     No comments.

test/sun/tools/jcmd/dcmd-big-script.txt
     No comments.

test/sun/tools/jcmd/jcmd-big-script.sh
     Since your test is using 'set +e' to avoid a premature exit due
     to 'grep' exiting with a non-zero exit code, you also need to
     handle a non-zero exit code from JCMD. I'm guessing that one
     of the common/... script is calling the original 'set -e'.

     Please consider:

     set +e
     ${JCMD} -J-XX:+UsePerfData $appJavaPid -f 
${TESTSRC}/dcmd-big-script.txt > jcmd.out 2>&1
     status="$?"
     set -e

     and:

     set +e #if the test passes, grep will "fail" with an exit code of 1
     grep Exception jcmd.out > /dev/null 2>&1
     status="4?"
     set -e
     if [ "$status" = 0 ]; then

Sorry I missed this the first time.

Dan





>
> Changes:
>
> JCmd.java :
>   1. fixed copyright statement (s/2012/2011, 2012/)
>   2. changed regular expression used to split up script into lines 
> from "\\r?\\n" to "\\n"
>
> dcmd-big-script.txt :
>   3. removed extraneous blank line at end of dcmd-big-script.txt
>
> jcmd-big-script.sh :
>   4. improved output message when jcmd returns a non-zero exit code so 
> that it prints out the exit code jcmd returned.
>   5. redirected stdout/stderr from grep command
>
> Change 2 warrants further explanation. I do not want to use "$" as the 
> regex to split up the input because it will register an additional 
> line between the final command's "\n" and the end of the string. So I 
> would need to add code to remove these additional empty "lines". I see 
> no need to further complicate the code.
>
> If we look at how the command string is generated 
> (src/share/classes/sun/tools/jcmd/Arguments.java), we see that we have 
> already read in the file line by line and then use a StringBuilder to 
> paste it back together (with an added "\n" at the end of each line). 
> So it turns out that the regex I originally chose, "\\r?\\n", was 
> unnecessarily complicated as the string we are reading in is not the 
> raw file, but a string we built ourselves with our own chosen line 
> separator, "\n". Since we know for sure that there is no CRs (\r) 
> dividing our lines, we can (and should) just use "\\n". This way we 
> mirror the way we put the sting together in the first place. Sorry 
> about not catching that sooner.
>
> Hopefully the fix should now be ready to push. Please let me know what 
> you think. Thank you all for the feedback.
>
> Cheers,
> -Buck
>
> On 3/30/2012 3:20 AM, Staffan Larsen wrote:
>>
>> On 29 mar 2012, at 17:11, David Buck wrote:
>>
>>> 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.
>>
>> It may be possible to use the error messages sent from the VM to the 
>> client to do this. See the ATTACH_ERROR_* constants in 
>> attachListener_solaris.cpp for example. Similar things exists on 
>> linux, but apparently not for windows. Anyway, that would be outside 
>> the scope of this fix.
>>
>> Regards,
>> /Staffan