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.spit...@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
