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

[serguei.spitsyn at oracle.com]

Dmitry,


On 5/18/17 04:33, Dmitry Samersoff wrote:
> Serguei,
>
> *a*
>
>> 4. The suggested *allow* feature is too destructive for the
>> transport API. It makes the original function StartListening unusable
>> and its entry slot a garbage. It will become a big mess if we add
>> more function clones like StartListening11. . . . 6. The suggested
>> API update makes the transport API VERSION_1_1 incompatible with the
>> initial VERSION_1_0.
> Transport 1.0 uses StartListening slot. Transport 1.1 uses
> StartListeningWithAllow slot.
>
> I don't see any difference between this approach and approach of adding
> extra slot and extra function.
>
> Transport 1.0 will continue to work as is (ever without recompilation).
> Transport 1.1 will crash if it doesn't fill proper slot.

It is not good enough.
The Transport 1.1 can also support and be compatible with the 1.0 API.
It works well for both alternate approaches.
It does not work in your case (see the webrev.18).

The API will become ugly and messy if more function clones
like the StartListening clones are added in the future.
The old duplicates hold the API slots that become unusable.



> *b*
>
> I'm against separate Allow() call because:
>
> 1) Socket communication steps is well known. Extra step (call Allow
> before StartListen) is not natural.

I'm rejecting this argument.
The StartListeningWithAllow step is also not well known and natural.
There is no problem to add the AllowPeers step.


> 2) API have to clear visible and self documenting.
>
> If transport developer doesn't implement Allow() they doesn't
> see any sign of mistake until they starts testing the
> transport with allow parameter.
>
> As opposite unused parameter is clear visible and cause compiler warning.

This argument also looks strange to me.
Why would not the transport developer implement AllowPeers()
if he is trying to implement the transport 1.1 API?
It is the developer responsibility.


> 3) In a future, parsing of allow list might require results of socket
> call (e.g. to distinguish between IPv4 only and IPv4/Ipv6 cases)

There is no point to discuss the IPv6 until the API for it is suggested.
Otherwise, this discussion is too abstract.


> *c*
>
> As for configuration structures, any jdwpConfiguration structure
> requires versioning i.e. see webrev.15. But we decided to go in other
> direction.

(already replied to Robbin)
No need to have a separate versioning for the jdwpConfiguration.
The transport API version will work well for it.


Thanks,
Serguei



>
> -Dmitry
>
>
> On 2017-05-18 13:20, serguei.spitsyn at oracle.com wrote:
>> Hi Dmitry,
>>
>> Let's get to a consensus for the last item on our plate:
>> jdwpTransportError AllowPeers(const char* peers); .VS.
>> jdwpTransportError StartListeningWithAllow(const char* address,
>> char** actual_address, char *allow);
>>
>>
>> I still do not like the StartListening() function clone and its
>> name. It looks like a wrong direction to me.
>>
>> This is a fragment of my bug report comment on this issue:
>>
>> 4. The suggested *allow* feature is too destructive for the
>> transport API. It makes the original function StartListening unusable
>> and its entry slot a garbage. It will become a big mess if we add
>> more function clones like StartListening11. . . . 6. The suggested
>> API update makes the transport API VERSION_1_1 incompatible with the
>> initial VERSION_1_0.
>>
>>
>> One more alternate solution to suggest is to add new function:
>> jdwpTransportError
>> SetTransportConfiguration(jdwpTransportConfiguration config);
>>
>> Where: typedef struct { const char* allowed_peers; }
>> jdwpTransportConfiguration;
>>
>>
>> This approach allows to extend the jdwpTransportConfiguration in the
>> future if necessary. However, I'm not sure we really need this
>> ability.
>>
>>
>> Both alternatives allow for the transport library to support both
>> API versions. It is good by itself and also, it is a way to simplify
>> the transport library.
>>
>> We could formulate the following requirements to the API updates: -
>> the transport API update must support previous API versions - the
>> updated API layout must be compatible with previous versions
>>
>> It seems to me, these requirements are achievable and two alternate
>> approaches show it.
>>
>> Please, share your opinions?
>>
>>
>> Thanks, Serguei