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

[Fredrik Arvidsson]

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/65026c70/attachment.html