RFR 8039080: "jinfo server_id at host" fails with "Invalid process identifier"

[Staffan Larsen]

On 3 apr 2014, at 14:47, Jaroslav Bachorik <jaroslav.bachorik at oracle.com> wrote:

> On 3.4.2014 14:33, Staffan Larsen wrote:
>> 
>> On 3 apr 2014, at 14:20, Jaroslav Bachorik <jaroslav.bachorik at oracle.com> wrote:
>> 
>>> On 3.4.2014 14:08, Dmitry Samersoff wrote:
>>>> Jaroslav,
>>>> 
>>>> Command line processing is still not clean, e.g. you check for -help in
>>>> three different places.
>>>> 
>>>> If you go to refactor this code rather than provide a minimal fix,
>>>> it might be better to create a separate inner class that do all command
>>>> line processing and provide couple of flags like should_sa(),
>>>> should_help(), should_attach() etc.
>>> 
>>> Yes, I know :(
>>> 
>>> My first approach was exactly like that. But I was rewriting the command line parser from scratch for at least the 3rd time for "tools" alone :( And the change was going to be rather big - more of a complete rewrite than a fix.
>> 
>> I just re-wrote that command line parser. Obviously with mixed results… :(  I think the real problem here is that this command line is not specified in a way that is easily parseable, and we don’t want to change how it works.
>> 
>> Command line parsing is one of the really hard areas in computer science.
> 
> There are some rather flexible frameworks allowing nice command line parsing in java. But they are all 3rd party utilities.
> 
>> 
>> 
>> You have added two methods that do not look like they are being called: useSA() and getArgs().
> 
> These two methods are there only to make the code testable without resorting to reflection and setAccessible(). Not sure what is the lesser evil …

My bad - I missed the test.

> 
>> 
>> Otherwise the changes look good.
>> 
>> If you haven’t already, make sure sun/tools/jinfo/Basic.sh passes on all platforms and add cases to it if that is appropriate. (Note that it currently fails on solaris JDK-8038296).
> Basic.sh should not be affected by this change at all. But I will make sure the tests pass on all platforms before pushing.

Thanks.

Reviewed.

/Staffan


> 
> -JB-
> 
>> 
>> Thanks,
>> /Staffan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140403/b1ae000f/attachment-0001.html>