cr7154822 : Request for code review

[David Buck]

Hi!

I would like to submit a new version of the fix for review:

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

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