RFR(M): 8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

[serguei.spitsyn at oracle.com]

Hi Christoph and Robbin,

Thank you a lot for review!
A reply below.


On 8/25/17 01:38, Robbin Ehn wrote:
> On 08/25/2017 10:29 AM, Langer, Christoph wrote:
>> Hi Serguei,
>>
>> the change looks good to me, too.
>
> +1
>
> Thanks for fixing Dmitry and Serguei!
>
> /Robbin
>
>>
>> Minor remark: The copyright year in transport.h was not updated.
>>
>> I'm also wondering why the transport layer is only capable of IPv4.

Most likely because it is dated back to the JVMDI time when the IPv6 was 
not popular yet.

>> But that's probably something that would need to be addressed 
>> separately.

Yes, this needs to be addressed separately.

Thanks,
Serguei


>>
>> Best regards
>> Christoph
>>
>>
>>> -----Original Message-----
>>> From: serviceability-dev [mailto:serviceability-dev-
>>> bounces at openjdk.java.net] On Behalf Of serguei.spitsyn at oracle.com
>>> Sent: Donnerstag, 24. August 2017 11:20
>>> To: Dmitry Samersoff <dms at samersoff.net>; serviceability-
>>> dev at openjdk.java.net; Robbin Ehn <robbin.ehn at oracle.com>; Daniel
>>> Daugherty <daniel.daugherty at oracle.com>; Alan Bateman
>>> <Alan.Bateman at oracle.com>
>>> Subject: Re: RFR(M): 8061228 Allow JDWP socket connector to accept
>>> connections from certain ip addresses only
>>>
>>> Thanks, Dmitry!
>>> I'm not sure, who is a reviewer in our case? :)
>>> You are the author, aren't you?
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 8/24/17 01:08, Dmitry Samersoff wrote:
>>>> Serguei,
>>>>
>>>> The changes looks good to me.
>>>>
>>>> Thank you for doing it.
>>>>
>>>> -Dmitry
>>>>
>>>> On 23.08.2017 23:59, serguei.spitsyn at oracle.com wrote:
>>>>> Added Dmitry's email address to the list as he is not subscribed 
>>>>> on the
>>>>> serviceability-dev.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 8/22/17 16:22, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review another revision of the fix for the enhancement:
>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8061228
>>>>>>
>>>>>> CSR:
>>>>>>     https://bugs.openjdk.java.net/browse/CCC-8061228
>>>>>>
>>>>>>     The SCR is in the DRAFT state.
>>>>>>     Joe suggested to consider this CSR approved and gave a GO for
>>>>>> integration.
>>>>>>     It will be moved to the right state later when the CSR tools 
>>>>>> are ready.
>>>>>>     I'm still asking at least one reviewer to look at this CSR 
>>>>>> and give
>>>>>> a thumbs up.
>>>>>>     It is to ensure everything is going in a right direction.
>>>>>>     I'll finalize the CSR after that.
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-
>>> jdi-transport.1/
>>>>>>
>>>>>> The lastest webrev from Dmitry:
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.18/
>>>>>>
>>>>>> Incremental webrev vs the latest webrev from Dmitry:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-
>>> jdi-transport.1.inc/
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>     This enhancement was developed by Dmitry who left the team.
>>>>>>     I don't know what email address to use to CC him at this point.
>>>>>>     I hope, Dmitry will find this discussion and reply accordingly.
>>>>>>     The latest webrev revision from Dmitry was v18 (please, see 
>>>>>> above).
>>>>>>
>>>>>>     This revision covers the following:
>>>>>>       - Cleanup for versioning negotiation protocol (back up to the
>>>>>> original).
>>>>>>         Now the transport library supports both versions 1_0 and 1_1
>>>>>> (newly introduced).
>>>>>>       - The transport native interface was changed.
>>>>>>         The function SetTransportConfiguration() is introduced 
>>>>>> instead
>>>>>> of the
>>>>>>         StartListeningWithAllow(). It allows to the same transport
>>>>>> library to support
>>>>>>         both old and new version of the transport interface. At this
>>>>>> point, the
>>>>>>         new structure jdwpTransportConfiguration includes only 
>>>>>> one field:
>>>>>>            const char* allowed_peers;
>>>>>>         But it can be extended in the future if any other update in
>>>>>> configuration
>>>>>>         will be required.
>>>>>>       - The unit test was updated to provide better coverage of the
>>>>>> corner cases
>>>>>>         for 'allow' option introduced by this enhancement.
>>>>>>       - Fixes to improve diagnosability.
>>>>>>       - A couple of bugs/regressions were fixed so that all the JDI
>>>>>> tests are passed now.
>>>>>>       - A cleanup that includes some renaming and reformatting.
>>>>>>
>>>>>>
>>>>>> Testing:
>>>>>>     Tested new agent flag (allow), with new test:
>>>>>>        jdk/test/com/sun/jdi/BasicJDWPConnectionTest.java
>>>>>>     Ran the nsk.jdi, nsk.jdwp and jtreg jdk_jdi for both release and
>>>>>> fastdebug builds.
>>>>>>     All tests are passed.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>
>>