RFR (S) 8025638: jmap returns 0 instead of 1 when it fails.

[Kevin Walls]

Hi Fredrik,

Yes, I think that looks good.

Thanks!
Kevin


On 15/10/13 13:48, Fredrik Arvidsson wrote:
> I have created a new review of this with the changes suggested.
>
> The review can be found here:
> http://cr.openjdk.java.net/~allwin/farvidss/8025638/webrev.01/ 
> <http://cr.openjdk.java.net/%7Eallwin/farvidss/8025638/webrev.01/>
>
> /F
>
> On 2013-10-10 16:49, Kevin Walls wrote:
>>
>> Hi,
>>
>> Yes the removal of System.exit didn't take the error value into 
>> account.  8010278 removed the System.exit so these Tool classes were 
>> usable by other tools.  I suppose we can use it in main() as if we're 
>> there we should be running as a standalone app.
>>
>> The execute method seems like a nice simplification for the Tools.  
>> An external app that wants to call one of the Tools can't use it, as 
>> it may call System.exit (and you don't want to make execute return an 
>> int as then every Tool class has to have more logic in its main() ).
>>
>> nits in Tool.java
>> 115 {  should be on the same line as execute definition begins.
>> 120 } finally { on one line
>>
>> I don't think stop() should become private, an existing app that used 
>> "public void start()" would expect to call a public stop().
>>
>> There are a few other files that might want looking at to get them 
>> returning an error code: CLHSDB and HSDB (CommandProcessor looks OK, 
>> it only had System.exit(0) removed.)  That could be a separate bug...
>>
>> Thanks!
>> Kevin
>>
>>
>> On 10/10/13 12:49, Staffan Larsen wrote:
>>> Thanks for doing this!
>>>
>>> Tool.java:116 - shouldn't the default return value be 1? In case 
>>> start() throws an exception for some reason.
>>>
>>> Tool.java: I find the start(String[] args), start(), startInternal() 
>>> methods confusing in naming and usage. Not directly related to your 
>>> change of course, just a comment on the code.
>>>
>>> /Staffan
>>>
>>>
>>> On 10 okt 2013, at 13:38, Staffan Larsen <staffan.larsen at oracle.com 
>>> <mailto:staffan.larsen at oracle.com>> wrote:
>>>
>>>> Clicking on the link in this email takes me to the wrong webrev. 
>>>> The correct URL is in the text: 
>>>> http://cr.openjdk.java.net/~allwin/farvidss/8025638/webrev.00/ 
>>>> <http://cr.openjdk.java.net/%7Eallwin/farvidss/8025638/webrev.00/>
>>>>
>>>> /Staffan
>>>>
>>>> On 10 okt 2013, at 12:42, Fredrik Arvidsson 
>>>> <fredrik.arvidsson at oracle.com 
>>>> <mailto:fredrik.arvidsson at oracle.com>> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> Please help me to review the changes below:
>>>>>
>>>>> Jira case: https://bugs.openjdk.java.net/browse/JDK-8025638 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8024423>
>>>>> Webrev: 
>>>>> http://cr.openjdk.java.net/~allwin/farvidss/8025638/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/>
>>>>>
>>>>> About this change.
>>>>> A previous change 
>>>>> (https://bugs.openjdk.java.net/browse/JDK-8010278) in the 
>>>>> Tool.java class caused any tool deriving from this base class 
>>>>> return the wrong value to the caller when failing.
>>>>> Changes were made to the Tool.java class and to the derived tool 
>>>>> implementation classes to handle errors/exceptions during 
>>>>> execution and ensure that the tool returns 1 to the caller if it 
>>>>> fails, and 0 if it succeeds.
>>>>>
>>>>> Previously failed Aurora tests have been run using UTE and 
>>>>> verified to PASS.
>>>>>
>>>>> Cheers
>>>>> /Fredrik
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131015/7a8da349/attachment-0001.html