On 11 apr 2014, at 10:36, Ivan Gerasimov <[email protected]> wrote:
> > On 11.04.2014 11:51, Staffan Larsen wrote: >> Of course…This is getting embarrassing. :) >> >> http://cr.openjdk.java.net/~sla/8039173/webrev.03/ >> > Now it looks fine to me :) Thanks. > Not a reviewer though. > > > There are a few minor formatting glitches, but I don't have a strong opinion > whether they should be changed or not. > > Here they are anyway, in case you need it: > 1) > src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java > 267 while ((n = sis.read(b)) != -1) { > two spaces before the curly brace Will fix. > > 2) > src/solaris/classes/sun/tools/attach/BsdVirtualMachine.java, > src/solaris/classes/sun/tools/attach/LinuxVirtualMachine.java, > src/solaris/classes/sun/tools/attach/SolarisVirtualMachine.java > src/windows/classes/sun/tools/attach/WindowsVirtualMachine.java > 212 } > 213 else { > looks inconsistent with other else clauses in these files. > Shouldn't it be places on the same line? Will fix. > > Personally, I would prefer > throw new AttachOperationFailedException(message != null ? message : "Command > failed in target VM”); I went back and forth on that one. The above would lead to a longer line which I would then have to break and I would be back where I started. I don’t have a strong opinion, though. /Staffan > Sincerely yours, > Ivan > > >> /Staffan >> >> On 11 apr 2014, at 09:32, Ivan Gerasimov <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>>>> 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 >>>>>>>>>>> >>> >> >
