Looks good to me! /Staffan
On 30 mar 2012, at 06:23, 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/ > > 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
