[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    cr7154822 : Request for code review
From:       daniel.daugherty () oracle ! com (Daniel D !  Daugherty)
Date:       2012-03-30 12:56:06
Message-ID: 4F75AD66.3040907 () oracle ! com
[Download RAW message or body]

On 3/29/12 10:23 PM, David Buck wrote:
> 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/

Looks good to me. Ship it!


src/share/classes/sun/tools/jcmd/JCmd.java
test/sun/tools/jcmd/dcmd-big-script.txt
test/sun/tools/jcmd/jcmd-big-script.sh
     No comments on the above files.


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

[prev in list] [next in list] [prev in thread] [next in thread]