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