cr7154822 : Request for code review

[David Buck]

Hi Dan,

Thanks you! Not sure how I realized that I needed to set +e for the grep 
command but it did not occur to me to do the same thing for the jcmd 
invocation.

Here is the new webrev:

http://cr.openjdk.java.net/~dbuck/7154822/webrev.02/

Please let me know what you think.

I tested new version of the script locally (solaris/SPARC). Rerunning on 
JPRT just to make sure all platforms are OK. (I'm a sucker for testing.)

Cheers,
-Buck

On 3/30/2012 12:36 PM, Daniel D. Daugherty wrote:
> 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