Of course…This is getting embarrassing. :)
http://cr.openjdk.java.net/~sla/8039173/webrev.03/
/Staffan
On 11 apr 2014, at 09:32, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote:
> Hi Staffan!
>
> Isn't the second read in line# 268 erroneous?
>
> 267 while ((n = sis.read(b)) != -1) {
> 268 n = sis.read(b);
>
> Sincerely yours,
> Ivan
>
> On 11.04.2014 11:16, Staffan Larsen wrote:
>> Here is a new version where the 200 char cap is removed:
>>
>> http://cr.openjdk.java.net/~sla/8039173/webrev.02/
>>
>> Thanks,
>> /Staffan
>>
>> On 4 apr 2014, at 16:27, Staffan Larsen <staffan.lar...@oracle.com> wrote:
>>
>>> Thanks Ivan.
>>>
>>> I’ve been thinking a bit more about this and discussing with people here.
>>> I’m not sure anymore that it is a good idea to cap the error message. My
>>> original thought was that it made sense to not overflow the client with
>>> tons of output if something went wrong in the target. On the other hand,
>>> there is nothing that says that the error message will be in the first 200
>>> characters. It is perfectly possible for an operation to successfully run
>>> for a while and only at the end run into a problem. In that case the output
>>> will have the exception at the end, following any amount of output. Capping
>>> the message in that case would hide the error.
>>>
>>> (My second thought was: let’s only take the last 200 chars, but there is no
>>> guarantee that that will contain the complete error message either).
>>>
>>> So the "keep it simple” principle here would be to remove the cap (which
>>> also makes the code in getErrorMessage() simpler).
>>>
>>> If someone has a better solution for propagating error messages from the
>>> operations, I’m all ears.
>>>
>>> Thanks,
>>> /Staffan
>>>
>>>
>>>
>>> On 4 apr 2014, at 14:41, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote:
>>>
>>>> An alternative, more compact variant might be
>>>>
>>>> ------------------------
>>>> String getErrorMessage(InputStream sis, int maxlen) throws IOException {
>>>> int n, off = 0, len = maxlen + 1;
>>>> byte b[] = new byte[len];
>>>> while ((n = sis.read(b, off, len - off)) > 0)
>>>> off += n;
>>>> return (off == 0) ? null
>>>> : (off < len)
>>>> ? new String(b, 0, off, "UTF-8")
>>>> : new String(b, 0, maxlen, "UTF-8") + " ...";
>>>> }
>>>> ------------------------
>>>>
>>>> Not a big deal, of course.
>>>>
>>>> Sincerely yours,
>>>> Ivan
>>>>
>>>>
>>>>
>>>> On 04.04.2014 16:24, Ivan Gerasimov wrote:
>>>>> Now you reintroduced the smallish issue, when the message is exactly 200
>>>>> characters (or whatever maxlen is).
>>>>> The dots will be appended to the message, even though it's not necessary.
>>>>>
>>>>> I think the only reliable way to deal with it is to try to read one extra
>>>>> character from sis.
>>>>>
>>>>> Something like this should do:
>>>>> ------------------------
>>>>> String getErrorMessage(InputStream sis, int maxlen) throws IOException {
>>>>> byte b[] = new byte[maxlen + 1];
>>>>> int n, off = 0, len = b.length;
>>>>> do {
>>>>> n = sis.read(b, off, len);
>>>>> if (n == -1) {
>>>>> break;
>>>>> }
>>>>> off += n;
>>>>> len -= n;
>>>>> } while (off < maxlen);
>>>>>
>>>>> String message = null;
>>>>> if (off > 0) {
>>>>> message = (off > maxlen)
>>>>> ? new String(b, 0, maxlen, "UTF-8") + " ..."
>>>>> : new String(b, 0, off, "UTF-8");
>>>>> }
>>>>> return message;
>>>>> }
>>>>> ------------------------
>>>>>
>>>>> Sincerely yours,
>>>>> Ivan
>>>>>
>>>>> On 04.04.2014 16:08, Staffan Larsen wrote:
>>>>>> I’m afraid you are right! Doh. Need to add more testing...
>>>>>>
>>>>>> How about this change:
>>>>>>
>>>>>> --- a/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>> +++ b/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>> @@ -267,9 +267,11 @@
>>>>>> String getErrorMessage(InputStream sis, int maxlen) throws
>>>>>> IOException {
>>>>>> byte b[] = new byte[maxlen];
>>>>>> int n, off = 0, len = maxlen;
>>>>>> + boolean complete = false;
>>>>>> do {
>>>>>> n = sis.read(b, off, len);
>>>>>> if (n == -1) {
>>>>>> + complete = true;
>>>>>> break;
>>>>>> }
>>>>>> off += n;
>>>>>> @@ -280,7 +282,7 @@
>>>>>> if (off > 0) {
>>>>>> message = new String(b, 0, off, "UTF-8");
>>>>>> }
>>>>>> - if (off > b.length && message != null) {
>>>>>> + if (!complete && message != null) {
>>>>>> message += " ...";
>>>>>> }
>>>>>> return message;
>>>>>>
>>>>>>
>>>>>> On 4 apr 2014, at 13:55, Ivan Gerasimov <ivan.gerasi...@oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Thank you Staffan for fixing them!
>>>>>>>
>>>>>>> But I'm afraid that now the function will never add ellipsis to the
>>>>>>> message, even if it gets truncated.
>>>>>>>
>>>>>>> Sincerely yours,
>>>>>>> Ivan
>>>>>>>
>>>>>>> On 04.04.2014 15:47, Staffan Larsen wrote:
>>>>>>>> Thanks for finding these bugs, Ivan!
>>>>>>>>
>>>>>>>> I have updated the webrev at:
>>>>>>>> http://cr.openjdk.java.net/~sla/8039173/webrev.01/, and I have also
>>>>>>>> included the diff below.
>>>>>>>>
>>>>>>>> The updated webrev also has some changes in the javadoc for
>>>>>>>> VirtualMachine to clarify that some methods can now throw
>>>>>>>> AttachOperationFailedException.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Staffan
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>>> b/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>>> --- a/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>>> +++ b/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>>> @@ -266,18 +266,21 @@
>>>>>>>> */
>>>>>>>> String getErrorMessage(InputStream sis, int maxlen) throws
>>>>>>>> IOException {
>>>>>>>> byte b[] = new byte[maxlen];
>>>>>>>> - int n, off = 0, len = b.length;
>>>>>>>> + int n, off = 0, len = maxlen;
>>>>>>>> do {
>>>>>>>> n = sis.read(b, off, len);
>>>>>>>> + if (n == -1) {
>>>>>>>> + break;
>>>>>>>> + }
>>>>>>>> off += n;
>>>>>>>> len -= n;
>>>>>>>> - } while (n >= 0 && off < b.length);
>>>>>>>> + } while (off < maxlen);
>>>>>>>>
>>>>>>>> String message = null;
>>>>>>>> if (off > 0) {
>>>>>>>> message = new String(b, 0, off, "UTF-8");
>>>>>>>> }
>>>>>>>> - if (off == b.length && message != null) {
>>>>>>>> + if (off > b.length && message != null) {
>>>>>>>> message += " ...";
>>>>>>>> }
>>>>>>>> return message;
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4 apr 2014, at 11:18, Ivan Gerasimov <ivan.gerasi...@oracle.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi Staffan!
>>>>>>>>>
>>>>>>>>> I think there is a couple of minor bugs in
>>>>>>>>> getErrorMessage(InputStream sis, int maxlen).
>>>>>>>>>
>>>>>>>>> 1) If maxlen is exactly the size of the message to read, the function
>>>>>>>>> will add an ellipsis, even though the message isn't truncated,
>>>>>>>>> 2) If maxlen is greater than needed, then sis.read(b, off, len) at
>>>>>>>>> the line #271 will eventually return -1, and it will cause the
>>>>>>>>> message to lose its last character.
>>>>>>>>>
>>>>>>>>> Sincerely yours,
>>>>>>>>> Ivan
>>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>
>>
>
